Merge pull request #973 from nicholasbishop/bishop-unsafe-free

Mark free_pages and free_pool as unsafe

CHANGELOG.md

@@ -8,6 +8,8 @@
 ### Changed
 - We fixed a memory leak in `GraphicsOutput::query_mode`. As a consequence, we
   had to add `&BootServices` as additional parameter.
+- `BootServices::free_pages` and `BootServices::free_pool` are now `unsafe` to
+  call, since it is possible to trigger UB by freeing memory that is still in use.
 
 ## uefi-macros - [Unreleased]
 

uefi-test-runner/src/boot/memory.rs

@@ -31,7 +31,7 @@ fn allocate_pages(bt: &BootServices) {
     buf[4095] = 0x23;
 
     // Clean up to avoid memory leaks.
-    bt.free_pages(pgs, 1).unwrap();
+    unsafe { bt.free_pages(pgs, 1) }.unwrap();
 }
 
 // Simple test to ensure our custom allocator works with the `alloc` crate.

uefi/src/proto/console/gop.rs

@@ -90,8 +90,7 @@ impl GraphicsOutput {
 
                 // User has no benefit from propagating this error. If this
                 // fails, it is an error of the UEFI implementation.
-                bs.free_pool(info_heap_ptr)
-                    .expect("buffer should be deallocatable");
+                unsafe { bs.free_pool(info_heap_ptr) }.expect("buffer should be deallocatable");
 
                 Mode {
                     index,

uefi/src/proto/device_path/text.rs

@@ -70,9 +70,7 @@ impl<'a> Deref for PoolString<'a> {
 impl Drop for PoolString<'_> {
     fn drop(&mut self) {
         let addr = self.text as *mut u8;
-        self.boot_services
-            .free_pool(addr)
-            .expect("Failed to free pool [{addr:#?}]");
+        unsafe { self.boot_services.free_pool(addr) }.expect("Failed to free pool [{addr:#?}]");
     }
 }
 

uefi/src/table/boot.rs

@@ -184,13 +184,18 @@ impl BootServices {
 
     /// Frees memory pages allocated by UEFI.
     ///
+    /// # Safety
+    ///
+    /// The caller must ensure that no references into the allocation remain,
+    /// and that the memory at the allocation is not used after it is freed.
+    ///
     /// # Errors
     ///
     /// See section `EFI_BOOT_SERVICES.FreePages()` in the UEFI Specification for more details.
     ///
     /// * [`uefi::Status::NOT_FOUND`]
     /// * [`uefi::Status::INVALID_PARAMETER`]
-    pub fn free_pages(&self, addr: PhysicalAddress, count: usize) -> Result {
+    pub unsafe fn free_pages(&self, addr: PhysicalAddress, count: usize) -> Result {
         unsafe { (self.0.free_pages)(addr, count) }.to_result()
     }
 
@@ -293,13 +298,17 @@ impl BootServices {
 
     /// Frees memory allocated from a pool.
     ///
+    /// # Safety
+    ///
+    /// The caller must ensure that no references into the allocation remain,
+    /// and that the memory at the allocation is not used after it is freed.
+    ///
     /// # Errors
     ///
     /// See section `EFI_BOOT_SERVICES.FreePool()` in the UEFI Specification for more details.
     ///
     /// * [`uefi::Status::INVALID_PARAMETER`]
-    #[allow(clippy::not_unsafe_ptr_arg_deref)]
-    pub fn free_pool(&self, addr: *mut u8) -> Result {
+    pub unsafe fn free_pool(&self, addr: *mut u8) -> Result {
         unsafe { (self.0.free_pool)(addr) }.to_result()
     }
 
@@ -1867,7 +1876,7 @@ pub struct ProtocolsPerHandle<'a> {
 impl<'a> Drop for ProtocolsPerHandle<'a> {
     fn drop(&mut self) {
         // Ignore the result, we can't do anything about an error here.
-        let _ = self.boot_services.free_pool(self.protocols.cast::<u8>());
+        let _ = unsafe { self.boot_services.free_pool(self.protocols.cast::<u8>()) };
     }
 }
 
@@ -1906,7 +1915,7 @@ pub struct HandleBuffer<'a> {
 impl<'a> Drop for HandleBuffer<'a> {
     fn drop(&mut self) {
         // Ignore the result, we can't do anything about an error here.
-        let _ = self.boot_services.free_pool(self.buffer.cast::<u8>());
+        let _ = unsafe { self.boot_services.free_pool(self.buffer.cast::<u8>()) };
     }
 }