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 Failed

QEMU validation did not complete successfully or did not shutdown as expected.

Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/26074838332

⚠️ Build Failure Detected

The Setup and Build patina-dxe-core-qemu step failed in the following job(s):

• Run Patina QEMU Validation / Validate QEMU - Q35 (Linux)
• Run Patina QEMU Validation / Validate QEMU Q35 (Windows)
• Run Patina QEMU Validation / Validate QEMU - SBSA (Linux)

Build logs are available in the workflow run artifacts:

• qemu-validation-logs-Windows-Q35
• qemu-validation-logs-Linux-SBSA
• qemu-validation-logs-Linux-Q35

Download the log artifacts from the workflow run to review the build output and
determine whether the error is expected.

Is this expected? If the PR introduces breaking changes to patina that
require a corresponding update to patina-dxe-core-qemu,
the build failure is expected.

Contact a repo admin or PR reviewer to override the Patina QEMU PR Validation status check.

Job Results

Job	Result
Gather Incoming PR Metadata	✅
Run Patina QEMU Validation / Preflight Checks	✅
Run Patina QEMU Validation / Post In-Progress Notification	✅
Run Patina QEMU Validation / Get Constants / Get Repository Constants	✅
Run Patina QEMU Validation / Validate QEMU - Q35 (Linux)	❌
Run Patina QEMU Validation / Validate QEMU Q35 (Windows)	❌
Run Patina QEMU Validation / Validate QEMU - SBSA (Linux)	❌
Run Patina QEMU Validation / Emit PR Metadata	✅

Error Details

qemu-validation-logs-Linux-Q35/q35-linux.log (15 error/warning sections)

… (truncated)


error[E0308]: mismatched types
   --> src/q35/component/service/smbios_platform.rs:125:30
    |
125 |             security_status: 0x02,
    |                              ^^^^ expected `SecurityStatus`, found integer


error[E0308]: mismatched types
   --> src/q35/component/service/smbios_platform.rs:155:28
    |
155 |             feature_flags: 0x01, // Board is a hosting board
    |                            ^^^^ expected `FeatureFlags`, found integer
    |
help: call `Into::into` on this expression to convert `{integer}` into `FeatureFlags`
    |
155 |             feature_flags: 0x01.into(), // Board is a hosting board
    |                                +++++++


error[E0308]: mismatched types
   --> src/q35/component/service/smbios_platform.rs:158:25
    |
158 |             board_type: 0x0A, // Motherboard
    |                         ^^^^ expected `BoardType`, found integer


