Patina: R-EFI 6.0 migration

[vineelko]

Description

R-EFI 6.0 Migration

Upstream r-efi 6.0 marks all extern "efiapi" function pointers in EFI Boot
Services, Runtime Services, and other protocols as unsafe, since their safety
cannot be enforced by the Rust type system at compile time.

This PR audits almost all usage of UEFI service function pointers across the Patina
codebase for correctness and safety.

• There are three primary areas where unsafe usage has been audited:

  1. The FFI boundary - all extern "efiapi" functions
  2. The Patina wrappers - all functions within the BootServices trait that
     eventually call into the FFI
  3. Core Patina routines that implement the FFI functions

• Not all functions in the above categories need to be marked unsafe. For
  example, extern "efiapi" close_event is not marked unsafe because it can
  safely be called with an arbitrary event parameter without causing undefined
  behavior in the Patina implementation. The event parameter is treated as an
  opaque pointer and is never dereferenced.

  That said, any indirect caller that dereferences this Boot Services function
  pointer must still use an unsafe block, since the function pointer itself is
  defined as unsafe in r-efi 6.0. In addition, inherently unsafe Rust
  functions (such as core_free_pool()) are now explicitly marked unsafe.

• Each inspected function or call site is documented with appropriate safety
  comments where necessary, and with explanations where unsafe is not
  required.

• There are no functional changes.

• Clippy flags public functions that accept raw pointer parameters and pass them
  across an FFI boundary without being marked unsafe, with the following
  warning:
  "this public function might dereference a raw pointer but is not marked unsafe"

  unsafe extern "C" {
      fn some_ffi_call(ptr: *mut i32);
  }
  
  // Triggers clippy::not_unsafe_ptr_arg_deref:
  // - public function
  // - not marked `unsafe`
  // - raw pointer parameter `ptr` is passed into an `unsafe` block
  pub fn example(ptr: *mut i32) {
      unsafe { some_ffi_call(ptr) };
  }
  

  In Patina, this pattern is common for functions that take efi::Event or
  efi::Handle parameters and call Boot Services function pointers. We
  explicitly suppress this lint for such functions because these types are
  treated as opaque pointers and are never dereferenced:
  #[allow(clippy::not_unsafe_ptr_arg_deref)]

Geiger Unsafe Stats

|             | x86_64-unknown-uefi(before) | x86_64-unknown-uefi(after) | aarch64-unknown-uefi(before) | aarch64-unknown-uefi(after) |
|-------------|-----------------------------|----------------------------|------------------------------|-----------------------------|
| overall     | 11.90% (yellow)             | 16.30% (red)               | 12.20% (yellow)              | 16.30% (red)                |
| functions   | 2.60%  (green)              | 11.40% (yellow)            | 2.80%  (green)               | 11.40% (yellow)             |
| exprs       | 12.40% (yellow)             | 17.00% (red)               | 12.60% (yellow)              | 17.00% (red)                |
| item_impls  | 12.50% (yellow)             | 13.80% (yellow)            | 13.80% (yellow)              | 13.80% (yellow)             |
| item_traits | 12.10% (yellow)             | 13.00% (yellow)            | 13.00% (yellow)              | 13.00% (yellow)             |
| methods     | 5.60%  (green)              | 6.60%  (green)             | 5.60%  (green)               | 6.60%  (green)              |

How This Was Tested

Booted to UEFI Shell Q35/SBSA

Integration Instructions

All consumers of Patina will need to pick the newer Patina version when published and also should update their r-efi version to 6.0.0.

[patina-automation]

⌛ QEMU Validation Pending

QEMU validation is pending on successful CI completion.

Note: Any previous results are available in this comment's edit history.

This comment was automatically generated by the Patina QEMU PR Validation workflow.

[codecov]

Codecov Report

❌ Patch coverage is 87.83505% with 59 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
patina_dxe_core/src/protocols.rs	33.33%	32 Missing ⚠️
sdk/patina/src/boot_services.rs	94.93%	11 Missing ⚠️
patina_dxe_core/src/driver_services.rs	85.42%	7 Missing ⚠️
patina_dxe_core/src/pi_dispatcher/image.rs	88.00%	6 Missing ⚠️
patina_dxe_core/src/allocator.rs	91.18%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[makubacki]

I haven't looked at the PR changes yet, but it should target the major branch due to the severity of breaking changes.

[vineelko]

I haven't looked at the PR changes yet, but it should target the major branch due to the severity of breaking changes.

Michael, The branch is now updated to target major instead of main.

[makubacki]

I notice that you changed core_disconnect_controller() from unsafe to safe, and I understand the reasoning, given the platform/environment uncertainties during disconnect versus conditions controlled by the caller for connect. While the nuance is fine, at the function level, the asymmetry is more apparent by the fact that core_connect_controller() remains unsafe.

Can you please add a brief clarifying comment about this in the core_disconnect_controller() doc comment?

[vineelko]

This one feel bit odd to me, because marking the core_connect_controller() as unsafe for its undefined behavior not caused by the current inputs(but purely based on spec assumptions). This cannot be guaranteed by the caller. So, even though we have a safety comment explaining the scenario it feels like that function is marked unsafe incorrectly. In any case, we should have marked both unsafe or both safe. For now, I am marking the core_disconnect_controller() also as unsafe as similar undefined behavior can happen in disconnect call. I am open to change both of them otherwise.

[makubacki]

Yeah, my comment here is specifically about clearly explaining the decision made regardless of the decision itself.

Regarding the decision, my take is:

Handles are disjoint from the type system but should be able to be safely verified by the protocol database implementation. Since core_disconnect_controller() exclusively takes three handles, I thought it was reasonable to be safe. Implementation-aware safety invariants can be considered separately from UEFI-specification environmental dangers.

Because core_connect_controller() takes handles + a raw pointer (optional, to a protocol), I thought it was reasonable to be unsafe. The pointer can be checked for being non-null simple enough, but there is a lifetime aspect to its protocol interface buffer being valid throughout the execution of core_connect_controller() that is, at least partially, the caller's responsibility and cannot be verified otherwise than the safety contract.

[vineelko]

Updated the function comments to explain why they are being marked unsafe. The lifetime aspect of the protocol interface also seems to apply for the core_disconnect_controller() because it internally calls DriverBinding->Stop(). Hence keeping both of them as unsafe(Even though the inputs are exclusively handles which can be validated against the ProtocolDB).