test: fix null pointer issue when DevicePath is null + integration test

[scholzp]

About this test

This test tries to open the LoadedImageDevicePath on an image with Null as DevicePath. The handle to the image containing the Null pointer is created by executing the EFI_BOOT_SERVICES.LoadImage() to load an image from buffer. If no DevicePath is given, the DevicePath of the loaded image will be Null. This case is correctly implemented in LoadImageSource. The reverse way of using EFI protocols on an image created in this way should (in my understandig) work too. An example of a protocol which could be used on such an image is EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL. The EFI specification (section 9.2.1, p. 250) states for this case:

It is legal to call LoadImage() for a buffer in memory with a NULL DevicePath parameter. In this case, the Loaded Image Device Path Protocol is installed with a NULL interface pointer.

Therefore it shouldn't be a problem working with and executing protocols on such an image handle.

The Problem with this test

In the current state this test will fail. This seems to be caused by DevicePathNode::from_ffi_ptr, which dereferences the Null pointer returned by EFI, which results in undefinined behavior. The documentation of form_ffi_ptr states:

The input pointer must point to valid data.

I think this assumption doesn't hold in this case, as the user of the crate can not do anything against this behavior when working with a respective image (handle) returned from the UEFI implementation.

Suggested solution

I think the way to go would be to implement a Null pointer check in DevicePathNode::from_ffi_ptr and to make it return an Option rather then the raw pointer to Self. As it turned out, this change would also result in more invasive changes done to other parts of the crate. A few would be:

• Change ProtocolPointer trait to return an Option
• Implement Null pointer checks for all implementation of ProtocolPointer trait
• OpenProtocl might need changes
• Maybe even ScopedProtocol trait

There might be even more spots affected by these changes, which I didn't see yet. I tried to implement the suggested changes of the first two points but found the other problems on my way. As I'm not that experienced with Rust and UEFI, I thought at this point, it would be the best to reach out to you. I already spoke to @phip1611 about this and we both think, that a few of these changes do need some proper considerations to be implemented correctly. I therefore opened this PR for tracking and discussing changes to be done and would happily implement them under your guidance.

I look forward to hear about you opinions!

Checklist

• Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
• Update the changelog (if necessary)

[phip1611]

You're right, this is a bug in the uefi crate. I verified the output of the open_protocol function and it says:

status=SUCCESS, ptr=0x0000000000000000

for the case you described above.

So having a null-pointer returned for the protocol is valid in UEFI. The question is: Is this just the case for device paths or can other protocols also be null when status states SUCCESS? I think it would be a very invasive change to add an option for all protocols when just device paths can be null.

---

Maybe having a ScopedProtocol<Option<P: Protocol>> is the right solution. The documentation could say that only for device paths this value can be None.

Can you please add the following patch to your branch in a dedicated commit? This might be obsolete if we decide to use Option<> at this level.. but I want to make sure that we do not forget this.