error[E0308]: mismatched types
   --> src/q35/component/service/smbios_test.rs:45:49
    |
 45 |         boot_services.locate_protocol_unchecked(&SMBIOS_PROTOCOL_GUID, core::ptr::null_mut()).map_err(|e| {
    |                       ------------------------- ^^^^^^^^^^^^^^^^^^^^^ expected `r_efi::base::Guid`, found a different `r_efi::base::Guid`
    |                       |
    |                       arguments to this method are incorrect
    |
note: there are multiple different versions of crate `r_efi` in the dependency graph
   --> /.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/r-efi-6.0.0/src/base.rs:392:1
    |
392 | pub struct Guid {
    | ^^^^^^^^^^^^^^^ this is the expected type
    |
   ::: /.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/r-efi-5.3.0/src/base.rs:392:1
    |
392 | pub struct Guid {
    | --------------- this is the found type
    = help: you can use `cargo tree` to explore your dependency tree
note: method defined here
   --> /__w/patina/patina/sdk/patina/src/boot_services.rs:826:15
    |
826 |     unsafe fn locate_protocol_unchecked(
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0308`.

error: could not compile `qemu_dxe_core` (lib) due to 12 previous errors

warning: build failed, waiting for other jobs to finish...

[cargo-make] ERROR - Error while running duckscript: Source: Unknown Line: 113 - Error while executing command, exit code: 101

qemu-validation-logs-Windows-Q35/q35-windows.log (15 error/warning sections)

… (truncated)


error[E0308]: mismatched types
   --> src\q35\component\service\smbios_platform.rs:125:30
    |
125 |             security_status: 0x02,
    |                              ^^^^ expected `SecurityStatus`, found integer


error[E0308]: mismatched types
   --> src\q35\component\service\smbios_platform.rs:155:28
    |
155 |             feature_flags: 0x01, // Board is a hosting board
    |                            ^^^^ expected `FeatureFlags`, found integer
    |
help: call `Into::into` on this expression to convert `{integer}` into `FeatureFlags`
    |
155 |             feature_flags: 0x01.into(), // Board is a hosting board
    |                                +++++++


error[E0308]: mismatched types
   --> src\q35\component\service\smbios_platform.rs:158:25
    |
158 |             board_type: 0x0A, // Motherboard
    |                         ^^^^ expected `BoardType`, found integer


error[E0308]: mismatched types
   --> src\q35\component\service\smbios_test.rs:45:49
    |
 45 |         boot_services.locate_protocol_unchecked(&SMBIOS_PROTOCOL_GUID, core::ptr::null_mut()).map_err(|e| {
    |                       ------------------------- ^^^^^^^^^^^^^^^^^^^^^ expected `r_efi::base::Guid`, found a different `r_efi::base::Guid`
    |                       |
    |                       arguments to this method are incorrect
    |
note: there are multiple different versions of crate `r_efi` in the dependency graph
   --> C:\Users\runneradmin\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\r-efi-6.0.0\src\base.rs:392:1
    |
392 | pub struct Guid {
    | ^^^^^^^^^^^^^^^ this is the expected type
    |
   ::: C:\Users\runneradmin\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\r-efi-5.3.0\src\base.rs:392:1
    |
392 | pub struct Guid {
    | --------------- this is the found type
    = help: you can use `cargo tree` to explore your dependency tree
note: method defined here
   --> D:\a\patina\patina\sdk\patina\src\boot_services.rs:826:15
    |
826 |     unsafe fn locate_protocol_unchecked(
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0308`.

error: could not compile `qemu_dxe_core` (lib) due to 12 previous errors

warning: build failed, waiting for other jobs to finish...

[cargo-make] ERROR - Error while running duckscript: Source: Unknown Line: 113 - Error while executing command, exit code: 101

qemu-validation-logs-Linux-SBSA/sbsa-linux.log (15 error/warning sections)

… (truncated)


error[E0308]: mismatched types
   --> src/q35/component/service/smbios_platform.rs:125:30
    |
125 |             security_status: 0x02,
    |                              ^^^^ expected `SecurityStatus`, found integer


error[E0308]: mismatched types
   --> src/q35/component/service/smbios_platform.rs:155:28
    |
155 |             feature_flags: 0x01, // Board is a hosting board
    |                            ^^^^ expected `FeatureFlags`, found integer
    |
help: call `Into::into` on this expression to convert `{integer}` into `FeatureFlags`
    |
155 |             feature_flags: 0x01.into(), // Board is a hosting board
    |                                +++++++


error[E0308]: mismatched types
   --> src/q35/component/service/smbios_platform.rs:158:25
    |
158 |             board_type: 0x0A, // Motherboard
    |                         ^^^^ expected `BoardType`, found integer


error[E0308]: mismatched types
   --> src/q35/component/service/smbios_test.rs:45:49
    |
 45 |         boot_services.locate_protocol_unchecked(&SMBIOS_PROTOCOL_GUID, core::ptr::null_mut()).map_err(|e| {
    |                       ------------------------- ^^^^^^^^^^^^^^^^^^^^^ expected `r_efi::base::Guid`, found a different `r_efi::base::Guid`
    |                       |
    |                       arguments to this method are incorrect
    |
note: there are multiple different versions of crate `r_efi` in the dependency graph
   --> /.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/r-efi-6.0.0/src/base.rs:392:1
    |
392 | pub struct Guid {
    | ^^^^^^^^^^^^^^^ this is the expected type
    |
   ::: /.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/r-efi-5.3.0/src/base.rs:392:1
    |
392 | pub struct Guid {
    | --------------- this is the found type
    = help: you can use `cargo tree` to explore your dependency tree
note: method defined here
   --> /__w/patina/patina/sdk/patina/src/boot_services.rs:826:15
    |
826 |     unsafe fn locate_protocol_unchecked(
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0308`.

error: could not compile `qemu_dxe_core` (lib) due to 12 previous errors

warning: build failed, waiting for other jobs to finish...

[cargo-make] ERROR - Error while running duckscript: Source: Unknown Line: 113 - Error while executing command, exit code: 101

Dependencies

Repository	Ref
patina	cc656fc
patina-dxe-core-qemu	e178e37
patina-fw-patcher	d656f1d
patina-qemu firmware	v3.0.0
patina-qemu build script	e0231b7

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

[codecov]

Codecov Report

❌ Patch coverage is 89.60177% with 47 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
patina_dxe_core/src/protocols.rs	28.12%	23 Missing ⚠️
sdk/patina/src/boot_services.rs	94.91%	11 Missing ⚠️
patina_dxe_core/src/driver_services.rs	86.84%	5 Missing ⚠️
patina_dxe_core/src/allocator.rs	91.18%	3 Missing ⚠️
patina_dxe_core/src/pi_dispatcher.rs	40.00%	3 Missing ⚠️
patina_dxe_core/src/pi_dispatcher/image.rs	96.36%	2 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.

[joschock]

there is a lot of "unsafe poisoning" downstream of these raw pointers to device_path::Protocol. Could we instead make a newtype of these with an unsafe constructor so that the safety invariant is not spread everywhere?

struct DevicePathPtr(*mut efi::protocols::device_path::Protocol);
impl DevicePathPtr {
   /// Safety: Caller must guarantee that device path pointer is non-null and points to a valid device path.
   pub unsafe fn new(ptr: *mut efi::Protocols::device_path::Protocol) -> Self {
      Self(ptr)
   }
   pub fn as_raw(&mut self) -> *mut T {
     self.0
   }
}

pub load_image(
  &self,
  boot_policy: bool,
  parent_image_handle: efi::Handle, 
  file_path: DevicePathPtr
) -> Result<etc> {
...
}

// Safety: my_raw_path is a properly constructed valid device path with static lifetime (or whatever)
let file_path = unsafe { DevicePathPtr::new(my_raw_path); }
PI_DISPATCHER.load_image(boot_policy, parent_image_handle, file_path); // safe

[vineelko]

I am prototyping safe DevicePathPtr around *mut efi::protocols::device_path::Protocol. This newtype looks very promising and resembles much like NonNull<T> type. And it feels like it is not limited to DevicePath and can be applicable to other raw pointer types which are used in Patina.

pub struct RawPtr<T>(*mut T);

impl<T> RawPtr<T> {
    pub unsafe fn new(ptr: *mut T) -> Self {
        Self(ptr)
    }

    pub unsafe fn from_raw(ptr: *mut T) -> Option<Self> {
        if ptr.is_null() {
            None
        } else {
            Some(unsafe { Self::new(ptr) })
        }
    }

    pub fn as_ptr(self) -> *mut T {
        self.0
    }
}

My only concern is that, the change will have far more impact beyond the scope of this PR. Especially, when patina\sdk\patina\src\device_path\walker.rs is also rewritten based on DevicePathPtr. So, I think it is better to pull in that change immediately after this PR. I can create a work item for tracking and can work on it.

[joschock]

I'm not sure how I feel about a universal RawPtr safe wrapper. That seems a bit generic? But I guess it would be unsafe at both point of creation and point of deref; so I guess it doesn't do too much to mask unsafety.

[joschock]

I'm not sure this needs to be unsafe. If the image_handle is not valid, you can just return an error. We can add additional validation (e.g. make sure that the handle is not only valid, but that it is actually associated with an image). The caller can't really uphold the invariant of whether the loaded_images memory and entry point are valid; those are upheld by load_image implementation.

[vineelko]

    /// Starts execution of a previously loaded image.
    ///
    /// The `image_handle` is validated against the protocol database and the
    /// private image data map; an error is returned if the handle is unknown or
    /// the image has already been started. However, a valid handle must still
    /// refer to a properly loaded image whose entry point is executable code.
    /// It is the caller's responsibility to first load such an image via
    /// [`Self::load_image`] before calling this function.
    ///
    /// # Safety
    ///
    /// The caller must ensure that `image_handle` was obtained from a prior
    /// successful call to [`Self::load_image`] and that the loaded image's
    /// memory and entry point remain valid.

My thought process was about the case where a malformed PE image has a valid entry point but contains invalid code (or something similar), which we cannot guarantee against. LoadImage will load the image because it satisfies the PE format requirements, but StartImage can still fail during execution.

[joschock]

I think concerns about a malformed image buffer should be on load_image. I think the promise chain goes: "This is a good image" -> load_image -> image_handle -> start_image. So start_image is basically relying on the fact that it is an image_handle to signify a promise that it was properly constructed. Practically speaking, if the image buffer is junk, you won't change the arguments to start_image, you'll change the arguments to load_image.

[joschock]

The safety here is pretty much something like: "caller must uphold the UEFI spec contract with respect to the semantics of this API"

[joschock]

Since this is a UEFI spec function, I don't think the caller needs to be aware of or uphold rust's aliasing rules, unless it is a rust caller (in which case it should just use core::ptr::copy() directly).

[joschock]

I'm not sure the caller can directly uphold the "no weird edge cases exist in the platform where the driver is uninstalled between Supported() and Start()".

Other safety conditions hold tho.

[vineelko]

Agreed, I don't mean to make it as a caller's responsibility 😄. But I really want to document that edge case in the function comment so that the caller is aware of it. I will move it above the SAFETY comments.