diff --git a/uefi/src/proto/device_path/mod.rs b/uefi/src/proto/device_path/mod.rs
index 9ec1097..0c80ef6 100644
--- a/uefi/src/proto/device_path/mod.rs
+++ b/uefi/src/proto/device_path/mod.rs
@@ -296,10 +296,12 @@ pub struct DevicePath {
 
 impl ProtocolPointer for DevicePath {
     unsafe fn ptr_from_ffi(ptr: *const c_void) -> *const Self {
+        assert!(!ptr.is_null());
         ptr_meta::from_raw_parts(ptr.cast(), Self::size_in_bytes_from_ptr(ptr))
     }
 
     unsafe fn mut_ptr_from_ffi(ptr: *mut c_void) -> *mut Self {
+        assert!(!ptr.is_null());
         ptr_meta::from_raw_parts_mut(ptr.cast(), Self::size_in_bytes_from_ptr(ptr))
     }
 }
@@ -684,10 +686,12 @@ pub struct LoadedImageDevicePath(DevicePath);
 
 impl ProtocolPointer for LoadedImageDevicePath {
     unsafe fn ptr_from_ffi(ptr: *const c_void) -> *const Self {
+        assert!(!ptr.is_null());
         ptr_meta::from_raw_parts(ptr.cast(), DevicePath::size_in_bytes_from_ptr(ptr))
     }
 
     unsafe fn mut_ptr_from_ffi(ptr: *mut c_void) -> *mut Self {
+        assert!(!ptr.is_null());
         ptr_meta::from_raw_parts_mut(ptr.cast(), DevicePath::size_in_bytes_from_ptr(ptr))
     }
 }
diff --git a/uefi/src/proto/mod.rs b/uefi/src/proto/mod.rs
index 71d7530..fd0a01b 100644
--- a/uefi/src/proto/mod.rs
+++ b/uefi/src/proto/mod.rs
@@ -56,10 +56,12 @@ where
     P: Protocol,
 {
     unsafe fn ptr_from_ffi(ptr: *const c_void) -> *const Self {
+        assert!(!ptr.is_null());
         ptr.cast::<Self>()
     }
 
     unsafe fn mut_ptr_from_ffi(ptr: *mut c_void) -> *mut Self {
+        assert!(!ptr.is_null());
         ptr.cast::<Self>()
     }
 }

[phip1611]

I don't know how nice or dirty this solution is, but this could be a possible fix

diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs
index e9ff3e5..4b00c62 100644
--- a/uefi/src/table/boot.rs
+++ b/uefi/src/table/boot.rs
@@ -1322,15 +1322,21 @@ impl BootServices {
         attributes: OpenProtocolAttributes,
     ) -> Result<ScopedProtocol<P>> {
         let mut interface = ptr::null_mut();
-        (self.open_protocol)(
+        let mut status = (self.open_protocol)(
             params.handle,
             &P::GUID,
             &mut interface,
             params.agent,
             params.controller,
             attributes as u32,
-        )
-        .to_result_with_val(|| {
+        );
+
+        if status == Status::SUCCESS && interface.is_null() {
+            log::debug!("Status is success but interface is null. Probably, a device path is not existing. Mapping to Status::NOT_FOUND");
+            status = Status::NOT_FOUND
+        }
+
+        status.to_result_with_val(|| {
             let interface = P::mut_ptr_from_ffi(interface) as *const UnsafeCell<P>;
 
             ScopedProtocol {

[phip1611]

Functions such as https://uefi.org/specs/UEFI/2.10/10_Protocols_Device_Path_Protocol.html#efi-device-path-utilities-protocol-getdevicepathsize are specified with device-paths being zero in mind. The uefi crate should respect that.

[phip1611]

We might benefit from the following property:

assert_eq!(size_of::<Option<&u8>>(), size_of::<&u8>());

Both variants have the same layout in memory. So we can use more rust convenience
while also being FFI safe.

---

Obsolete

I tried another possible way to address the issue. With this patch

diff --git a/uefi-test-runner/src/boot/mod.rs b/uefi-test-runner/src/boot/mod.rs
index e8f55fe..e2cf214 100644
--- a/uefi-test-runner/src/boot/mod.rs
+++ b/uefi-test-runner/src/boot/mod.rs
@@ -2,7 +2,7 @@ use alloc::string::ToString;
 use uefi::proto::console::text::Output;
 use uefi::proto::device_path::media::FilePath;
 use uefi::proto::device_path::{DevicePath, LoadedImageDevicePath};
-use uefi::table::boot::{BootServices, LoadImageSource, SearchType};
+use uefi::table::boot::{BootServices, LoadImageSource, OpenProtocolAttributes, OpenProtocolParams, SearchType};
 use uefi::table::{Boot, SystemTable};
 use uefi::{CString16, Identify};
 
@@ -85,11 +85,24 @@ fn test_load_image(bt: &BootServices) {
             buffer: image_data.as_slice(),
             file_path: None,
         };
-        let _ = bt
+        let loaded_image = bt
             .load_image(bt.image_handle(), load_source)
             .expect("should load image");
 
         log::debug!("load_image with FromBuffer strategy works");
+
+        unsafe {
+            let _ = bt
+                .open_protocol::<LoadedImageDevicePath>(
+                    OpenProtocolParams {
+                        handle: loaded_image,
+                        agent: bt.image_handle(),
+                        controller: None,
+                    },
+                    OpenProtocolAttributes::GetProtocol,
+                )
+                .expect("should open LoadedImageDevicePath protocol");
+        }
     }
     // Variant B: FromDevicePath
     {
diff --git a/uefi/src/proto/device_path/mod.rs b/uefi/src/proto/device_path/mod.rs
index 9ec1097..7fc18c3 100644
--- a/uefi/src/proto/device_path/mod.rs
+++ b/uefi/src/proto/device_path/mod.rs
@@ -309,6 +309,10 @@ impl DevicePath {
     /// at `ptr`. This adds up each node's length, including the
     /// end-entire node.
     unsafe fn size_in_bytes_from_ptr(ptr: *const c_void) -> usize {
+        if ptr.is_null() {
+            return 0;
+        }
+
         let mut ptr = ptr.cast::<u8>();
         let mut total_size_in_bytes: usize = 0;
         loop {

I get the following error:

[PANIC]: panicked at 'should open LoadedImageDevicePath protocol: Error { status: SUCCESS, data: () }', uefi-test-runner/src/boot/mod.rs:104:18

The problem here is that we dereference a null pointer which causes undefined behaviour. Because of that, we see the strange output that we have an error with status success.

[phip1611]

I tested another possible solution that solves the problem (partially) at least for the low-level API. With that, we never dereference a null pointer but at least do get an explicit panic once we use the deref() which calls unwrap on a None value.

diff --git a/uefi-test-runner/src/boot/mod.rs b/uefi-test-runner/src/boot/mod.rs
index e8f55fe..0445da1 100644
--- a/uefi-test-runner/src/boot/mod.rs
+++ b/uefi-test-runner/src/boot/mod.rs
@@ -2,7 +2,7 @@ use alloc::string::ToString;
 use uefi::proto::console::text::Output;
 use uefi::proto::device_path::media::FilePath;
 use uefi::proto::device_path::{DevicePath, LoadedImageDevicePath};
-use uefi::table::boot::{BootServices, LoadImageSource, SearchType};
+use uefi::table::boot::{BootServices, LoadImageSource, OpenProtocolAttributes, OpenProtocolParams, SearchType};
 use uefi::table::{Boot, SystemTable};
 use uefi::{CString16, Identify};
 
@@ -85,11 +85,24 @@ fn test_load_image(bt: &BootServices) {
             buffer: image_data.as_slice(),
             file_path: None,
         };
-        let _ = bt
+        let loaded_image = bt
             .load_image(bt.image_handle(), load_source)
             .expect("should load image");
 
         log::debug!("load_image with FromBuffer strategy works");
+
+        unsafe {
+            let x = bt
+                .open_protocol::<LoadedImageDevicePath>(
+                    OpenProtocolParams {
+                        handle: loaded_image,
+                        agent: bt.image_handle(),
+                        controller: None,
+                    },
+                    OpenProtocolAttributes::GetProtocol,
+                )
+                .expect("should open LoadedImageDevicePath protocol");
+        }
     }
     // Variant B: FromDevicePath
     {
diff --git a/uefi/src/proto/device_path/mod.rs b/uefi/src/proto/device_path/mod.rs
index 9ec1097..7fc18c3 100644
--- a/uefi/src/proto/device_path/mod.rs
+++ b/uefi/src/proto/device_path/mod.rs
@@ -309,6 +309,10 @@ impl DevicePath {
     /// at `ptr`. This adds up each node's length, including the
     /// end-entire node.
     unsafe fn size_in_bytes_from_ptr(ptr: *const c_void) -> usize {
+        if ptr.is_null() {
+            return 0;
+        }
+
         let mut ptr = ptr.cast::<u8>();
         let mut total_size_in_bytes: usize = 0;
         loop {
diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs
index 2669f4e..8df86c1 100644
--- a/uefi/src/table/boot.rs
+++ b/uefi/src/table/boot.rs
@@ -230,9 +230,14 @@ pub struct BootServices {
     ) -> Status,
 
     // Protocol open / close services
+    //
     open_protocol: unsafe extern "efiapi" fn(
         handle: Handle,
         protocol: &Guid,
+        // Might returns null if no structure is associated with the given
+        // protocol, even if the status code indicates SUCCESS. For example,
+        // an image may not have a device path, if it was loaded from a buffer
+        // into memory.
         interface: &mut *mut c_void,
         agent_handle: Handle,
         controller_handle: Option<Handle>,
@@ -1341,7 +1346,7 @@ impl BootServices {
             let interface = P::mut_ptr_from_ffi(interface) as *const UnsafeCell<P>;
 
             ScopedProtocol {
-                interface: &*interface,
+                interface: interface.as_ref(),
                 open_params: params,
                 boot_services: self,
             }
@@ -1819,7 +1824,8 @@ pub struct OpenProtocolParams {
 #[derive(Debug)]
 pub struct ScopedProtocol<'a, P: Protocol + ?Sized> {
     /// The protocol interface.
-    interface: &'a UnsafeCell<P>,
+    // Memory: Same Layout as size_of::<&P>()
+    interface: Option<&'a UnsafeCell<P>>,
 
     open_params: OpenProtocolParams,
     boot_services: &'a BootServices,
@@ -1848,13 +1854,13 @@ impl<'a, P: Protocol + ?Sized> Deref for ScopedProtocol<'a, P> {
     type Target = P;
 
     fn deref(&self) -> &Self::Target {
-        unsafe { &*self.interface.get() }
+        unsafe { &*self.interface.unwrap().get() }
     }
 }
 
 impl<'a, P: Protocol + ?Sized> DerefMut for ScopedProtocol<'a, P> {
     fn deref_mut(&mut self) -> &mut Self::Target {
-        unsafe { &mut *self.interface.get() }
+        unsafe { &mut *self.interface.unwrap().get() }
     }
 }

[nicholasbishop]

I'm still digesting the above conversation so I don't have any real thoughts yet, but just wanted to say thanks for the detailed bug report!

[nicholasbishop]

Merged a fix for this issue in #861. Trying to access a null interface will now panic instead of dereferencing a null pointer, and you can use ScopedProtocol::get/ScopedProtocol_get_mut to get an Option instead of panicking.

Thanks again for the report :)