diff --git a/Cargo.toml b/Cargo.toml index 10177a92f..752a1f2b6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,7 +29,7 @@ linked_list_allocator = { version = "^0.10" } linkme = { version = "^0.3.29" } log = { version = "0.4", default-features = false } memoffset = {version = "0.9.1" } -mu_rust_helpers = { version = "3.0.2" } +mu_rust_helpers = { version = "4.0.0" } num-traits = { version = "0.2", default-features = false } patina = { version = "21.0.2", path = "sdk/patina" } patina_debugger = { version = "21.0.2", path = "core/patina_debugger" } @@ -49,7 +49,7 @@ patina_stacktrace = { version = "21.0.2", path = "core/patina_stacktrace" } patina_test = { version = "21.0.2", path = "components/patina_test" } proc-macro2 = { version = "1" } quote = { version = "1" } -r-efi = { version = "5.0.0", default-features = false } +r-efi = { version = "6.0.0", default-features = false } scroll = { version = "0.13", default-features = false, features = ["derive"]} spin = { version = "^0.9" } syn = { version = "2" } diff --git a/components/patina_acpi/src/acpi_table.rs b/components/patina_acpi/src/acpi_table.rs index c55dbdb61..2440ac909 100644 --- a/components/patina_acpi/src/acpi_table.rs +++ b/components/patina_acpi/src/acpi_table.rs @@ -498,11 +498,14 @@ impl AcpiTable { }; // Allocate memory in appropriate ACPI region, up to page granularity. + // SAFETY: allocation_strategy is either PageAllocationStrategy::MaxAddress(SIZE_4GB - 1) for + // FACS tables (which must reside in the lower 32-bit address space) or + // PageAllocationStrategy::Any for all others. Neither requires dereferencing a + // caller-provided address. + let alloc_options = + unsafe { AllocationOptions::new().with_memory_type(allocator_type).with_strategy(allocation_strategy) }; let table_page_alloc = mm - .allocate_pages( - uefi_size_to_pages!(table_length), - AllocationOptions::new().with_memory_type(allocator_type).with_strategy(allocation_strategy), - ) + .allocate_pages(uefi_size_to_pages!(table_length), alloc_options) .map_err(|_e| AcpiError::AllocationFailed)?; // Get the raw pointer to the allocated memory for copying. diff --git a/components/patina_test/src/component.rs b/components/patina_test/src/component.rs index e3bef7d76..cdfbc5663 100644 --- a/components/patina_test/src/component.rs +++ b/components/patina_test/src/component.rs @@ -310,7 +310,7 @@ pub(crate) mod tests { extern "efiapi" fn noop_create_event( _type: u32, _tpl: r_efi::efi::Tpl, - _notify_function: Option, + _notify_function: Option, _notify_context: *mut core::ffi::c_void, _event: *mut r_efi::efi::Event, ) -> r_efi::efi::Status { @@ -320,7 +320,7 @@ pub(crate) mod tests { extern "efiapi" fn noop_create_event_ex( _type: u32, _tpl: r_efi::efi::Tpl, - _notify_function: Option, + _notify_function: Option, _notify_context: *const core::ffi::c_void, _guid: *const r_efi::efi::Guid, _event: *mut r_efi::efi::Event, @@ -351,7 +351,7 @@ pub(crate) mod tests { extern "efiapi" fn noop_create_event( _type: u32, _tpl: r_efi::efi::Tpl, - _notify_function: Option, + _notify_function: Option, _notify_context: *mut core::ffi::c_void, _event: *mut r_efi::efi::Event, ) -> r_efi::efi::Status { @@ -361,7 +361,7 @@ pub(crate) mod tests { extern "efiapi" fn noop_create_event_ex( _type: u32, _tpl: r_efi::efi::Tpl, - _notify_function: Option, + _notify_function: Option, _notify_context: *const core::ffi::c_void, _guid: *const r_efi::efi::Guid, _event: *mut r_efi::efi::Event, diff --git a/patina_dxe_core/src/allocator.rs b/patina_dxe_core/src/allocator.rs index 5e24f1e9f..f460dfe09 100644 --- a/patina_dxe_core/src/allocator.rs +++ b/patina_dxe_core/src/allocator.rs @@ -591,7 +591,17 @@ fn alloc_error_handler(layout: alloc::alloc::Layout) -> ! { panic!("allocation error: {:?}", layout) } -extern "efiapi" fn allocate_pool(pool_type: efi::MemoryType, size: usize, buffer: *mut *mut c_void) -> efi::Status { +/// Allocates pool memory of the specified type. +/// +/// # Safety +/// +/// `buffer` must be a pointer pointing to valid, writable memory for a `*mut c_void`. +/// The pointer is null-checked, but validity of the referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn allocate_pool( + pool_type: efi::MemoryType, + size: usize, + buffer: *mut *mut c_void, +) -> efi::Status { if buffer.is_null() { return efi::Status::INVALID_PARAMETER; } @@ -606,6 +616,10 @@ extern "efiapi" fn allocate_pool(pool_type: efi::MemoryType, size: usize, buffer } } +/// Allocates pool memory of the specified type. +/// +/// This function is safe because all input parameters are value types and any internal unsafe +/// operations are fully encapsulated with validated preconditions. pub fn core_allocate_pool(pool_type: efi::MemoryType, size: usize) -> Result<*mut c_void, EfiError> { // It is not valid to attempt to allocate these memory types if matches!(pool_type, efi::CONVENTIONAL_MEMORY | efi::PERSISTENT_MEMORY | efi::UNACCEPTED_MEMORY_TYPE) { @@ -623,14 +637,31 @@ pub fn core_allocate_pool(pool_type: efi::MemoryType, size: usize) -> Result<*mu } } -extern "efiapi" fn free_pool(buffer: *mut c_void) -> efi::Status { - match core_free_pool(buffer) { +/// Frees pool memory previously allocated with [`allocate_pool`]. +/// +/// # Safety +/// +/// `buffer` must be a pointer pointing to valid, previously allocated memory +/// from [`allocate_pool`]. This is needed to locate allocation signatures. The +/// current implementation does not track allocations, so the caller is +/// responsible for ensuring the pointer is valid. +unsafe extern "efiapi" fn free_pool(buffer: *mut c_void) -> efi::Status { + // SAFETY: The caller is responsible for ensuring `buffer` points to a valid allocation. + match unsafe { core_free_pool(buffer) } { Ok(_) => efi::Status::SUCCESS, Err(status) => status.into(), } } -pub fn core_free_pool(buffer: *mut c_void) -> Result<(), EfiError> { +/// Frees pool memory previously allocated with [`core_allocate_pool`]. +/// +/// # Safety +/// +/// `buffer` must be a pointer pointing to valid, previously allocated memory +/// from [`core_allocate_pool`]. This is needed to locate allocation signatures. +/// The current implementation does not track allocations, so the caller is +/// responsible for ensuring the pointer is valid. +pub unsafe fn core_free_pool(buffer: *mut c_void) -> Result<(), EfiError> { if buffer.is_null() { return Err(EfiError::InvalidParameter); } @@ -648,29 +679,43 @@ pub fn core_free_pool(buffer: *mut c_void) -> Result<(), EfiError> { } } -extern "efiapi" fn allocate_pages( +/// Allocates pages of the specified type and returns the address through the `memory` pointer. +/// +/// # Safety +/// +/// The caller is responsible for ensuring that `memory` points to valid writable memory. The +/// pointer is null-checked, but validity of the referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn allocate_pages( allocation_type: efi::AllocateType, memory_type: efi::MemoryType, pages: usize, memory: *mut efi::PhysicalAddress, ) -> efi::Status { - match core_allocate_pages(allocation_type, memory_type, pages, memory, None) { - Ok(_) => efi::Status::SUCCESS, + // SAFETY: The caller is responsible for ensuring `memory` is valid. + if memory.is_null() { + return efi::Status::INVALID_PARAMETER; + } + + // SAFETY: caller must ensure that "memory" is a valid pointer. It is null-checked above. + let address = unsafe { memory.read_unaligned() }; + match core_allocate_pages(allocation_type, memory_type, pages, address, None) { + Ok(address) => { + // SAFETY: caller must ensure that "memory" is a valid pointer. It is null-checked above. + unsafe { memory.write_unaligned(address) } + efi::Status::SUCCESS + } Err(status) => status.into(), } } +/// Allocates pages of the specified type and returns the address through the `memory` pointer. pub fn core_allocate_pages( allocation_type: efi::AllocateType, memory_type: efi::MemoryType, pages: usize, - memory: *mut efi::PhysicalAddress, + memory: efi::PhysicalAddress, alignment: Option, -) -> Result<(), EfiError> { - if memory.is_null() { - return Err(EfiError::InvalidParameter); - } - +) -> Result { // It is not valid to attempt to allocate these memory types if matches!(memory_type, efi::CONVENTIONAL_MEMORY | efi::PERSISTENT_MEMORY | efi::UNACCEPTED_MEMORY_TYPE) { return Err(EfiError::InvalidParameter); @@ -684,25 +729,15 @@ pub fn core_allocate_pages( let result = match allocation_type { efi::ALLOCATE_ANY_PAGES => allocator.allocate_pages(DEFAULT_ALLOCATION_STRATEGY, pages, alignment), efi::ALLOCATE_MAX_ADDRESS => { - // SAFETY: caller must ensure that "memory" is a valid pointer. It is null-checked above. - let address = unsafe { memory.read_unaligned() }; - allocator.allocate_pages(AllocationStrategy::TopDown(Some(address as usize)), pages, alignment) + allocator.allocate_pages(AllocationStrategy::TopDown(Some(memory as usize)), pages, alignment) } efi::ALLOCATE_ADDRESS => { - // SAFETY: caller must ensure that "memory" is a valid pointer. It is null-checked above. - let address = unsafe { memory.read_unaligned() }; - allocator.allocate_pages(AllocationStrategy::Address(address as usize), pages, alignment) + allocator.allocate_pages(AllocationStrategy::Address(memory as usize), pages, alignment) } _ => Err(EfiError::InvalidParameter), }; - if let Ok(ptr) = result { - // SAFETY: caller must ensure that "memory" is a valid pointer. It is null-checked above. - unsafe { memory.write_unaligned(ptr.expose_provenance().get() as u64) } - Ok(()) - } else { - result.map(|_| ()) - } + result.map(|ptr| ptr.expose_provenance().get() as efi::PhysicalAddress) } Err(err) => Err(err), }; @@ -731,14 +766,27 @@ pub fn memory_type_for_handle(handle: efi::Handle) -> Option { ALLOCATORS.lock().memory_type_for_handle(handle) } -extern "efiapi" fn free_pages(memory: efi::PhysicalAddress, pages: usize) -> efi::Status { - match core_free_pages(memory, pages) { +/// Frees pages previously allocated with [`allocate_pages`]. +/// +/// # Safety +/// +/// `memory` must be a valid physical address corresponding to a previously allocated page range. +/// The caller must have exclusive ownership of the memory being freed. +unsafe extern "efiapi" fn free_pages(memory: efi::PhysicalAddress, pages: usize) -> efi::Status { + // SAFETY: The caller is responsible for ensuring `memory` is a valid, previously allocated address. + match unsafe { core_free_pages(memory, pages) } { Ok(_) => efi::Status::SUCCESS, Err(status) => status.into(), } } -pub fn core_free_pages(memory: efi::PhysicalAddress, pages: usize) -> Result<(), EfiError> { +/// Frees pages previously allocated with [`core_allocate_pages`]. +/// +/// # Safety +/// +/// `memory` must be a valid physical address corresponding to a previously allocated page range. +/// The caller must have exclusive ownership of the memory being freed. +pub unsafe fn core_free_pages(memory: efi::PhysicalAddress, pages: usize) -> Result<(), EfiError> { let size = match pages.checked_mul(UEFI_PAGE_SIZE) { Some(size) => size, None => return Err(EfiError::InvalidParameter), @@ -790,12 +838,20 @@ pub fn core_free_pages(memory: efi::PhysicalAddress, pages: usize) -> Result<(), res } -extern "efiapi" fn copy_mem(destination: *mut c_void, source: *mut c_void, length: usize) { +/// # Safety +/// +/// The caller must ensure that `destination` and `source` are valid pointers for `length` bytes +/// and that the regions they point to do not violate Rust's aliasing rules for `core::ptr::copy`. +unsafe extern "efiapi" fn copy_mem(destination: *mut c_void, source: *mut c_void, length: usize) { // SAFETY: caller must ensure that the source and destination are valid for length bytes. unsafe { core::ptr::copy(source as *mut u8, destination as *mut u8, length) } } -extern "efiapi" fn set_mem(buffer: *mut c_void, size: usize, value: u8) { +/// # Safety +/// +/// The caller must ensure that `buffer` is a valid pointer to a contiguous region of at least +/// `size` bytes. +unsafe extern "efiapi" fn set_mem(buffer: *mut c_void, size: usize, value: u8) { // SAFETY: caller must ensure that the buffer is valid for size bytes. unsafe { let dst_buffer = from_raw_parts_mut(buffer as *mut u8, size); @@ -803,7 +859,15 @@ extern "efiapi" fn set_mem(buffer: *mut c_void, size: usize, value: u8) { } } -extern "efiapi" fn get_memory_map( +/// Returns the current UEFI memory map. +/// +/// # Safety +/// +/// `memory_map_size` must be a valid pointer to a `usize` indicating the buffer size. +/// `memory_map` must point to a writable buffer of at least `*memory_map_size` bytes, or be null +/// (in which case only the required size is returned). `map_key`, `descriptor_size`, and +/// `descriptor_version` must each be valid pointers or null. +unsafe extern "efiapi" fn get_memory_map( memory_map_size: *mut usize, memory_map: *mut efi::MemoryDescriptor, map_key: *mut usize, @@ -939,7 +1003,7 @@ fn process_hob_allocations(hob_list: &HobList) { continue; } - let mut address = desc.memory_base_address; + let address = desc.memory_base_address; match GCD.get_existent_memory_descriptor_for_address(address) { // we found the region in the GCD, so we can allocate it Ok(gcd_desc) => { @@ -962,7 +1026,7 @@ fn process_hob_allocations(hob_list: &HobList) { efi::ALLOCATE_ADDRESS, desc.memory_type, uefi_size_to_pages!(desc.memory_length as usize), - &mut address as *mut efi::PhysicalAddress, + address as efi::PhysicalAddress, None, ), GcdMemoryType::NonExistent | GcdMemoryType::Unaccepted => { @@ -984,7 +1048,7 @@ fn process_hob_allocations(hob_list: &HobList) { protocol_db::DXE_CORE_HANDLE, None, ) - .map(|_| ()), + .map(|address| address as efi::PhysicalAddress), }; if let Err(err) = alloc_res { @@ -1077,16 +1141,13 @@ fn process_hob_allocations(hob_list: &HobList) { // before we allocate page 0, as it may not live in system memory, in which case we cannot allocate it. match GCD.get_existent_memory_descriptor_for_address(0) { Ok(desc) if desc.memory_type == GcdMemoryType::SystemMemory => { - let mut address: efi::PhysicalAddress = 0; - if core_allocate_pages( - efi::ALLOCATE_ADDRESS, - efi::BOOT_SERVICES_DATA, - 1, - &mut address as *mut efi::PhysicalAddress, - None, - ) - .is_err() - { + let address: efi::PhysicalAddress = 0; + + // SAFETY: `address` is a local variable set to 0 for null pointer detection and this is + // valid expected usage. + let status = core_allocate_pages(efi::ALLOCATE_ADDRESS, efi::BOOT_SERVICES_DATA, 1, address, None); + + if status.is_err() { // if we failed, we should just continue, we will still unmap page 0, but it will be possible to // allocate by another entity, which is dangerous. log::warn!( @@ -1565,32 +1626,38 @@ mod tests { // test that disallowed types cannot be allocated assert_eq!( - allocate_pool(efi::CONVENTIONAL_MEMORY, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)), + // SAFETY: `buffer_ptr` is a valid local variable and its address points to writable memory. + unsafe { allocate_pool(efi::CONVENTIONAL_MEMORY, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)) }, efi::Status::INVALID_PARAMETER ); assert_eq!( - allocate_pool(efi::PERSISTENT_MEMORY, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)), + // SAFETY: `buffer_ptr` is a valid local variable and its address points to writable memory. + unsafe { allocate_pool(efi::PERSISTENT_MEMORY, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)) }, efi::Status::INVALID_PARAMETER ); assert_eq!( - allocate_pool(efi::UNUSABLE_MEMORY, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)), + // SAFETY: `buffer_ptr` is a valid local variable and its address points to writable memory. + unsafe { allocate_pool(efi::UNUSABLE_MEMORY, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)) }, efi::Status::SUCCESS ); assert_eq!( - allocate_pool(efi::UNACCEPTED_MEMORY_TYPE, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)), + // SAFETY: `buffer_ptr` is a valid local variable and its address points to writable memory. + unsafe { allocate_pool(efi::UNACCEPTED_MEMORY_TYPE, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)) }, efi::Status::INVALID_PARAMETER ); assert_eq!( - allocate_pool(efi::BOOT_SERVICES_DATA, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)), + // SAFETY: `buffer_ptr` is a valid local variable and its address points to writable memory. + unsafe { allocate_pool(efi::BOOT_SERVICES_DATA, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)) }, efi::Status::SUCCESS ); assert_eq!( - allocate_pool(efi::BOOT_SERVICES_DATA, 0x2000000, core::ptr::null_mut()), + // SAFETY: null pointer is passed intentionally to validate the error path. + unsafe { allocate_pool(efi::BOOT_SERVICES_DATA, 0x2000000, core::ptr::null_mut()) }, efi::Status::INVALID_PARAMETER ); }); @@ -1601,13 +1668,16 @@ mod tests { with_locked_state(GcdInit::WithSize(0x1000000), |_physical_hob_list| { let mut buffer_ptr = core::ptr::null_mut(); assert_eq!( - allocate_pool(efi::BOOT_SERVICES_DATA, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)), + // SAFETY: `buffer_ptr` is a valid local variable and its address points to writable memory. + unsafe { allocate_pool(efi::BOOT_SERVICES_DATA, 0x1000, core::ptr::addr_of_mut!(buffer_ptr)) }, efi::Status::SUCCESS ); - assert_eq!(free_pool(buffer_ptr), efi::Status::SUCCESS); + // SAFETY: `buffer_ptr` was successfully allocated above and points to valid pool memory. + assert_eq!(unsafe { free_pool(buffer_ptr) }, efi::Status::SUCCESS); - assert_eq!(free_pool(core::ptr::null_mut()), efi::Status::INVALID_PARAMETER); + // SAFETY: null pointer is intentional to test the error path. + assert_eq!(unsafe { free_pool(core::ptr::null_mut()) }, efi::Status::INVALID_PARAMETER); //TODO: these cause non-unwinding panic which crashes the test even with "#[should_panic]". //assert_eq!(free_pool(buffer_ptr), efi::Status::INVALID_PARAMETER); //assert_eq!(free_pool(((buffer_ptr as usize) + 10) as *mut c_void), efi::Status::INVALID_PARAMETER); @@ -1676,116 +1746,150 @@ mod tests { with_locked_state(GcdInit::WithSize(0x1000000), |_physical_hob_list| { //test test null memory pointer fails with invalid param. assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::BOOT_SERVICES_DATA, - 0x4, - core::ptr::null_mut() as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::BOOT_SERVICES_DATA, + 0x4, + core::ptr::null_mut() as *mut efi::PhysicalAddress, + ) + }, efi::Status::INVALID_PARAMETER ); //test can't allocate un-allocatable types assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::CONVENTIONAL_MEMORY, - 0x4, - core::ptr::null_mut() as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::CONVENTIONAL_MEMORY, + 0x4, + core::ptr::null_mut() as *mut efi::PhysicalAddress, + ) + }, efi::Status::INVALID_PARAMETER ); assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::PERSISTENT_MEMORY, - 0x4, - core::ptr::null_mut() as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::PERSISTENT_MEMORY, + 0x4, + core::ptr::null_mut() as *mut efi::PhysicalAddress, + ) + }, efi::Status::INVALID_PARAMETER ); assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::UNUSABLE_MEMORY, - 0x4, - core::ptr::null_mut() as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::UNUSABLE_MEMORY, + 0x4, + core::ptr::null_mut() as *mut efi::PhysicalAddress, + ) + }, efi::Status::INVALID_PARAMETER ); assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::UNACCEPTED_MEMORY_TYPE, - 0x4, - core::ptr::null_mut() as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::UNACCEPTED_MEMORY_TYPE, + 0x4, + core::ptr::null_mut() as *mut efi::PhysicalAddress, + ) + }, efi::Status::INVALID_PARAMETER ); //test successful allocate_any let mut buffer_ptr: *mut u8 = core::ptr::null_mut(); assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::BOOT_SERVICES_DATA, - 0x10, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::BOOT_SERVICES_DATA, + 0x10, + core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::SUCCESS ); - free_pages(buffer_ptr as u64, 0x10); + // SAFETY: `buffer_ptr` was successfully allocated above and points to a valid page range of 0x10 pages. + unsafe { free_pages(buffer_ptr as u64, 0x10) }; //test successful allocate_address at the address that was just freed assert_eq!( - allocate_pages( - efi::ALLOCATE_ADDRESS, - efi::BOOT_SERVICES_DATA, - 0x10, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ADDRESS, + efi::BOOT_SERVICES_DATA, + 0x10, + core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::SUCCESS ); - free_pages(buffer_ptr as u64, 0x10); + // SAFETY: `buffer_ptr` was successfully allocated above and points to a valid page range of 0x10 pages. + unsafe { free_pages(buffer_ptr as u64, 0x10) }; //test successful allocate_max where max is greater than the address that was just freed. buffer_ptr = buffer_ptr.wrapping_add(0x11 * 0x1000); assert_eq!( - allocate_pages( - efi::ALLOCATE_MAX_ADDRESS, - efi::BOOT_SERVICES_DATA, - 0x10, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_MAX_ADDRESS, + efi::BOOT_SERVICES_DATA, + 0x10, + core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::SUCCESS ); - free_pages(buffer_ptr as u64, 0x10); + // SAFETY: `buffer_ptr` was successfully allocated above and points to a valid page range of 0x10 pages. + unsafe { free_pages(buffer_ptr as u64, 0x10) }; //test invalid allocation type assert_eq!( - allocate_pages( - 0x12345, - efi::BOOT_SERVICES_DATA, - 0x10, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + 0x12345, + efi::BOOT_SERVICES_DATA, + 0x10, + core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::INVALID_PARAMETER ); //test creation of new allocator for OS/OEM defined allocator type. assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - 0x71234567, - 0x10, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + 0x71234567, + 0x10, + core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::SUCCESS ); - free_pages(buffer_ptr as u64, 0x10); + // SAFETY: `buffer_ptr` was successfully allocated above and points to a valid page range of 0x10 pages. + unsafe { free_pages(buffer_ptr as u64, 0x10) }; let allocators = ALLOCATORS.lock(); let allocator = allocators.get_allocator(0x71234567).unwrap(); let handle = allocator.handle(); @@ -1795,22 +1899,28 @@ mod tests { //test that creation of new allocator for illegal type fails. assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::PERSISTENT_MEMORY, - 0x10, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::PERSISTENT_MEMORY, + 0x10, + core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::INVALID_PARAMETER ); assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::UNUSABLE_MEMORY, - 0x10, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::UNUSABLE_MEMORY, + 0x10, + core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::SUCCESS ); }) @@ -1819,10 +1929,14 @@ mod tests { #[test] fn free_pages_error_scenarios_should_be_handled_properly() { with_locked_state(GcdInit::WithSize(0x1000000), |_physical_hob_list| { - assert_eq!(free_pages(0x12345000, !0xFFF), efi::Status::INVALID_PARAMETER); - assert_eq!(free_pages(!0xFFF, 0x10), efi::Status::INVALID_PARAMETER); - assert_eq!(free_pages(0x12345678, 1), efi::Status::INVALID_PARAMETER); - assert_eq!(free_pages(0x12345000, 1), efi::Status::NOT_FOUND); + // SAFETY: invalid page count to test overflow detection. + assert_eq!(unsafe { free_pages(0x12345000, !0xFFF) }, efi::Status::INVALID_PARAMETER); + // SAFETY: address near end of address space to test overflow detection. + assert_eq!(unsafe { free_pages(!0xFFF, 0x10) }, efi::Status::INVALID_PARAMETER); + // SAFETY: misaligned address to test alignment validation. + assert_eq!(unsafe { free_pages(0x12345678, 1) }, efi::Status::INVALID_PARAMETER); + // SAFETY: valid-looking but unallocated address to test not-found path. + assert_eq!(unsafe { free_pages(0x12345000, 1) }, efi::Status::NOT_FOUND); }); } @@ -1830,14 +1944,16 @@ mod tests { fn copy_mem_should_copy_mem() { let mut dest = vec![0xa5u8; 0x10]; let mut src = vec![0x5au8; 0x10]; - copy_mem(dest.as_mut_ptr() as *mut c_void, src.as_mut_ptr() as *mut c_void, 0x10); + // SAFETY: The passed in values are safe because they are constructed in this test case. + unsafe { copy_mem(dest.as_mut_ptr() as *mut c_void, src.as_mut_ptr() as *mut c_void, 0x10) }; assert_eq!(dest, src); } #[test] fn set_mem_should_set_mem() { let mut dest = vec![0xa5u8; 0x10]; - set_mem(dest.as_mut_ptr() as *mut c_void, 0x10, 0x00); + // SAFETY: The passed in values are safe because they are constructed in this test case. + unsafe { set_mem(dest.as_mut_ptr() as *mut c_void, 0x10, 0x00) }; assert_eq!(dest, vec![0x00u8; 0x10]); } @@ -1850,24 +1966,30 @@ mod tests { // allocate some "custom" type pages to create something interesting to find in the map. let mut buffer_ptr: *mut u8 = core::ptr::null_mut(); assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - 0x71234567, - 0x10, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + 0x71234567, + 0x10, + core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::SUCCESS ); // allocate some "runtime" type pages to create something interesting to find in the map. let mut runtime_buffer_ptr: *mut u8 = core::ptr::null_mut(); assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::RUNTIME_SERVICES_DATA, - 0x10, - core::ptr::addr_of_mut!(runtime_buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::RUNTIME_SERVICES_DATA, + 0x10, + core::ptr::addr_of_mut!(runtime_buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::SUCCESS ); @@ -1875,13 +1997,17 @@ mod tests { let mut map_key = 0; let mut descriptor_size = 0; let mut version = 0; - let status = get_memory_map( - core::ptr::addr_of_mut!(memory_map_size), - core::ptr::null_mut(), - core::ptr::addr_of_mut!(map_key), - core::ptr::addr_of_mut!(descriptor_size), - core::ptr::addr_of_mut!(version), - ); + // SAFETY: all pointers are derived from local variables declared above and are valid. + // memory_map is null to perform a size query. + let status = unsafe { + get_memory_map( + core::ptr::addr_of_mut!(memory_map_size), + core::ptr::null_mut(), + core::ptr::addr_of_mut!(map_key), + core::ptr::addr_of_mut!(descriptor_size), + core::ptr::addr_of_mut!(version), + ) + }; assert_eq!(status, efi::Status::BUFFER_TOO_SMALL); assert_ne!(memory_map_size, 0); assert_eq!(descriptor_size, core::mem::size_of::()); @@ -1899,13 +2025,17 @@ mod tests { memory_map_size / descriptor_size ]; - let status = get_memory_map( - core::ptr::addr_of_mut!(memory_map_size), - memory_map_buffer.as_mut_ptr(), - core::ptr::addr_of_mut!(map_key), - core::ptr::addr_of_mut!(descriptor_size), - core::ptr::addr_of_mut!(version), - ); + // SAFETY: all pointers are derived from local variables. memory_map_buffer is + // properly sized from the previous size query. + let status = unsafe { + get_memory_map( + core::ptr::addr_of_mut!(memory_map_size), + memory_map_buffer.as_mut_ptr(), + core::ptr::addr_of_mut!(map_key), + core::ptr::addr_of_mut!(descriptor_size), + core::ptr::addr_of_mut!(version), + ) + }; assert_eq!(status, efi::Status::SUCCESS); assert_eq!(descriptor_size, core::mem::size_of::()); assert_eq!(version, 1); @@ -1943,13 +2073,17 @@ mod tests { .expect("Failed to find runtime allocation."); //get_memory_map with null size should return invalid parameter - let status = get_memory_map( - core::ptr::null_mut(), - memory_map_buffer.as_mut_ptr(), - core::ptr::addr_of_mut!(map_key), - core::ptr::addr_of_mut!(descriptor_size), - core::ptr::addr_of_mut!(version), - ); + // SAFETY: memory_map_size is intentionally null to test the error path. + // All other pointers are valid local variables. + let status = unsafe { + get_memory_map( + core::ptr::null_mut(), + memory_map_buffer.as_mut_ptr(), + core::ptr::addr_of_mut!(map_key), + core::ptr::addr_of_mut!(descriptor_size), + core::ptr::addr_of_mut!(version), + ) + }; assert_eq!(status, efi::Status::INVALID_PARAMETER); }) } @@ -1960,24 +2094,30 @@ mod tests { // allocate some "custom" type pages to create something interesting to find in the map. let mut buffer_ptr: *mut u8 = core::ptr::null_mut(); assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - 0x71234567, - 0x10, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + 0x71234567, + 0x10, + core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::SUCCESS ); // allocate some "custom" type pages to create something interesting to find in the map. let mut runtime_buffer_ptr: *mut u8 = core::ptr::null_mut(); assert_eq!( - allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::RUNTIME_SERVICES_DATA, - 0x10, - core::ptr::addr_of_mut!(runtime_buffer_ptr) as *mut efi::PhysicalAddress - ), + // SAFETY: the parameters are as expected for the test. + unsafe { + allocate_pages( + efi::ALLOCATE_ANY_PAGES, + efi::RUNTIME_SERVICES_DATA, + 0x10, + core::ptr::addr_of_mut!(runtime_buffer_ptr) as *mut efi::PhysicalAddress, + ) + }, efi::Status::SUCCESS ); @@ -1986,13 +2126,17 @@ mod tests { let mut map_key = 0; let mut descriptor_size = 0; let mut version = 0; - let status = get_memory_map( - core::ptr::addr_of_mut!(memory_map_size), - core::ptr::null_mut(), - core::ptr::addr_of_mut!(map_key), - core::ptr::addr_of_mut!(descriptor_size), - core::ptr::addr_of_mut!(version), - ); + // SAFETY: all pointers are derived from local variables declared above and are valid. + // memory_map is null to perform a size query. + let status = unsafe { + get_memory_map( + core::ptr::addr_of_mut!(memory_map_size), + core::ptr::null_mut(), + core::ptr::addr_of_mut!(map_key), + core::ptr::addr_of_mut!(descriptor_size), + core::ptr::addr_of_mut!(version), + ) + }; assert_eq!(status, efi::Status::BUFFER_TOO_SMALL); let mut memory_map_buffer: Vec = vec![ @@ -2006,13 +2150,17 @@ mod tests { memory_map_size / descriptor_size ]; - let status = get_memory_map( - core::ptr::addr_of_mut!(memory_map_size), - memory_map_buffer.as_mut_ptr(), - core::ptr::addr_of_mut!(map_key), - core::ptr::addr_of_mut!(descriptor_size), - core::ptr::addr_of_mut!(version), - ); + // SAFETY: all pointers are derived from local variables. memory_map_buffer is + // properly sized from the previous size query. + let status = unsafe { + get_memory_map( + core::ptr::addr_of_mut!(memory_map_size), + memory_map_buffer.as_mut_ptr(), + core::ptr::addr_of_mut!(map_key), + core::ptr::addr_of_mut!(descriptor_size), + core::ptr::addr_of_mut!(version), + ) + }; assert_eq!(status, efi::Status::SUCCESS); assert!(terminate_memory_map(map_key).is_ok()); diff --git a/patina_dxe_core/src/allocator/uefi_allocator.rs b/patina_dxe_core/src/allocator/uefi_allocator.rs index 89acb2083..f63f07e2d 100644 --- a/patina_dxe_core/src/allocator/uefi_allocator.rs +++ b/patina_dxe_core/src/allocator/uefi_allocator.rs @@ -148,8 +148,8 @@ where } }; - //TODO: trusting that "buffer" is legit is pretty naive - but performant. Presently the allocator doesn't have - //tracking mechanisms that permit the validation of the pointer (hence the unsafe). + // TODO: trusting that "buffer" is legit is pretty naive - but performant. Presently the allocator doesn't have + // tracking mechanisms that permit the validation of the pointer (hence the unsafe). // SAFETY: Caller must follow safety contract defined by this function. let mut ptr = unsafe { @@ -168,7 +168,7 @@ where if allocation_info.memory_type != self.memory_type() { return Err(EfiError::NotFound); } - //zero after check so it doesn't get reused. + // zero after check so it doesn't get reused. allocation_info.signature = 0; // SAFETY: Caller must follow safety contract defined by this function. diff --git a/patina_dxe_core/src/allocator/usage_tests/uefi_memory_map.rs b/patina_dxe_core/src/allocator/usage_tests/uefi_memory_map.rs index f55c84a86..fc25029c8 100644 --- a/patina_dxe_core/src/allocator/usage_tests/uefi_memory_map.rs +++ b/patina_dxe_core/src/allocator/usage_tests/uefi_memory_map.rs @@ -219,13 +219,17 @@ mod tests { let mut descriptor_size: usize = 0; let mut descriptor_version: u32 = 0; - let status = get_memory_map( - ptr::from_mut(&mut memory_map_size), - ptr::null_mut(), - ptr::from_mut(&mut map_key), - ptr::from_mut(&mut descriptor_size), - ptr::from_mut(&mut descriptor_version), - ); + // SAFETY: all pointers are derived from local variables and are valid. + // memory_map is null to perform a size query. + let status = unsafe { + get_memory_map( + ptr::from_mut(&mut memory_map_size), + ptr::null_mut(), + ptr::from_mut(&mut map_key), + ptr::from_mut(&mut descriptor_size), + ptr::from_mut(&mut descriptor_version), + ) + }; if status != efi::Status::BUFFER_TOO_SMALL { return Err(format!("Expected BUFFER_TOO_SMALL, got {:?}", status)); @@ -237,13 +241,17 @@ mod tests { // SAFETY: Capacity was reserved for `descriptor_count` elements and the length below matches that. unsafe { descriptors.set_len(descriptor_count) }; - let status = get_memory_map( - ptr::from_mut(&mut memory_map_size), - descriptors.as_mut_ptr().cast(), - ptr::from_mut(&mut map_key), - ptr::from_mut(&mut descriptor_size), - ptr::from_mut(&mut descriptor_version), - ); + // SAFETY: all pointers are derived from local variables. descriptors buffer + // is properly sized from the previous size query. + let status = unsafe { + get_memory_map( + ptr::from_mut(&mut memory_map_size), + descriptors.as_mut_ptr().cast(), + ptr::from_mut(&mut map_key), + ptr::from_mut(&mut descriptor_size), + ptr::from_mut(&mut descriptor_version), + ) + }; if status != efi::Status::SUCCESS { return Err(format!("get_memory_map() failed: {:?}", status)); diff --git a/patina_dxe_core/src/config_tables.rs b/patina_dxe_core/src/config_tables.rs index 654d73841..ca532b733 100644 --- a/patina_dxe_core/src/config_tables.rs +++ b/patina_dxe_core/src/config_tables.rs @@ -23,7 +23,15 @@ use crate::{ systemtables::{EfiSystemTable, SYSTEM_TABLE}, }; -extern "efiapi" fn install_configuration_table(table_guid: *mut efi::Guid, table: *mut c_void) -> efi::Status { +/// Installs a configuration table entry identified by `table_guid` into the system table. +/// +/// # Safety +/// +/// The caller is responsible for ensuring that `table_guid` points to valid, readable memory +/// containing an `efi::Guid` and that `table` (if non-null) points to valid memory that remains +/// valid for the lifetime of the configuration table entry. `table_guid` is null-checked, but +/// validity of the referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn install_configuration_table(table_guid: *mut efi::Guid, table: *mut c_void) -> efi::Status { if table_guid.is_null() { return efi::Status::INVALID_PARAMETER; } @@ -43,8 +51,10 @@ extern "efiapi" fn install_configuration_table(table_guid: *mut efi::Guid, table } } -/// Install a configuration table in the system table, replacing any existing table with the same GUID. -/// If a table is replaced or deleted, a pointer to the old table is returned. +/// Install a configuration table in the system table, replacing any existing table with the same +/// GUID. If a table is replaced or deleted, a pointer to the old table is returned. This function +/// is not marked as unsafe because the `vendor_table` parameter is not dereferenced inside the +/// function. pub fn core_install_configuration_table( vendor_guid: efi::Guid, vendor_table: *mut c_void, @@ -184,7 +194,11 @@ mod tests { assert!(get_configuration_table(&guid).is_none()); - assert_eq!(install_configuration_table(&guid as *const _ as *mut _, table), efi::Status::SUCCESS); + assert_eq!( + // SAFETY: The passed in values are safe because they are constructed in this test case. + unsafe { install_configuration_table(&guid as *const _ as *mut _, table) }, + efi::Status::SUCCESS + ); assert_eq!(get_configuration_table(&guid).unwrap().as_ptr(), table); }); } @@ -195,7 +209,11 @@ mod tests { let guid: efi::Guid = guid::Guid::from_string("78926ab0-af16-49e4-8e05-115aafbca1df").to_efi_guid(); let table = 0x12345678u32 as *mut c_void; - assert_eq!(install_configuration_table(&guid as *const _ as *mut _, table), efi::Status::SUCCESS); + assert_eq!( + // SAFETY: The passed in values are safe because they are constructed in this test case. + unsafe { install_configuration_table(&guid as *const _ as *mut _, table) }, + efi::Status::SUCCESS + ); assert_eq!(get_configuration_table(&guid).unwrap().as_ptr(), table); diff --git a/patina_dxe_core/src/config_tables/memory_attributes_table.rs b/patina_dxe_core/src/config_tables/memory_attributes_table.rs index 9528c0ce0..4744ac434 100644 --- a/patina_dxe_core/src/config_tables/memory_attributes_table.rs +++ b/patina_dxe_core/src/config_tables/memory_attributes_table.rs @@ -313,27 +313,22 @@ mod tests { let page_count = entry_count * uefi_size_to_pages!(crate::allocator::RUNTIME_PAGE_ALLOCATION_GRANULARITY); - let mut buffer_ptr: *mut u8 = core::ptr::null_mut(); - match core_allocate_pages( - efi::ALLOCATE_ANY_PAGES, - page_type.0, - page_count, - core::ptr::addr_of_mut!(buffer_ptr) as *mut efi::PhysicalAddress, - None, - ) { + let address = match core_allocate_pages(efi::ALLOCATE_ANY_PAGES, page_type.0, page_count, 0, None) { // because we allocate top down, we need to insert at the front of the vector - Ok(_) if page_type.0 != efi::BOOT_SERVICES_DATA => { - allocated_pages.insert(0, (buffer_ptr, page_type, page_count)) + Ok(address) => { + if page_type.0 != efi::BOOT_SERVICES_DATA { + allocated_pages.insert(0, (address, page_type, page_count)); + } + address } - Ok(_) => (), _ => panic!("Failed to allocate pages"), - } + }; let len = page_count * UEFI_PAGE_SIZE; // ignore failures here, we can't set attributes in the actual page table here, but the GCD will // get updated - let _ = core_set_memory_space_capabilities(buffer_ptr as u64, len as u64, u64::MAX); - let _ = core_set_memory_space_attributes(buffer_ptr as u64, len as u64, page_type.1); + let _ = core_set_memory_space_capabilities(address, len as u64, u64::MAX); + let _ = core_set_memory_space_attributes(address, len as u64, page_type.1); } // before we create the MAT, we expect MEMORY_ATTRIBUTES_TABLE to be None @@ -368,7 +363,7 @@ mod tests { // We don't assume ordering; find by physical_start and number_of_pages. for page in allocated_pages.iter() { let expected_type = page.1.0; - let expected_physical_start = page.0 as u64; + let expected_physical_start = page.0; let expected_number_of_pages = page.2 as u64; // expected_attribute from setup isn't used directly; MAT constrains attrs based on type. diff --git a/patina_dxe_core/src/debugger_reload.rs b/patina_dxe_core/src/debugger_reload.rs index 2969d084a..c8388796e 100644 --- a/patina_dxe_core/src/debugger_reload.rs +++ b/patina_dxe_core/src/debugger_reload.rs @@ -194,10 +194,11 @@ fn core_reload(image: &[u8], out: &mut dyn core::fmt::Write) { // Step 1: allocate the image memory. let image_size = pe_info.size_of_image as usize; - let alloc = match CoreMemoryManager.allocate_pages( - uefi_size_to_pages!(image_size), - AllocationOptions::new().with_strategy(ARCH_ALLOCATION_STRATEGY), - ) { + // SAFETY: ARCH_ALLOCATION_STRATEGY is a compile-time constant that is either + // PageAllocationStrategy::MaxAddress(0xFFFF_FFFF) or PageAllocationStrategy::Any; + // neither requires dereferencing a caller provided address. + let options = unsafe { AllocationOptions::new().with_strategy(ARCH_ALLOCATION_STRATEGY) }; + let alloc = match CoreMemoryManager.allocate_pages(uefi_size_to_pages!(image_size), options) { Ok(pages) => pages, Err(err) => { let _ = writeln!(out, "Failed to allocate load buffer: {:?}", err); diff --git a/patina_dxe_core/src/driver_services.rs b/patina_dxe_core/src/driver_services.rs index f156cf58b..fcf0378f8 100644 --- a/patina_dxe_core/src/driver_services.rs +++ b/patina_dxe_core/src/driver_services.rs @@ -56,11 +56,15 @@ fn get_platform_driver_override_bindings( let mut driver_overrides = Vec::new(); let mut driver_image_handle: efi::Handle = core::ptr::null_mut(); loop { - let status = (driver_override_protocol.get_driver)( - driver_override_protocol, - controller_handle, - core::ptr::addr_of_mut!(driver_image_handle), - ); + // SAFETY: get_driver is an EFI function pointer that is expected to be safe to call with a valid + // protocol pointer. We have already verified that driver_override_protocol is a valid pointer above. + let status = unsafe { + (driver_override_protocol.get_driver)( + driver_override_protocol, + controller_handle, + core::ptr::addr_of_mut!(driver_image_handle), + ) + }; if status != efi::Status::SUCCESS { break; } @@ -88,7 +92,10 @@ fn get_family_override_bindings() -> Vec<*mut efi::protocols::driver_binding::Pr .as_mut() .expect("bad protocol ptr") }; - let version = (driver_override_protocol.get_version)(driver_override_protocol); + + // SAFETY: get_version is an EFI function pointer that is expected to be safe to call with a valid + // protocol pointer. We have already verified that driver_override_protocol is a valid pointer above. + let version = unsafe { (driver_override_protocol.get_version)(driver_override_protocol) }; driver_override_map.insert(version, handle); } Err(_) => continue, @@ -117,10 +124,14 @@ fn get_bus_specific_override_bindings( let mut bus_overrides = Vec::new(); let mut driver_image_handle: efi::Handle = core::ptr::null_mut(); loop { - let status = (bus_specific_override_protocol.get_driver)( - bus_specific_override_protocol, - core::ptr::addr_of_mut!(driver_image_handle), - ); + // SAFETY: get_driver is an EFI function pointer that is expected to be safe to call with a valid + // protocol pointer. We have already verified that bus_specific_override_protocol is a valid pointer above. + let status = unsafe { + (bus_specific_override_protocol.get_driver)( + bus_specific_override_protocol, + core::ptr::addr_of_mut!(driver_image_handle), + ) + }; if status != efi::Status::SUCCESS { break; } @@ -227,7 +238,7 @@ fn core_connect_single_controller( driver_bindings.retain(|x| !driver_candidates.contains(x)); driver_candidates.append(&mut driver_bindings); - //loop until no more drivers can be started on handle. + // loop until no more drivers can be started on handle. let mut one_started = false; loop { let mut started_drivers = Vec::new(); @@ -243,8 +254,12 @@ fn core_connect_single_controller( create_performance_measurement, ); - //driver claims support; attempt to start it. - match (driver_binding.supported)(driver_binding_interface, controller_handle, device_path) { + // Driver claims support; attempt to start it. + // SAFETY: driver_binding_interface is a valid pointer to a driver binding protocol instance + // as ensured by the construction of driver_candidates above. + let status = + unsafe { (driver_binding.supported)(driver_binding_interface, controller_handle, device_path) }; + match status { efi::Status::SUCCESS => { perf_driver_binding_support_end( driver_binding.driver_binding_handle, @@ -260,9 +275,11 @@ fn core_connect_single_controller( create_performance_measurement, ); - if (driver_binding.start)(driver_binding_interface, controller_handle, device_path) - == efi::Status::SUCCESS - { + // SAFETY: driver_binding_interface is a valid pointer to a driver binding protocol instance + // as ensured by the construction of driver_candidates above. + let status = + unsafe { (driver_binding.start)(driver_binding_interface, controller_handle, device_path) }; + if status == efi::Status::SUCCESS { one_started = true; } @@ -315,7 +332,7 @@ fn core_connect_single_controller( /// valid for the duration of its execution. For example, if a driver were be unloaded in a timer callback after /// returning true from Supported() before Start() is called, then the driver binding instance would be uninstalled or /// invalid and Start() would be an invalid function pointer when invoked. In general, the spec implicitly assumes -/// that driver binding instances that are valid at the start of he call to ConnectController() must remain valid for +/// that driver binding instances that are valid at the start of the call to ConnectController() must remain valid for /// the duration of the ConnectController() call. If this is not true, then behavior is undefined. This function is /// marked unsafe for this reason. /// @@ -347,7 +364,34 @@ pub unsafe fn core_connect_controller( return_status } -extern "efiapi" fn connect_controller( +/// Raw UEFI `ConnectController()` entry point installed into the boot services +/// table. +/// +/// NOTE: This routine cannot hold the protocol db lock while executing +/// DriverBinding->Supported()/Start() since they need to access protocol db +/// services. That means this routine can't guarantee that driver bindings +/// remain valid for the duration of its execution. For example, if a driver +/// were be unloaded in a timer callback after returning true from Supported() +/// before Start() is called, then the driver binding instance would be +/// uninstalled or invalid and Start() would be an invalid function pointer +/// when invoked. In general, the spec implicitly assumes that driver binding +/// instances that are valid at the start of the call to ConnectController() +/// must remain valid for the duration of the ConnectController() call. If this +/// is not true, then behavior is undefined. This function is marked unsafe for +/// this reason. +/// +/// # Safety +/// +/// The caller must ensure that: +/// - If `driver_image_handle` is non-null, it must point to a valid, +/// null-terminated list of image handles. +/// - If `remaining_device_path` is non-null, it must point to a valid device +/// path structure. +/// - The other parameters are either native Rust types or opaque UEFI handles +/// wrapped inside Rust types. Passing an invalid value does not by itself +/// cause undefined behavior; the firmware is expected to reject it by +/// returning an error status. +unsafe extern "efiapi" fn connect_controller( handle: efi::Handle, driver_image_handle: *mut efi::Handle, remaining_device_path: *mut efi::protocols::device_path::Protocol, @@ -390,14 +434,13 @@ extern "efiapi" fn connect_controller( /// 7.3.13. Refer to the UEFI spec description for details on input parameters, behavior, and error return codes. /// /// # Safety -/// This routine cannot hold the protocol db lock while executing DriverBinding->Supported()/Start() since -/// they need to access protocol db services. That means this routine can't guarantee that driver bindings remain -/// valid for the duration of its execution. For example, if a driver were be unloaded in a timer callback after -/// returning true from Supported() before Start() is called, then the driver binding instance would be uninstalled or -/// invalid and Start() would be an invalid function pointer when invoked. In general, the spec implicitly assumes -/// that driver binding instances that are valid at the start of he call to ConnectController() must remain valid for -/// the duration of the ConnectController() call. If this is not true, then behavior is undefined. This function is -/// marked unsafe for this reason. +/// This routine cannot hold the protocol db lock while executing DriverBinding->Stop() since it needs to access +/// protocol db services. That means this routine can't guarantee that driver bindings remain valid for the duration +/// of its execution. For example, if a driver were to be unloaded in a timer callback while Stop() is being called +/// on another driver, the driver binding instance could become invalid and Stop() would be an invalid function +/// pointer when invoked. In general, the spec implicitly assumes that driver binding instances that are valid at the +/// start of the call to DisconnectController() must remain valid for the duration of the DisconnectController() +/// call. If this is not true, then behavior is undefined. This function is marked unsafe for this reason. /// /// ## Example /// @@ -488,21 +531,21 @@ pub unsafe fn core_disconnect_controller( let total_children = child_handles.len(); let mut is_only_child = false; if let Some(handle) = child_handle { - //if the child was specified, but was the only child, then the driver should be disconnected. - //if the child was specified, but other children were present, then the driver should not be disconnected. + // if the child was specified, but was the only child, then the driver should be disconnected. + // if the child was specified, but other children were present, then the driver should not be disconnected. child_handles.retain(|x| x == &handle); is_only_child = total_children == child_handles.len(); } - //resolve the handle to the driver_binding. - //N.B. Corner case: a driver could install a driver-binding instance; then be asked to manage a controller (and - //thus, become an agent_handle in the open protocol information), and then something uninstalls the driver binding - //from the agent_handle. This would mean that the agent_handle now no longer supports the driver binding but is - //marked in the protocol database as managing the controller. This code just returns INVALID_PARAMETER in this case - //(which effectively makes the controller "un-disconnect-able" since all subsequent disconnects will also fail for - //the same reason). This matches the reference C implementation. As an enhancement, the core could track driver - //bindings that are actively managing controllers and return an ACCESS_DENIED status if something attempts to - //uninstall a binding that is in use. + // resolve the handle to the driver_binding. + // N.B. Corner case: a driver could install a driver-binding instance; then be asked to manage a controller (and + // thus, become an agent_handle in the open protocol information), and then something uninstalls the driver binding + // from the agent_handle. This would mean that the agent_handle now no longer supports the driver binding but is + // marked in the protocol database as managing the controller. This code just returns INVALID_PARAMETER in this case + // (which effectively makes the controller "un-disconnect-able" since all subsequent disconnects will also fail for + // the same reason). This matches the reference C implementation. As an enhancement, the core could track driver + // bindings that are actively managing controllers and return an ACCESS_DENIED status if something attempts to + // uninstall a binding that is in use. let driver_binding_interface = PROTOCOL_DB .get_interface_for_handle(driver_handle, efi::protocols::driver_binding::PROTOCOL_GUID) .or(Err(EfiError::InvalidParameter))?; @@ -513,16 +556,23 @@ pub unsafe fn core_disconnect_controller( let mut status = efi::Status::SUCCESS; if !child_handles.is_empty() { - //disconnect the child controller(s). - status = (driver_binding.stop)( - driver_binding_interface, - controller_handle, - child_handles.len(), - child_handles.as_mut_ptr(), - ); + // Disconnect the child controller(s). + // SAFETY: driver_binding_interface is a valid pointer to a driver binding protocol instance + // as ensured by the construction of driver_candidates above. + status = unsafe { + (driver_binding.stop)( + driver_binding_interface, + controller_handle, + child_handles.len(), + child_handles.as_mut_ptr(), + ) + }; } if status == efi::Status::SUCCESS && (child_handle.is_none() || is_only_child) { - status = (driver_binding.stop)(driver_binding_interface, controller_handle, 0, core::ptr::null_mut()); + // SAFETY: driver_binding_interface is a valid pointer to a driver binding protocol instance + // as ensured by the construction of driver_candidates above. + status = + unsafe { (driver_binding.stop)(driver_binding_interface, controller_handle, 0, core::ptr::null_mut()) }; } if status == efi::Status::SUCCESS { one_or_more_drivers_disconnected = true; @@ -532,7 +582,24 @@ pub unsafe fn core_disconnect_controller( if one_or_more_drivers_disconnected || no_drivers { Ok(()) } else { Err(EfiError::NotFound) } } -extern "efiapi" fn disconnect_controller( +/// # Safety +/// +/// This routine cannot hold the protocol db lock while executing +/// DriverBinding->Stop() since it needs to access protocol db services. That +/// means this routine can't guarantee that driver bindings remain valid for the +/// duration of its execution. For example, if a driver were to be unloaded in a +/// timer callback while Stop() is being called on another driver, the driver +/// binding instance could become invalid and Stop() would be an invalid +/// function pointer when invoked. In general, the spec implicitly assumes that +/// driver binding instances that are valid at the start of the call to +/// DisconnectController() must remain valid for the duration of the +/// DisconnectController() call. If this is not true, then behavior is +/// undefined. This function is marked unsafe for this reason. +/// +/// - All parameters are opaque UEFI handles wrapped inside Rust types. +/// Passing an invalid value does not by itself cause undefined behavior; +/// the firmware is expected to reject it by returning an error status. +unsafe extern "efiapi" fn disconnect_controller( controller_handle: efi::Handle, driver_image_handle: efi::Handle, child_handle: efi::Handle, @@ -1393,12 +1460,16 @@ mod tests { // Test 1: Call with single driver handle let mut driver_handles = vec![driver_handle1, core::ptr::null_mut()]; - let status = connect_controller( - controller_handle, - driver_handles.as_mut_ptr(), - core::ptr::null_mut(), // No remaining device path - efi::Boolean::FALSE, - ); + // SAFETY: controller_handle is a valid handle installed above. driver_handles is a + // null-terminated array of valid image handles. Remaining device path is null. + let status = unsafe { + connect_controller( + controller_handle, + driver_handles.as_mut_ptr(), + core::ptr::null_mut(), // No remaining device path + efi::Boolean::FALSE, + ) + }; assert_eq!(status, efi::Status::SUCCESS); assert_eq!(SUPPORTED_CALL_COUNT.load(Ordering::SeqCst), 1); @@ -1408,12 +1479,16 @@ mod tests { SUPPORTED_CALL_COUNT.store(0, Ordering::SeqCst); START_CALL_COUNT.store(0, Ordering::SeqCst); - let status = connect_controller( - controller_handle, - core::ptr::null_mut(), // Null driver handle array - core::ptr::null_mut(), // No remaining device path - efi::Boolean::FALSE, - ); + // SAFETY: controller_handle is a valid handle installed above. Both driver image + // handle and remaining device path are null, which are valid inputs. + let status = unsafe { + connect_controller( + controller_handle, + core::ptr::null_mut(), // Null driver handle array + core::ptr::null_mut(), // No remaining device path + efi::Boolean::FALSE, + ) + }; assert_eq!(status, efi::Status::SUCCESS); // At least one support call should have happened @@ -1825,17 +1900,22 @@ mod tests { .unwrap(); // Test the extern "efiapi" function with null handles (should succeed for empty controller) - let status = disconnect_controller( - controller_handle, - core::ptr::null_mut(), // No specific driver - core::ptr::null_mut(), // No child handle - ); + // SAFETY: Test code - controller_handle was obtained from install_protocol_interface above. + // Null driver/child handles are valid per UEFI spec (disconnect all). + let status = unsafe { + disconnect_controller( + controller_handle, + core::ptr::null_mut(), // No specific driver + core::ptr::null_mut(), // No child handle + ) + }; assert_eq!(status, efi::Status::SUCCESS, "disconnect_controller should succeed with null handles"); // Test with invalid controller handle let invalid_handle = 0x9999 as efi::Handle; - let status = disconnect_controller(invalid_handle, core::ptr::null_mut(), core::ptr::null_mut()); + // SAFETY: Test code - intentionally passing invalid handle to verify error handling. + let status = unsafe { disconnect_controller(invalid_handle, core::ptr::null_mut(), core::ptr::null_mut()) }; // Should return error status for invalid handle assert_ne!(status, efi::Status::SUCCESS, "disconnect_controller should fail with invalid handle"); diff --git a/patina_dxe_core/src/dxe_services.rs b/patina_dxe_core/src/dxe_services.rs index 8cad89644..22dfdbe57 100644 --- a/patina_dxe_core/src/dxe_services.rs +++ b/patina_dxe_core/src/dxe_services.rs @@ -1472,7 +1472,8 @@ mod tests { let out_slice = unsafe { core::slice::from_raw_parts(out_ptr, out_count) }; assert_eq!(out_slice, expected.as_slice()); - assert!(crate::allocator::core_free_pool(out_ptr as *mut core::ffi::c_void).is_ok()); + // SAFETY: `out_ptr` was allocated by `get_memory_space_map` and is valid pool memory. + assert!(unsafe { crate::allocator::core_free_pool(out_ptr as *mut core::ffi::c_void) }.is_ok()); }); } @@ -1508,8 +1509,8 @@ mod tests { let out_slice = unsafe { core::slice::from_raw_parts(out_ptr, out_count) }; assert_eq!(out_slice, expected.as_slice()); - // cleanup - assert!(crate::allocator::core_free_pool(out_ptr as *mut core::ffi::c_void).is_ok()); + // SAFETY: `out_ptr` was allocated by `get_memory_space_map` and is valid pool memory. + assert!(unsafe { crate::allocator::core_free_pool(out_ptr as *mut core::ffi::c_void) }.is_ok()); }); } @@ -1565,7 +1566,8 @@ mod tests { let out_slice = unsafe { core::slice::from_raw_parts(out_ptr, out_count) }; assert_eq!(out_slice, expected.as_slice()); - assert!(crate::allocator::core_free_pool(out_ptr as *mut core::ffi::c_void).is_ok()); + // SAFETY: `out_ptr` was allocated by `get_io_space_map` and is valid pool memory. + assert!(unsafe { crate::allocator::core_free_pool(out_ptr as *mut core::ffi::c_void) }.is_ok()); }); } @@ -1596,7 +1598,8 @@ mod tests { let out_slice = unsafe { core::slice::from_raw_parts(out_ptr, out_count) }; assert_eq!(out_slice, expected.as_slice()); - assert!(crate::allocator::core_free_pool(out_ptr as *mut core::ffi::c_void).is_ok()); + // SAFETY: `out_ptr` was allocated by `get_io_space_map` and is valid pool memory. + assert!(unsafe { crate::allocator::core_free_pool(out_ptr as *mut core::ffi::c_void) }.is_ok()); }); } diff --git a/patina_dxe_core/src/event_db.rs b/patina_dxe_core/src/event_db.rs index e324df0e7..26d0cedcd 100644 --- a/patina_dxe_core/src/event_db.rs +++ b/patina_dxe_core/src/event_db.rs @@ -308,6 +308,11 @@ impl EventDb { EventDb { events: BTreeMap::new(), next_event_id: 1, pending_notifies: BTreeSet::new(), notify_tags: 0 } } + /// Creates a new event and inserts it into the event database. + /// + /// This is a safe, pure Rust implementation that does not dereference any + /// user provided pointer inputs. The `notify_function`, `notify_context`, + /// and `event_group` values are stored as-is without being dereferenced. fn create_event( &mut self, event_type: u32, @@ -346,6 +351,12 @@ impl EventDb { Ok(id as efi::Event) } + /// Closes (removes) an event from the event database. + /// + /// This function is safe to call with event value coming from arbitrary + /// sources. It checks whether the given event ID is present in the database + /// (or runtime event table) and returns `Err(EfiError::InvalidParameter)` + /// if it is not found. No unsafe operations are performed. fn close_event(&mut self, event: efi::Event) -> Result<(), EfiError> { let id = event as usize; if (id & Self::RT_EVENT) != 0 { @@ -372,6 +383,12 @@ impl EventDb { } } + /// Signals an event in the event database. + /// + /// This function is safe to call with event value coming from arbitrary + /// sources. It checks whether the given event ID is present in the database + /// and returns `Err(EfiError::InvalidParameter)` if it is not found. + /// No unsafe operations are performed. fn signal_event(&mut self, event: efi::Event) -> Result<(), EfiError> { let id = event as usize; let current_event = self.events.get_mut(&id).ok_or(EfiError::InvalidParameter)?; @@ -451,6 +468,12 @@ impl EventDb { } } + /// Sets a timer on an event in the event database. + /// + /// This function is safe to call with event value coming from arbitrary + /// sources. It checks whether the given event ID is present in the database + /// and returns `Err(EfiError::InvalidParameter)` if it is not found. + /// No unsafe operations are performed. fn set_timer( &mut self, event: efi::Event, diff --git a/patina_dxe_core/src/events.rs b/patina_dxe_core/src/events.rs index 6362cfbaf..7c12fabca 100644 --- a/patina_dxe_core/src/events.rs +++ b/patina_dxe_core/src/events.rs @@ -29,7 +29,11 @@ pub static EVENT_DB: SpinLockedEventDb = SpinLockedEventDb::new(); static CURRENT_TPL: AtomicUsize = AtomicUsize::new(efi::TPL_APPLICATION); static SYSTEM_TIME: AtomicU64 = AtomicU64::new(0); -extern "efiapi" fn create_event( +/// # Safety +/// +/// Caller must ensure that `event` points to valid writable memory. Only a null +/// check is performed here; no other validation of the pointer is done. +unsafe extern "efiapi" fn create_event( event_type: u32, notify_tpl: efi::Tpl, notify_function: Option, @@ -60,7 +64,11 @@ extern "efiapi" fn create_event( } } -extern "efiapi" fn create_event_ex( +/// # Safety +/// +/// Caller must ensure that `event` points to valid writable memory. Only a null +/// check is performed here; no other validation of the pointer is done. +unsafe extern "efiapi" fn create_event_ex( event_type: u32, notify_tpl: efi::Tpl, notify_function: Option, @@ -94,6 +102,12 @@ extern "efiapi" fn create_event_ex( } } +/// Closes an event from the event database. +/// +/// This function is safe to call with an unverified `event` parameter. The +/// underlying implementation looks up the event by ID in the database and +/// returns a failure status if it is not found. No incoming pointers are +/// dereferenced, so this is not marked `unsafe`. pub extern "efiapi" fn close_event(event: efi::Event) -> efi::Status { match EVENT_DB.close_event(event) { Ok(()) => efi::Status::SUCCESS, @@ -101,17 +115,31 @@ pub extern "efiapi" fn close_event(event: efi::Event) -> efi::Status { } } +/// Signals an event in the event database. +/// +/// This function is safe to call with an unverified `event` parameter. The +/// underlying implementation looks up the event by ID in the database and +/// returns a failure status if it is not found. No incoming pointers are +/// dereferenced, so this is not marked `unsafe`. pub extern "efiapi" fn signal_event(event: efi::Event) -> efi::Status { - //Note: The C-reference implementation of SignalEvent gets an immediate dispatch of - //pending events as a side effect of the locking implementation calling raise/restore - //TPL. This will occur when the event lock is dropped at the end of signal_event(). + // Note: The C-reference implementation of SignalEvent gets an immediate dispatch of + // pending events as a side effect of the locking implementation calling raise/restore + // TPL. This will occur when the event lock is dropped at the end of signal_event(). match EVENT_DB.signal_event(event) { Ok(()) => efi::Status::SUCCESS, Err(err) => err.into(), } } -extern "efiapi" fn wait_for_event( +/// # Safety +/// +/// Caller must ensure that `event_array` points to a valid array of at least +/// `number_of_events` `efi::Event` entries. `number_of_events` must not exceed +/// the actual length of the array; otherwise out-of-bounds reads will occur. If +/// `out_index` is non-null, it must point to valid writable memory. Only a null +/// check is performed on `event_array`; no other validation of either pointer +/// is done. +unsafe extern "efiapi" fn wait_for_event( number_of_events: usize, event_array: *mut efi::Event, out_index: *mut usize, @@ -156,6 +184,12 @@ extern "efiapi" fn wait_for_event( } } +/// Checks whether an event is in the signaled state. +/// +/// This function is safe to call with an unverified `event` parameter. The +/// underlying implementation looks up the event by ID in the database and +/// returns a failure status if it is not found. No incoming pointers are +/// dereferenced, so this is not marked `unsafe`. pub extern "efiapi" fn check_event(event: efi::Event) -> efi::Status { let event_type = match EVENT_DB.get_event_type(event) { Ok(event_type) => event_type, @@ -196,6 +230,12 @@ pub extern "efiapi" fn check_event(event: efi::Event) -> efi::Status { efi::Status::NOT_READY } +/// Sets a timer on an event in the event database. +/// +/// This function is safe to call with an unverified `event` parameter. The +/// underlying implementation looks up the event by ID in the database and +/// returns a failure status if it is not found. No incoming pointers are +/// dereferenced, so this is not marked `unsafe`. pub extern "efiapi" fn set_timer(event: efi::Event, timer_type: efi::TimerDelay, trigger_time: u64) -> efi::Status { let timer_type = match TimerDelay::try_from(timer_type) { Err(err) => return err, @@ -214,6 +254,12 @@ pub extern "efiapi" fn set_timer(event: efi::Event, timer_type: efi::TimerDelay, } } +/// Raises the task priority level (TPL) to the specified value. +/// +/// This function is safe to call with any `new_tpl` value. It validates that +/// the requested TPL does not exceed `TPL_HIGH_LEVEL` and is not lower than the +/// current TPL, panicking on violation. No incoming parameters are dereferenced, +/// so this is not marked `unsafe`. pub extern "efiapi" fn raise_tpl(new_tpl: efi::Tpl) -> efi::Tpl { if new_tpl > efi::TPL_HIGH_LEVEL { panic!("Invalid attempt to raise TPL above TPL_HIGH_LEVEL: {new_tpl:#x?}"); @@ -231,6 +277,12 @@ pub extern "efiapi" fn raise_tpl(new_tpl: efi::Tpl) -> efi::Tpl { prev_tpl } +/// Restores the task priority level (TPL) to the specified value. +/// +/// This function is safe to call with any `new_tpl` value. It validates that +/// the requested TPL is not higher than the current TPL, panicking on violation. +/// Pending event notifications at higher TPLs are dispatched before returning. +/// No incoming parameters are dereferenced, so this is not marked `unsafe`. pub extern "efiapi" fn restore_tpl(new_tpl: efi::Tpl) { let prev_tpl = CURRENT_TPL.fetch_min(new_tpl, Ordering::SeqCst); @@ -274,14 +326,10 @@ pub extern "efiapi" fn restore_tpl(new_tpl: efi::Tpl) { let _ = EVENT_DB.clear_signal(event.event); } - //Caution: this is calling function pointer supplied by code outside DXE Rust. - //The notify_function is not "unsafe" per the signature, even though it's - //supplied by code outside the core module. If it were marked 'unsafe' - //then other Rust modules executing under DXE Rust would need to mark all event - //callbacks as "unsafe", and the r_efi definition for EventNotify would need to - //change. if let Some(notify_function) = event.notify_function { - (notify_function)(event.event, notify_context); + // SAFETY: Marking the call as unsafe as per r-efi definition because the function + // pointer could point to arbitrary code outside the control of DXE Rust. + unsafe { notify_function(event.event, notify_context) }; } } } @@ -394,7 +442,8 @@ mod tests { #[test] fn test_create_event_null_event_pointer() { with_locked_state(|| { - let result = create_event(0, efi::TPL_APPLICATION, None, ptr::null_mut(), ptr::null_mut()); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { create_event(0, efi::TPL_APPLICATION, None, ptr::null_mut(), ptr::null_mut()) }; assert_eq!(result, efi::Status::INVALID_PARAMETER); }); @@ -404,7 +453,8 @@ mod tests { fn test_create_event_success() { with_locked_state(|| { let mut event: efi::Event = ptr::null_mut(); - let result = create_event(0, efi::TPL_APPLICATION, None, ptr::null_mut(), &mut event); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { create_event(0, efi::TPL_APPLICATION, None, ptr::null_mut(), &mut event) }; assert_eq!(result, efi::Status::SUCCESS); }); @@ -415,7 +465,8 @@ mod tests { with_locked_state(|| { let mut event: efi::Event = ptr::null_mut(); let context = Box::into_raw(Box::new(42)) as *mut c_void; - let result = create_event(0, efi::TPL_APPLICATION, None, context, &mut event); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { create_event(0, efi::TPL_APPLICATION, None, context, &mut event) }; assert_eq!(result, efi::Status::SUCCESS); }); @@ -426,7 +477,10 @@ mod tests { with_locked_state(|| { let mut event: efi::Event = ptr::null_mut(); let notify_fn: Option = Some(test_notify); - let result = create_event(efi::EVT_NOTIFY_WAIT, efi::TPL_CALLBACK, notify_fn, ptr::null_mut(), &mut event); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event(efi::EVT_NOTIFY_WAIT, efi::TPL_CALLBACK, notify_fn, ptr::null_mut(), &mut event) + }; assert_eq!(result, efi::Status::SUCCESS); }); @@ -439,13 +493,16 @@ mod tests { let notify_fn: Option = Some(test_notify); - let result = create_event( - efi::EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, - efi::TPL_CALLBACK, - notify_fn, - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event( + efi::EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, + efi::TPL_CALLBACK, + notify_fn, + ptr::null_mut(), + &mut event, + ) + }; assert_eq!(result, efi::Status::SUCCESS); }); @@ -458,13 +515,16 @@ mod tests { let notify_fn: Option = Some(test_notify); - let result = create_event( - efi::EVT_SIGNAL_EXIT_BOOT_SERVICES, - efi::TPL_CALLBACK, - notify_fn, - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event( + efi::EVT_SIGNAL_EXIT_BOOT_SERVICES, + efi::TPL_CALLBACK, + notify_fn, + ptr::null_mut(), + &mut event, + ) + }; assert_eq!(result, efi::Status::SUCCESS); }); @@ -473,7 +533,9 @@ mod tests { #[test] fn test_create_event_ex_null_event() { with_locked_state(|| { - let result = create_event_ex(0, efi::TPL_APPLICATION, None, ptr::null(), ptr::null(), ptr::null_mut()); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = + unsafe { create_event_ex(0, efi::TPL_APPLICATION, None, ptr::null(), ptr::null(), ptr::null_mut()) }; assert_eq!(result, efi::Status::INVALID_PARAMETER); }); @@ -485,14 +547,17 @@ mod tests { let mut event: efi::Event = ptr::null_mut(); let event_guid: efi::Guid = patina::BinaryGuid::from_string("87A2E5D9-C34F-4B21-8E57-1AF93C82D76B").into(); let notify_fn: Option = Some(test_notify); - let result = create_event_ex( - efi::EVT_NOTIFY_SIGNAL, - efi::TPL_CALLBACK, - notify_fn, - ptr::null(), - &event_guid, - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event_ex( + efi::EVT_NOTIFY_SIGNAL, + efi::TPL_CALLBACK, + notify_fn, + ptr::null(), + &event_guid, + &mut event, + ) + }; assert_eq!(result, efi::Status::SUCCESS); }); @@ -503,14 +568,17 @@ mod tests { with_locked_state(|| { let mut event: efi::Event = ptr::null_mut(); // EVT_SIGNAL_EXIT_BOOT_SERVICES should fail with create_event_ex - let result = create_event_ex( - efi::EVT_SIGNAL_EXIT_BOOT_SERVICES, - efi::TPL_CALLBACK, - Some(test_notify), - ptr::null(), - ptr::null(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event_ex( + efi::EVT_SIGNAL_EXIT_BOOT_SERVICES, + efi::TPL_CALLBACK, + Some(test_notify), + ptr::null(), + ptr::null(), + &mut event, + ) + }; assert_eq!(result, efi::Status::INVALID_PARAMETER); }); @@ -521,14 +589,17 @@ mod tests { with_locked_state(|| { let mut event: efi::Event = ptr::null_mut(); // EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE should fail with create_event_ex - let result = create_event_ex( - efi::EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, - efi::TPL_CALLBACK, - Some(test_notify), - ptr::null(), - ptr::null(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event_ex( + efi::EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, + efi::TPL_CALLBACK, + Some(test_notify), + ptr::null(), + ptr::null(), + &mut event, + ) + }; assert_eq!(result, efi::Status::INVALID_PARAMETER); }); @@ -539,13 +610,16 @@ mod tests { with_locked_state(|| { let mut event: efi::Event = ptr::null_mut(); let notify_fn: Option = Some(test_notify); - let _ = create_event( - efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, - efi::TPL_NOTIFY, - notify_fn, - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let _ = unsafe { + create_event( + efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, + efi::TPL_NOTIFY, + notify_fn, + ptr::null_mut(), + &mut event, + ) + }; let result = EVENT_DB.close_event(event); @@ -559,13 +633,16 @@ mod tests { with_locked_state(|| { let mut event: efi::Event = ptr::null_mut(); let notify_fn: Option = Some(test_notify); - let _ = create_event( - efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, - efi::TPL_NOTIFY, - notify_fn, - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let _ = unsafe { + create_event( + efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, + efi::TPL_NOTIFY, + notify_fn, + ptr::null_mut(), + &mut event, + ) + }; let result = signal_event(event); assert_eq!(result, efi::Status::SUCCESS); @@ -578,14 +655,18 @@ mod tests { with_locked_state(|| { CURRENT_TPL.store(efi::TPL_APPLICATION, Ordering::SeqCst); let mut event: efi::Event = ptr::null_mut(); - create_event(efi::EVT_NOTIFY_WAIT, efi::TPL_NOTIFY, Some(test_notify), ptr::null_mut(), &mut event); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + unsafe { + create_event(efi::EVT_NOTIFY_WAIT, efi::TPL_NOTIFY, Some(test_notify), ptr::null_mut(), &mut event) + }; signal_event(event); let events: [efi::Event; 1] = [event]; let mut index: usize = 0; let mut test_wait = || { - let status = wait_for_event(1, events.as_ptr() as *mut efi::Event, &mut index as *mut usize); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let status = unsafe { wait_for_event(1, events.as_ptr() as *mut efi::Event, &mut index as *mut usize) }; assert_eq!(status, efi::Status::SUCCESS); assert_eq!(index, 0); }; @@ -602,13 +683,16 @@ mod tests { let mut event: efi::Event = ptr::null_mut(); let notify_fn: Option = Some(test_notify); - let result = create_event( - efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, - efi::TPL_NOTIFY, - notify_fn, - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event( + efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, + efi::TPL_NOTIFY, + notify_fn, + ptr::null_mut(), + &mut event, + ) + }; assert_eq!(result, efi::Status::SUCCESS); let initial_time = 1000u64; @@ -635,13 +719,16 @@ mod tests { let notify_fn: Option = Some(test_notify); // Create timer event - let result = create_event( - efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, - efi::TPL_NOTIFY, - notify_fn, - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event( + efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, + efi::TPL_NOTIFY, + notify_fn, + ptr::null_mut(), + &mut event, + ) + }; assert_eq!(result, efi::Status::SUCCESS); // Set timer with an invalid timer type @@ -661,13 +748,16 @@ mod tests { let mut event: efi::Event = ptr::null_mut(); let notify_fn: Option = Some(test_notify); - let result = create_event( - efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, - efi::TPL_NOTIFY, - notify_fn, - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event( + efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, + efi::TPL_NOTIFY, + notify_fn, + ptr::null_mut(), + &mut event, + ) + }; assert_eq!(result, efi::Status::SUCCESS); // Set a timer @@ -689,13 +779,16 @@ mod tests { let mut event: efi::Event = ptr::null_mut(); let notify_fn: Option = Some(test_notify); - let result = create_event( - efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, - efi::TPL_NOTIFY, - notify_fn, - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event( + efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, + efi::TPL_NOTIFY, + notify_fn, + ptr::null_mut(), + &mut event, + ) + }; assert_eq!(result, efi::Status::SUCCESS); // Set periodic timer @@ -717,13 +810,16 @@ mod tests { let mut event: efi::Event = ptr::null_mut(); // Create notification signal event - let result = create_event( - efi::EVT_NOTIFY_SIGNAL, - efi::TPL_CALLBACK, - Some(tracking_notify), - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event( + efi::EVT_NOTIFY_SIGNAL, + efi::TPL_CALLBACK, + Some(tracking_notify), + ptr::null_mut(), + &mut event, + ) + }; assert_eq!(result, efi::Status::SUCCESS); // Signal the event @@ -755,23 +851,29 @@ mod tests { let mut event: efi::Event = ptr::null_mut(); // Create notification signal event - let result = create_event( - efi::EVT_NOTIFY_SIGNAL, - efi::TPL_CALLBACK, - Some(tracking_notify), - ptr::null_mut(), - &mut event, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event( + efi::EVT_NOTIFY_SIGNAL, + efi::TPL_CALLBACK, + Some(tracking_notify), + ptr::null_mut(), + &mut event, + ) + }; assert_eq!(result, efi::Status::SUCCESS); let mut event2: efi::Event = ptr::null_mut(); - let result = create_event( - efi::EVT_NOTIFY_SIGNAL, - efi::TPL_NOTIFY, - Some(test_tpl_switching_notify), - ptr::null_mut(), - &mut event2, - ); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event( + efi::EVT_NOTIFY_SIGNAL, + efi::TPL_NOTIFY, + Some(test_tpl_switching_notify), + ptr::null_mut(), + &mut event2, + ) + }; assert_eq!(result, efi::Status::SUCCESS); //raise TPL to callback than event @@ -808,11 +910,13 @@ mod tests { let events: [efi::Event; 1] = [ptr::null_mut()]; // Test null event array - let status = wait_for_event(1, ptr::null_mut(), &mut index as *mut usize); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let status = unsafe { wait_for_event(1, ptr::null_mut(), &mut index as *mut usize) }; assert_eq!(status, efi::Status::INVALID_PARAMETER); // Test zero events - let status = wait_for_event(0, events.as_ptr() as *mut efi::Event, &mut index as *mut usize); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let status = unsafe { wait_for_event(0, events.as_ptr() as *mut efi::Event, &mut index as *mut usize) }; assert_eq!(status, efi::Status::INVALID_PARAMETER); }); } @@ -826,7 +930,8 @@ mod tests { // Set TPL to something other than APPLICATION CURRENT_TPL.store(efi::TPL_NOTIFY, Ordering::SeqCst); - let status = wait_for_event(1, events.as_ptr() as *mut efi::Event, &mut index as *mut usize); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let status = unsafe { wait_for_event(1, events.as_ptr() as *mut efi::Event, &mut index as *mut usize) }; assert_eq!(status, efi::Status::UNSUPPORTED); CURRENT_TPL.store(efi::TPL_APPLICATION, Ordering::SeqCst); @@ -848,8 +953,10 @@ mod tests { with_locked_state(|| { let mut event: efi::Event = ptr::null_mut(); // Create a notification signal event - let result = - create_event(efi::EVT_NOTIFY_SIGNAL, efi::TPL_NOTIFY, Some(test_notify), ptr::null_mut(), &mut event); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event(efi::EVT_NOTIFY_SIGNAL, efi::TPL_NOTIFY, Some(test_notify), ptr::null_mut(), &mut event) + }; assert_eq!(result, efi::Status::SUCCESS); // Check event should fail for notify signal events @@ -866,8 +973,10 @@ mod tests { with_locked_state(|| { let mut event: efi::Event = ptr::null_mut(); // Create a wait event - let result = - create_event(efi::EVT_NOTIFY_WAIT, efi::TPL_NOTIFY, Some(test_notify), ptr::null_mut(), &mut event); + // SAFETY: Test code - all pointers are test-controlled and valid for the duration of the call. + let result = unsafe { + create_event(efi::EVT_NOTIFY_WAIT, efi::TPL_NOTIFY, Some(test_notify), ptr::null_mut(), &mut event) + }; assert_eq!(result, efi::Status::SUCCESS); // Signal the event diff --git a/patina_dxe_core/src/filesystems.rs b/patina_dxe_core/src/filesystems.rs index bb9d7bc06..dfd2f9dcd 100644 --- a/patina_dxe_core/src/filesystems.rs +++ b/patina_dxe_core/src/filesystems.rs @@ -22,13 +22,18 @@ impl SimpleFile<'_> { /// Opens the given filename with appropriate mode/attributes and returns a new instance of SimpleFile for it. pub fn open(&mut self, filename: Vec, mode: u64, attributes: u64) -> Result { let mut file_ptr = core::ptr::null_mut(); - let status = (self.file.open)( - self.file, - core::ptr::addr_of_mut!(file_ptr), - filename.as_ptr() as *mut u16, - mode, - attributes, - ); + // SAFETY: self.file is a valid pointer to a file protocol instance + // obtained from the protocol database during the construction of this + // SimpleFile instance. + let status = unsafe { + (self.file.open)( + self.file, + core::ptr::addr_of_mut!(file_ptr), + filename.as_ptr() as *mut u16, + mode, + attributes, + ) + }; EfiError::status_to_result(status)?; @@ -49,7 +54,9 @@ impl SimpleFile<'_> { }; let mut file_system_ptr = core::ptr::null_mut(); - let status = (sfs.open_volume)(sfs, core::ptr::addr_of_mut!(file_system_ptr)); + // SAFETY: sfs is a valid pointer to a simple file system protocol instance + // obtained from the protocol database above. + let status = unsafe { (sfs.open_volume)(sfs, core::ptr::addr_of_mut!(file_system_ptr)) }; EfiError::status_to_result(status)?; // SAFETY: file_system_ptr is filled by the open_volume call above on success. @@ -61,12 +68,17 @@ impl SimpleFile<'_> { // returns a byte buffer containing the file info for this SimpleFile instance. fn get_info(&mut self) -> Result, EfiError> { let mut info_size = 0; - let status = (self.file.get_info)( - self.file, - &efi::protocols::file::INFO_ID as *const efi::Guid as *mut efi::Guid, - core::ptr::addr_of_mut!(info_size), - core::ptr::null_mut(), - ); + // SAFETY: self.file is a valid pointer to a file protocol instance + // obtained from the protocol database during the construction of this + // SimpleFile instance. + let status = unsafe { + (self.file.get_info)( + self.file, + &efi::protocols::file::INFO_ID as *const efi::Guid as *mut efi::Guid, + core::ptr::addr_of_mut!(info_size), + core::ptr::null_mut(), + ) + }; match status { efi::Status::BUFFER_TOO_SMALL => (), // expected efi::Status::SUCCESS => Err(EfiError::DeviceError)?, // unexpected success. @@ -74,12 +86,17 @@ impl SimpleFile<'_> { } let mut file_info_buffer = vec![0u8; info_size]; - let status = (self.file.get_info)( - self.file, - &efi::protocols::file::INFO_ID as *const efi::Guid as *mut efi::Guid, - core::ptr::addr_of_mut!(info_size), - file_info_buffer.as_mut_ptr() as *mut c_void, - ); + // SAFETY: self.file is a valid pointer to a file protocol instance + // obtained from the protocol database during the construction of this + // SimpleFile instance. + let status = unsafe { + (self.file.get_info)( + self.file, + &efi::protocols::file::INFO_ID as *const efi::Guid as *mut efi::Guid, + core::ptr::addr_of_mut!(info_size), + file_info_buffer.as_mut_ptr() as *mut c_void, + ) + }; EfiError::status_to_result(status).map(|_| file_info_buffer) } @@ -117,12 +134,18 @@ impl SimpleFile<'_> { let mut file_size = self.get_size()? as usize; let mut file_buffer = vec![0u8; file_size]; - - let status = (self.file.set_position)(self.file, 0); + // SAFETY: self.file is a valid pointer to a file protocol instance + // obtained from the protocol database during the construction of this + // SimpleFile instance. + let status = unsafe { (self.file.set_position)(self.file, 0) }; EfiError::status_to_result(status)?; - let status = - (self.file.read)(self.file, core::ptr::addr_of_mut!(file_size), file_buffer.as_mut_ptr() as *mut c_void); + // SAFETY: self.file is a valid pointer to a file protocol instance + // obtained from the protocol database during the construction of this + // SimpleFile instance. + let status = unsafe { + (self.file.read)(self.file, core::ptr::addr_of_mut!(file_size), file_buffer.as_mut_ptr() as *mut c_void) + }; EfiError::status_to_result(status)?; diff --git a/patina_dxe_core/src/gcd.rs b/patina_dxe_core/src/gcd.rs index a6c23dcb5..74b9195c2 100644 --- a/patina_dxe_core/src/gcd.rs +++ b/patina_dxe_core/src/gcd.rs @@ -1055,13 +1055,12 @@ mod tests { assert_eq!(policy.memory_allocation_default_attributes.get(), efi::MEMORY_XP); // allocate some loader code/data memory to test later - let mut loader_code_mem = 0; - let mut loader_data_mem = 0; - - allocator::core_allocate_pages(efi::ALLOCATE_ANY_PAGES, efi::LOADER_CODE, 2, &mut loader_code_mem, None) - .expect("Failed to allocate loader code memory"); - allocator::core_allocate_pages(efi::ALLOCATE_ANY_PAGES, efi::LOADER_DATA, 2, &mut loader_data_mem, None) - .expect("Failed to allocate loader data memory"); + let _loader_code_mem = + allocator::core_allocate_pages(efi::ALLOCATE_ANY_PAGES, efi::LOADER_CODE, 2, 0, None) + .expect("Failed to allocate loader code memory"); + let _loader_data_mem = + allocator::core_allocate_pages(efi::ALLOCATE_ANY_PAGES, efi::LOADER_DATA, 2, 0, None) + .expect("Failed to allocate loader data memory"); // loader code/data should be XP by default let loader_code_ranges = allocator::get_memory_ranges_for_memory_type(efi::LOADER_CODE); @@ -1079,15 +1078,9 @@ mod tests { } } - let mut image_base_page = 0; - allocator::core_allocate_pages( - efi::ALLOCATE_ANY_PAGES, - efi::BOOT_SERVICES_DATA, - 4, - &mut image_base_page, - None, - ) - .expect("Failed to allocate loader code memory"); + let image_base_page = + allocator::core_allocate_pages(efi::ALLOCATE_ANY_PAGES, efi::BOOT_SERVICES_DATA, 4, 0, None) + .expect("Failed to allocate loader code memory"); let image_num_pages = 4; let filename = "legacy_app.efi"; diff --git a/patina_dxe_core/src/memory_manager.rs b/patina_dxe_core/src/memory_manager.rs index ea1755624..01d9d11e0 100644 --- a/patina_dxe_core/src/memory_manager.rs +++ b/patina_dxe_core/src/memory_manager.rs @@ -60,11 +60,8 @@ impl MemoryManager for CoreMemoryManager { } }; - let result = - core_allocate_pages(alloc_type, options.memory_type().into(), page_count, &mut address, Some(alignment)); - - match result { - Ok(_) => { + match core_allocate_pages(alloc_type, options.memory_type().into(), page_count, address, Some(alignment)) { + Ok(address) => { // SAFETY: address/page_count come from a successful core_allocate_pages call. let allocation = unsafe { PageAllocation::new(address as usize, page_count, &CoreMemoryManager) @@ -83,7 +80,8 @@ impl MemoryManager for CoreMemoryManager { /// Caller must ensure that the given address corresponds to a valid block of pages that was allocated with /// [Self::allocate_pages]. unsafe fn free_pages(&self, address: usize, page_count: usize) -> Result<(), MemoryError> { - let result = core_free_pages(address as efi::PhysicalAddress, page_count); + // SAFETY: The caller must ensure that the provided address is valid. + let result = unsafe { core_free_pages(address as efi::PhysicalAddress, page_count) }; match result { Ok(_) => Ok(()), Err(EfiError::NotFound) => Err(MemoryError::InvalidAddress), @@ -100,6 +98,12 @@ impl MemoryManager for CoreMemoryManager { Ok(allocator as &dyn core::alloc::Allocator) } + /// # Safety + /// + /// Changing the attributes of a page of memory can result in undefined behavior + /// if the attributes are not correct for the memory usage. The caller is responsible + /// for understanding the use of the memory and verifying that all current and + /// future accesses of the memory align to the attributes configured. unsafe fn set_page_attributes( &self, address: usize, @@ -231,7 +235,9 @@ fn memory_manager_allocations_test(mm: Service) -> patina_tes // SAFETY: address was returned by allocate_pages for this manager. let result = unsafe { mm.free_pages(address, 1) }; u_assert!(result.is_ok(), "Failed to free page."); - let result = mm.allocate_pages(1, AllocationOptions::new().with_strategy(PageAllocationStrategy::Address(address))); + // SAFETY: address was previously allocated and freed by this manager, making it a valid target address. + let options = unsafe { AllocationOptions::new().with_strategy(PageAllocationStrategy::Address(address)) }; + let result = mm.allocate_pages(1, options); u_assert!(result.is_ok(), "Failed to allocate page by address"); u_assert_eq!(result.unwrap().into_raw_ptr::().unwrap() as usize, address, "Failed to allocate correct address"); @@ -249,8 +255,10 @@ fn memory_manager_allocations_test(mm: Service) -> patina_tes // Allocate with a max address limit. let max_address = 0x100_8000_0000; - let result = - mm.allocate_pages(1, AllocationOptions::new().with_strategy(PageAllocationStrategy::MaxAddress(max_address))); + // SAFETY: PageAllocationStrategy::MaxAddress constrains the upper bound only; the allocator + // chooses the actual address, so no caller-provided address is dereferenced. + let options = unsafe { AllocationOptions::new().with_strategy(PageAllocationStrategy::MaxAddress(max_address)) }; + let result = mm.allocate_pages(1, options); u_assert!(result.is_ok(), "Failed to allocate with max address limit."); let allocation = result.unwrap(); let address = allocation.into_raw_ptr::().unwrap() as usize; diff --git a/patina_dxe_core/src/misc_boot_services.rs b/patina_dxe_core/src/misc_boot_services.rs index 464140856..1ae49427c 100644 --- a/patina_dxe_core/src/misc_boot_services.rs +++ b/patina_dxe_core/src/misc_boot_services.rs @@ -62,7 +62,15 @@ static WATCHDOG_ARCH_PTR: ArchProtocolPtr = ArchP // { 0x5f1d7e16, 0x784a, 0x4da2, { 0xb0, 0x84, 0xf8, 0x12, 0xf2, 0x3a, 0x8d, 0xce }} pub const PRE_EBS_GUID: patina::BinaryGuid = patina::BinaryGuid::from_string("5F1D7E16-784A-4DA2-B084-F812F23A8DCE"); // TODO [END]: LOCAL (TEMP) GUID DEFINITIONS (MOVE LATER) -extern "efiapi" fn calculate_crc32(data: *mut c_void, data_size: usize, crc_32: *mut u32) -> efi::Status { + +/// Calculates the CRC32 of the given data buffer. +/// +/// # Safety +/// +/// The caller is responsible for ensuring that `data` points to valid, readable memory of at least +/// `data_size` bytes and that `crc_32` points to valid, writable memory. Both pointers are +/// null-checked, but validity of the referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn calculate_crc32(data: *mut c_void, data_size: usize, crc_32: *mut u32) -> efi::Status { if data.is_null() || data_size == 0 || crc_32.is_null() { return efi::Status::INVALID_PARAMETER; } @@ -172,6 +180,16 @@ extern "efiapi" fn watchdog_arch_available(event: efi::Event, _context: *mut c_v } } +/// Terminates all boot services. +/// +/// # Safety Considerations +/// +/// This function is not marked `unsafe` because both parameters are safe to accept from unverified +/// sources: +/// - `_handle` is currently unused and has no effect on execution. +/// - `map_key` is validated against the current memory map key in [`terminate_memory_map`]; an +/// invalid key causes the call to fail gracefully with an error status rather than undefined +/// behavior. pub extern "efiapi" fn exit_boot_services(_handle: efi::Handle, map_key: usize) -> efi::Status { static EXIT_BOOT_SERVICES_CALLED: Once<()> = Once::new(); @@ -349,11 +367,14 @@ mod tests { let mut data_crc: u32 = 0; // Test case 1: Valid parameters - successful CRC32 calculation - let status = (st.boot_services().get().calculate_crc32)( - BUFFER.as_ptr() as *mut c_void, - BUFFER.len(), - &mut data_crc as *mut u32, - ); + // SAFETY: The passed in values are safe because they are constructed in this test case. + let status = unsafe { + (st.boot_services().get().calculate_crc32)( + BUFFER.as_ptr() as *mut c_void, + BUFFER.len(), + &mut data_crc as *mut u32, + ) + }; // Verify the function succeeded and CRC32 was calculated correctly for zero buffer if status == efi::Status::SUCCESS { let expected_crc = crc32fast::hash(&BUFFER); @@ -367,11 +388,10 @@ mod tests { } // Test case 2: Zero data size - should return INVALID_PARAMETER - let status = (st.boot_services().get().calculate_crc32)( - BUFFER.as_ptr() as *mut c_void, - 0, - &mut data_crc as *mut u32, - ); + // SAFETY: The passed in values are safe because they are constructed in this test case. + let status = unsafe { + (st.boot_services().get().calculate_crc32)(BUFFER.as_ptr() as *mut c_void, 0, &mut data_crc as *mut u32) + }; if status == efi::Status::INVALID_PARAMETER { log::debug!("Zero data size correctly returned INVALID_PARAMETER"); } else { @@ -379,11 +399,14 @@ mod tests { } // Test case 3: Null data pointer - should return INVALID_PARAMETER - let status = (st.boot_services().get().calculate_crc32)( - core::ptr::null_mut(), - BUFFER.len(), - &mut data_crc as *mut u32, - ); + // SAFETY: The passed in values are safe because they are constructed in this test case. + let status = unsafe { + (st.boot_services().get().calculate_crc32)( + core::ptr::null_mut(), + BUFFER.len(), + &mut data_crc as *mut u32, + ) + }; if status == efi::Status::INVALID_PARAMETER { log::debug!("Null data pointer correctly returned INVALID_PARAMETER"); } else { @@ -391,11 +414,14 @@ mod tests { } // Test case 4: Null output pointer - should return INVALID_PARAMETER - let status = (st.boot_services().get().calculate_crc32)( - BUFFER.as_ptr() as *mut c_void, - BUFFER.len(), - core::ptr::null_mut(), - ); + // SAFETY: The passed in values are safe because they are constructed in this test case. + let status = unsafe { + (st.boot_services().get().calculate_crc32)( + BUFFER.as_ptr() as *mut c_void, + BUFFER.len(), + core::ptr::null_mut(), + ) + }; if status == efi::Status::INVALID_PARAMETER { log::debug!("Null output pointer correctly returned INVALID_PARAMETER"); } else { @@ -409,7 +435,9 @@ mod tests { init_misc_boot_services_support(st); // Test case 1: Set watchdog timer with null data - should return NOT_READY (no watchdog available in test) - let status = (st.boot_services().get().set_watchdog_timer)(300, 0, 0, ptr::null_mut()); + // SAFETY: The unsafe block is required because r-efi declares set_watchdog_timer as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { (st.boot_services().get().set_watchdog_timer)(300, 0, 0, ptr::null_mut()) }; if status == efi::Status::NOT_READY { log::debug!("Set watchdog timer correctly returned NOT_READY (no watchdog protocol)"); } else { @@ -417,7 +445,9 @@ mod tests { } // Test case 2: Disable watchdog timer with null data - should return NOT_READY - let status = (st.boot_services().get().set_watchdog_timer)(0, 0, 0, ptr::null_mut()); + // SAFETY: The unsafe block is required because r-efi declares set_watchdog_timer as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { (st.boot_services().get().set_watchdog_timer)(0, 0, 0, ptr::null_mut()) }; if status == efi::Status::NOT_READY { log::debug!("Disable watchdog timer correctly returned NOT_READY"); } else { @@ -428,7 +458,9 @@ mod tests { let data_ptr = data.as_ptr() as *mut efi::Char16; // Test case 3: Set the watchdog timer with non-null data - should return NOT_READY - let status = (st.boot_services().get().set_watchdog_timer)(300, 0, data.len(), data_ptr); + // SAFETY: The unsafe block is required because r-efi declares set_watchdog_timer as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { (st.boot_services().get().set_watchdog_timer)(300, 0, data.len(), data_ptr) }; if status == efi::Status::NOT_READY { log::debug!("Set watchdog timer with data correctly returned NOT_READY"); } else { @@ -436,7 +468,9 @@ mod tests { } // Test case 4: Disable the watchdog timer with non-null data - should return NOT_READY - let status = (st.boot_services().get().set_watchdog_timer)(0, 0, data.len(), data_ptr); + // SAFETY: The unsafe block is required because r-efi declares set_watchdog_timer as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { (st.boot_services().get().set_watchdog_timer)(0, 0, data.len(), data_ptr) }; if status == efi::Status::NOT_READY { log::debug!("Disable watchdog timer with data correctly returned NOT_READY"); } else { @@ -472,7 +506,9 @@ mod tests { WATCHDOG_ARCH_PTR.init(&watchdog as *const _ as *mut c_void); }; // Test case 5: Set watchdog timer with null data - should return SUCCESS (watchdog protocol available) - let status = (st.boot_services().get().set_watchdog_timer)(300, 0, 0, ptr::null_mut()); + // SAFETY: The unsafe block is required because r-efi declares set_watchdog_timer as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { (st.boot_services().get().set_watchdog_timer)(300, 0, 0, ptr::null_mut()) }; if status == efi::Status::SUCCESS { log::debug!("Set watchdog timer correctly returned SUCCESS (watchdog protocol available)"); assert!(SET_PERIOD_CALLED.is_completed(), "set_timer_period was not called during set_watchdog_timer."); @@ -487,7 +523,9 @@ mod tests { init_misc_boot_services_support(st); // Test case 1: Normal stall duration - should return NOT_READY (no metronome available in test) - let status = (st.boot_services().get().stall)(10000); + // SAFETY: The unsafe block is required because r-efi declares stall as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { (st.boot_services().get().stall)(10000) }; if status == efi::Status::NOT_READY { log::debug!("Stall function correctly returned NOT_READY (no metronome protocol)"); } else { @@ -495,7 +533,9 @@ mod tests { } // Test case 2: Zero microseconds stall - should return NOT_READY - let status = (st.boot_services().get().stall)(0); + // SAFETY: The unsafe block is required because r-efi declares stall as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { (st.boot_services().get().stall)(0) }; if status == efi::Status::NOT_READY { log::debug!("Zero stall correctly returned NOT_READY"); } else { @@ -503,7 +543,9 @@ mod tests { } // Test case 3: Maximum stall duration - should return NOT_READY - let status = (st.boot_services().get().stall)(usize::MAX); + // SAFETY: The unsafe block is required because r-efi declares stall as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { (st.boot_services().get().stall)(usize::MAX) }; if status == efi::Status::NOT_READY { log::debug!("Maximum stall correctly returned NOT_READY"); } else { @@ -533,7 +575,9 @@ mod tests { } // Test case 4: Normal stall duration - should return SUCCESS (metronome protocol available) - let status = (st.boot_services().get().stall)(10000); + // SAFETY: The unsafe block is required because r-efi declares stall as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { (st.boot_services().get().stall)(10000) }; if status == efi::Status::SUCCESS { log::debug!("Stall function correctly returned SUCCESS (metronome protocol available)"); assert!(WAIT_FOR_TICK_CALLED.is_completed(), "wait_for_tick was not called during stall."); @@ -550,7 +594,9 @@ mod tests { init_misc_boot_services_support(st); // Call exit_boot_services with a valid map_key let handle: efi::Handle = 0x1000 as efi::Handle; // Example handle - let _status = (st.boot_services().get().exit_boot_services)(handle, valid_map_key); + // SAFETY: The unsafe block is required because r-efi declares exit_boot_services as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let _status = unsafe { (st.boot_services().get().exit_boot_services)(handle, valid_map_key) }; }); } } diff --git a/patina_dxe_core/src/pi_dispatcher.rs b/patina_dxe_core/src/pi_dispatcher.rs index 352f6841c..71e6106e7 100644 --- a/patina_dxe_core/src/pi_dispatcher.rs +++ b/patina_dxe_core/src/pi_dispatcher.rs @@ -272,7 +272,12 @@ impl PiDispatcher

{ if driver.image_handle.is_none() { log::info!("Loading file: {:?}", guid_fmt!(driver.file_name)); let data = driver.pe32.try_content_as_slice()?; - match self.load_image(false, DXE_CORE_HANDLE, driver.device_path, Some(data)) { + + // SAFETY: The device path is constructed from the FV and file + // information and should be valid as long as the underlying FV + // and file data is valid. + let status = unsafe { self.load_image(false, DXE_CORE_HANDLE, driver.device_path, Some(data)) }; + match status { Ok(handle) => { driver.image_handle = Some(handle); driver.security_status = efi::Status::SUCCESS; @@ -295,7 +300,9 @@ impl PiDispatcher

{ dispatch_attempted = true; // Note: ignore error result of core_start_image here - an image returning an error code is expected in some // cases, and a debug output for that is already implemented in core_start_image. - let _status = self.start_image(image_handle); + // SAFETY: image_handle was obtained from a successful load_image call above + // and the loaded image's memory and entry point remain valid. + let _status = unsafe { self.start_image(image_handle) }; } efi::Status::SECURITY_VIOLATION => { log::info!( diff --git a/patina_dxe_core/src/pi_dispatcher/image.rs b/patina_dxe_core/src/pi_dispatcher/image.rs index aa0a30cb9..8e50a4c12 100644 --- a/patina_dxe_core/src/pi_dispatcher/image.rs +++ b/patina_dxe_core/src/pi_dispatcher/image.rs @@ -637,11 +637,18 @@ impl ImageData { } /// Returns a tuple of image meta-data: `(image_as_vec, from_fv, device_handle, authentication_status)` - fn locate_image_metadata_by_buffer( + /// + /// # Safety + /// + /// Caller must ensure `file_path` points to a valid device path or is null. + unsafe fn locate_image_metadata_by_buffer( image: &[u8], file_path: *mut efi::protocols::device_path::Protocol, ) -> (Vec, bool, *mut c_void, u32) { - if let Ok((_, device_handle)) = core_locate_device_path(efi::protocols::device_path::PROTOCOL_GUID, file_path) { + if let Ok((_, device_handle)) = + // SAFETY: file_path validity is guaranteed by the caller per this function's safety contract. + unsafe { core_locate_device_path(efi::protocols::device_path::PROTOCOL_GUID, file_path) } + { (image.to_vec(), false, device_handle, 0) } else { (image.to_vec(), false, protocol_db::INVALID_HANDLE, 0) @@ -651,7 +658,12 @@ impl ImageData { /// Returns the image metadata by its file path using simple file system or load file protocols. /// /// Returns a tuple of (image buffer, from_fv, device handle, authentication status). - fn locate_image_metadata_by_file_path( + /// + /// # Safety + /// + /// Caller must ensure `file_path` points to a valid device path or is null (null is checked + /// and returns `InvalidParameter`). + unsafe fn locate_image_metadata_by_file_path( boot_policy: bool, file_path: *mut efi::protocols::device_path::Protocol, ) -> Result<(Vec, bool, *mut c_void, u32), EfiError> { @@ -659,24 +671,30 @@ impl ImageData { Err(EfiError::InvalidParameter)?; } - if let Ok((buffer, device_handle)) = get_file_buffer_from_fw(file_path) { + // SAFETY: file_path is non-null (checked above) and valid per this function's safety contract. + let result = unsafe { get_file_buffer_from_fw(file_path) }; + if let Ok((buffer, device_handle)) = result { return Ok((buffer, true, device_handle, 0)); } - if let Ok((buffer, device_handle)) = get_file_buffer_from_sfs(file_path) { + // SAFETY: file_path is non-null (checked above) and valid per this function's safety contract. + let result = unsafe { get_file_buffer_from_sfs(file_path) }; + if let Ok((buffer, device_handle)) = result { return Ok((buffer, false, device_handle, 0)); } - if !boot_policy - && let Ok((buffer, device_handle)) = - get_file_buffer_from_load_protocol(efi::protocols::load_file2::PROTOCOL_GUID, false, file_path) - { + // SAFETY: file_path is non-null (checked above) and valid per this function's safety contract. + let result = + unsafe { get_file_buffer_from_load_protocol(efi::protocols::load_file2::PROTOCOL_GUID, false, file_path) }; + if !boot_policy && let Ok((buffer, device_handle)) = result { return Ok((buffer, false, device_handle, 0)); } - if let Ok((buffer, device_handle)) = + // SAFETY: file_path is non-null (checked above) and valid per this function's safety contract. + let result = unsafe { get_file_buffer_from_load_protocol(efi::protocols::load_file::PROTOCOL_GUID, boot_policy, file_path) - { + }; + if let Ok((buffer, device_handle)) = result { return Ok((buffer, false, device_handle, 0)); } @@ -703,7 +721,13 @@ impl super::PiDispatcher

{ /// Returns Ok(efi::Handle) if the image was loaded successfully. /// returns Err(ImageStatus) if there was an error loading the issue. The enum value determines if the image was loaded /// with security violations, or not at all. See [ImageStatus] for details. - pub fn load_image( + /// + /// # Safety + /// + /// If `file_path` is non-null, it must point to a valid UEFI device path + /// structure in readable memory, as it will be dereferenced to locate and + /// authenticate the image. + pub unsafe fn load_image( &self, boot_policy: bool, parent_image_handle: efi::Handle, @@ -719,9 +743,13 @@ impl super::PiDispatcher

{ ImageData::validate_parent(parent_image_handle)?; - let (image_to_load, from_fv, device_handle, auth_status) = match image { - Some(buffer) => ImageData::locate_image_metadata_by_buffer(buffer, file_path), - None => ImageData::locate_image_metadata_by_file_path(boot_policy, file_path)?, + // SAFETY: file_path was validated above (not-null when image is None) and originates from + // the caller's device path, which is required to be valid by the load_image contract. + let (image_to_load, from_fv, device_handle, auth_status) = unsafe { + match image { + Some(buffer) => ImageData::locate_image_metadata_by_buffer(buffer, file_path), + None => ImageData::locate_image_metadata_by_file_path(boot_policy, file_path)?, + } }; // authenticate the image @@ -796,23 +824,21 @@ impl super::PiDispatcher

{ } } - // Loads the image specified by the device_path or source_buffer argument. - // - //See EFI_BOOT_SERVICES::LoadImage() API definition - // in UEFI spec for usage details. - // * boot_policy - indicates whether the image is being loaded by the boot - // manager from the specified device path. ignored if - // source_buffer is not null. - // * parent_image_handle - the caller's image handle. - // * device_path - the file path from which the image is loaded. - // * source_buffer - if not null, pointer to the memory location containing the - // image to be loaded. - // * source_size - size in bytes of source_buffer. ignored if source_buffer is - // null. - // * image_handle - pointer to the returned image handle that is created on - // successful image load. + /// Loads the image specified by the device_path or source_buffer argument. + /// + /// See the EFI_BOOT_SERVICES::LoadImage() API definition in the UEFI spec for parameter + /// and usage details. + /// + /// # Safety + /// + /// - If `source_buffer` is non-null, it must point to valid readable memory of at least + /// `source_size` bytes for the duration of the call. + /// - If `device_path` is non-null, it must point to a valid, well-formed UEFI device path + /// structure in readable memory for the duration of the call. + /// - `image_handle` is null-checked and returns `INVALID_PARAMETER` if null; if non-null, + /// it must point to writable memory suitable for storing an `efi::Handle`. #[coverage(off)] - pub(super) extern "efiapi" fn load_image_efiapi( + pub(super) unsafe extern "efiapi" fn load_image_efiapi( boot_policy: efi::Boolean, parent_image_handle: efi::Handle, device_path: *mut efi::protocols::device_path::Protocol, @@ -834,20 +860,37 @@ impl super::PiDispatcher

{ Some(unsafe { from_raw_parts(source_buffer as *const u8, source_size) }) }; - let (handle, status) = - match Self::instance().load_image(boot_policy.into(), parent_image_handle, device_path, image) { - Ok(handle) => (handle, efi::Status::SUCCESS), - Err(ImageStatus::AccessDenied) => (null_mut(), efi::Status::ACCESS_DENIED), - Err(ImageStatus::SecurityViolation(handle)) => (handle, efi::Status::SECURITY_VIOLATION), - Err(ImageStatus::LoadError(err)) => return err.into(), - }; + // SAFETY: Caller must ensure that device_path (if non-null) points to a + // valid device path structure. + let result = + unsafe { Self::instance().load_image(boot_policy.into(), parent_image_handle, device_path, image) }; + let (handle, status) = match result { + Ok(handle) => (handle, efi::Status::SUCCESS), + Err(ImageStatus::AccessDenied) => (null_mut(), efi::Status::ACCESS_DENIED), + Err(ImageStatus::SecurityViolation(handle)) => (handle, efi::Status::SECURITY_VIOLATION), + Err(ImageStatus::LoadError(err)) => return err.into(), + }; // SAFETY: Caller must ensure that image_handle is a valid pointer. It is null-checked above. unsafe { image_handle.write_unaligned(handle) }; status } - pub fn start_image(&'static self, image_handle: efi::Handle) -> Result<(), efi::Status> { + /// 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. + pub unsafe fn start_image(&'static self, image_handle: efi::Handle) -> Result<(), efi::Status> { PROTOCOL_DB.validate_handle(image_handle)?; if let Some(private_data) = self.image_data.lock().private_image_data.get_mut(&image_handle) { @@ -882,17 +925,19 @@ impl super::PiDispatcher

{ // drop our reference to the private data (i.e. release the lock). drop(private_data); - // invoke the entry point. Code on the other side of this pointer is - // FFI, which is inherently unsafe, but it's not "technically" unsafe - // from a rust standpoint since r_efi doesn't define the ImageEntryPoint - // pointer type as "pointer to unsafe function" - status = entry_point(image_handle, system_table); - - //safety note: any variables with "Drop" routines that need to run - //need to be explicitly dropped before calling exit(). Since exit() - //effectively "longjmp"s back to StartImage(), rust automatic - //drops will not be triggered. - self.exit(image_handle, status, 0, core::ptr::null_mut()); + // SAFETY: Invokes the entry point. The code behind this pointer + // is FFI, which is inherently unsafe. The caller is responsible + // for ensuring that `self` refers to a valid loaded image + // (mapped), and that `entry_point` points to valid executable + // code. + status = unsafe { entry_point(image_handle, system_table) }; + + // SAFETY: any variables with "Drop" routines that need to run need to be explicitly + // dropped before calling exit(). Since exit() effectively "longjmp"s back to + // StartImage(), Rust automatic drops will not be triggered. `image_handle` was + // obtained from the outer `start_image` scope and is the handle of the currently + // running image which will be valid. `exit_data_size` is 0 and `exit_data` is null. + unsafe { self.exit(image_handle, status, 0, core::ptr::null_mut()) }; } else { status = efi::Status::NOT_FOUND; } @@ -937,20 +982,29 @@ impl super::PiDispatcher

{ } } - // Transfers control to the entry point of an image that was loaded by - // load_image. See EFI_BOOT_SERVICES::StartImage() API definition in UEFI spec - // for usage details. - // * image_handle - handle of the image to be started. - // * exit_data_size - pointer to receive the size, in bytes, of exit_data. - // if exit_data is null, this is parameter is ignored. - // * exit_data - pointer to receive a data buffer with exit data, if any. + /// Transfers control to the entry point of an image that was loaded by + /// load_image. See the EFI_BOOT_SERVICES::StartImage() API definition in + /// the UEFI spec for usage details. + /// + /// # Safety + /// + /// If `exit_data_size` and `exit_data` are non-null, they must point to + /// valid writable memory. The caller owns the returned exit data buffer. + /// + /// * image_handle - handle of the image to be started. + /// * exit_data_size - pointer to receive the size, in bytes, of exit_data. + /// if exit_data is null, this is parameter is ignored. + /// * exit_data - pointer to receive a data buffer with exit data, if any. #[coverage(off)] - pub(super) extern "efiapi" fn start_image_efiapi( + pub(super) unsafe extern "efiapi" fn start_image_efiapi( image_handle: efi::Handle, exit_data_size: *mut usize, exit_data: *mut *mut efi::Char16, ) -> efi::Status { - let status = Self::instance().start_image(image_handle); + // SAFETY: image_handle is provided by the caller and is passed through to start_image, + // which validates it against the protocol database and private image data map. The caller + // must ensure that image_handle refers to valid loaded image executable memory. + let status = unsafe { Self::instance().start_image(image_handle) }; // retrieve any exit data that was provided by the entry point. if !exit_data_size.is_null() && !exit_data.is_null() { @@ -981,6 +1035,15 @@ impl super::PiDispatcher

{ } } + /// Unloads a previously loaded image. + /// + /// # Safety Considerations + /// + /// This function is not marked `unsafe` because `image_handle` is safe to + /// accept from unverified sources. It is validated against the protocol + /// database via [`PROTOCOL_DB.validate_handle`] and looked up in the + /// private image data map; if the handle is not found, an error status is + /// returned rather than causing undefined behavior. pub fn unload_image(&self, image_handle: efi::Handle, force_unload: bool) -> Result<(), efi::Status> { PROTOCOL_DB.validate_handle(image_handle)?; let private_data = self.image_data.lock(); @@ -1062,6 +1125,15 @@ impl super::PiDispatcher

{ } #[coverage(off)] + /// Unloads a previously loaded image. + /// + /// # Safety Considerations + /// + /// This function is not marked `unsafe` because `image_handle` is safe to + /// accept from unverified sources. It is validated against the protocol + /// database via [`PROTOCOL_DB.validate_handle`] and looked up in the + /// private image data map; if the handle is not found, an error status is + /// returned rather than causing undefined behavior. pub(super) extern "efiapi" fn unload_image_efiapi(image_handle: efi::Handle) -> efi::Status { match Self::instance().unload_image(image_handle, false) { Ok(()) => efi::Status::SUCCESS, @@ -1069,7 +1141,20 @@ impl super::PiDispatcher

{ } } - fn exit( + /// Terminates a started image and returns control to the caller of + /// `start_image`. + /// + /// `image_handle` is validated against the private image data map; if not + /// found, `INVALID_PARAMETER` is returned. + /// + /// # Safety + /// + /// If `image_handle` is valid and if `exit_data_size` is non-zero and + /// `exit_data` is non-null, the caller must ensure that `exit_data` points + /// to a valid buffer of at least `exit_data_size` bytes. This pointer is + /// stored and later returned to the caller of `start_image` to retrieve the + /// exit data. + unsafe fn exit( &self, image_handle: efi::Handle, status: efi::Status, @@ -1128,21 +1213,35 @@ impl super::PiDispatcher

{ efi::Status::ACCESS_DENIED } - // Terminates a loaded EFI image and returns control to boot services. - // See EFI_BOOT_SERVICES::Exit() API definition in UEFI spec for usage details. - // * image_handle - the handle of the currently running image. - // * exit_status - the exit status for the image. - // * exit_data_size - the size of the exit_data buffer, if exit_data is not - // null. - // * exit_data - optional buffer of data provided by the caller. + /// Terminates a loaded EFI image and returns control to boot services. See + /// the EFI_BOOT_SERVICES::Exit() API definition in the UEFI spec for usage + /// details. + /// + /// `image_handle` is validated against the private image data map; if not + /// found, `INVALID_PARAMETER` is returned. + /// + /// * image_handle - the handle of the currently running image. + /// * exit_status - the exit status for the image. + /// * exit_data_size - the size of the exit_data buffer, if exit_data is notnull. + /// * exit_data - optional buffer of data provided by the caller. + /// + /// # Safety + /// + /// If `image_handle` is valid and if `exit_data_size` is non-zero and + /// `exit_data` is non-null, the caller must ensure that `exit_data` points + /// to a valid buffer of at least `exit_data_size` bytes. This pointer is + /// stored and later returned to the caller of `start_image` to retrieve the + /// exit data. #[coverage(off)] - pub(super) extern "efiapi" fn exit_efiapi( + pub(super) unsafe extern "efiapi" fn exit_efiapi( image_handle: efi::Handle, status: efi::Status, exit_data_size: usize, exit_data: *mut efi::Char16, ) -> efi::Status { - Self::instance().exit(image_handle, status, exit_data_size, exit_data) + // SAFETY: The caller of exit_efiapi is responsible for ensuring that exit_data + // (if non-null with non-zero size) points to a valid buffer. + unsafe { Self::instance().exit(image_handle, status, exit_data_size, exit_data) } } pub(super) extern "efiapi" fn runtime_image_protection_fixup_ebs(event: efi::Event, _context: *mut c_void) { @@ -1293,12 +1392,18 @@ fn get_file_guid_from_device_path(path: *mut efi::protocols::device_path::Protoc Ok(Guid::from_bytes(file_path_node.data().try_into().map_err(|_| EfiError::BadBufferSize)?)) } -fn get_file_buffer_from_fw( +/// Reads an image from a firmware volume located via the given device path. +/// +/// # Safety +/// +/// Caller must ensure `file_path` points to a valid device path. +unsafe fn get_file_buffer_from_fw( file_path: *mut efi::protocols::device_path::Protocol, ) -> Result<(Vec, efi::Handle), EfiError> { // Locate the handles to a device on the file_path that supports the firmware volume protocol + // SAFETY: file_path validity is guaranteed by the caller per this function's safety contract. let (remaining_file_path, handle) = - core_locate_device_path(pi::protocols::firmware_volume::PROTOCOL_GUID.into_inner(), file_path)?; + unsafe { core_locate_device_path(pi::protocols::firmware_volume::PROTOCOL_GUID.into_inner(), file_path) }?; // For FwVol File system there is only a single file name that is a GUID. let fv_name_guid = get_file_guid_from_device_path(remaining_file_path)?; @@ -1337,11 +1442,17 @@ fn get_file_buffer_from_fw( Ok((section_slice.to_vec(), handle)) } -fn get_file_buffer_from_sfs( +/// Reads an image from a simple file system located via the given device path. +/// +/// # Safety +/// +/// Caller must ensure `file_path` points to a valid device path. +unsafe fn get_file_buffer_from_sfs( file_path: *mut efi::protocols::device_path::Protocol, ) -> Result<(Vec, efi::Handle), EfiError> { + // SAFETY: file_path validity is guaranteed by the caller per this function's safety contract. let (remaining_file_path, handle) = - core_locate_device_path(efi::protocols::simple_file_system::PROTOCOL_GUID, file_path)?; + unsafe { core_locate_device_path(efi::protocols::simple_file_system::PROTOCOL_GUID, file_path) }?; let mut file = SimpleFile::open_volume(handle)?; @@ -1374,7 +1485,12 @@ fn get_file_buffer_from_sfs( Ok((file.read()?, handle)) } -fn get_file_buffer_from_load_protocol( +/// Reads an image via a load file protocol located on the given device path. +/// +/// # Safety +/// +/// Caller must ensure `file_path` points to a valid device path. +unsafe fn get_file_buffer_from_load_protocol( protocol: efi::Guid, boot_policy: bool, file_path: *mut efi::protocols::device_path::Protocol, @@ -1388,7 +1504,8 @@ fn get_file_buffer_from_load_protocol( Err(EfiError::InvalidParameter)?; } - let (remaining_file_path, handle) = core_locate_device_path(protocol, file_path)?; + // SAFETY: file_path validity is guaranteed by the caller per this function's safety contract. + let (remaining_file_path, handle) = unsafe { core_locate_device_path(protocol, file_path) }?; let load_file = PROTOCOL_DB.get_interface_for_handle(handle, protocol)?; // SAFETY: load_file is obtained from the protocol database and is a valid load_file protocol pointer. @@ -1397,13 +1514,16 @@ fn get_file_buffer_from_load_protocol( //determine buffer size. let mut buffer_size = 0; - let status = (load_file.load_file)( - load_file, - remaining_file_path, - boot_policy.into(), - core::ptr::addr_of_mut!(buffer_size), - core::ptr::null_mut(), - ); + // SAFETY: load_file is a valid pointer to a load_file protocol instance obtained from the protocol database above. + let status = unsafe { + (load_file.load_file)( + load_file, + remaining_file_path, + boot_policy.into(), + core::ptr::addr_of_mut!(buffer_size), + core::ptr::null_mut(), + ) + }; match status { efi::Status::BUFFER_TOO_SMALL => (), // expected @@ -1412,13 +1532,16 @@ fn get_file_buffer_from_load_protocol( } let mut file_buffer = vec![0u8; buffer_size]; - let status = (load_file.load_file)( - load_file, - remaining_file_path, - boot_policy.into(), - core::ptr::addr_of_mut!(buffer_size), - file_buffer.as_mut_ptr() as *mut c_void, - ); + // SAFETY: load_file is a valid pointer to a load_file protocol instance obtained from the protocol database above. + let status = unsafe { + (load_file.load_file)( + load_file, + remaining_file_path, + boot_policy.into(), + core::ptr::addr_of_mut!(buffer_size), + file_buffer.as_mut_ptr() as *mut c_void, + ) + }; EfiError::status_to_result(status).map(|_| (file_buffer, handle)) } @@ -1617,7 +1740,9 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let result = PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), None); + // SAFETY: Test code - file_path is null and no image buffer is provided; load_image will return an error. + let result = + unsafe { PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), None) }; assert!(matches!(result, Err(ImageStatus::LoadError(EfiError::InvalidParameter)))); }); @@ -1634,9 +1759,12 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let image_handle = PI_DISPATCHER - .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(image.as_slice())) - .unwrap(); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let image_handle = unsafe { + PI_DISPATCHER + .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(image.as_slice())) + .unwrap() + }; let private_data = PI_DISPATCHER.image_data.lock(); let image_data = private_data.private_image_data.get(&image_handle).unwrap(); @@ -1661,12 +1789,15 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let status = PI_DISPATCHER.load_image( - false, - protocol_db::DXE_CORE_HANDLE, - core::ptr::null_mut(), - Some(image.as_slice()), - ); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let status = unsafe { + PI_DISPATCHER.load_image( + false, + protocol_db::DXE_CORE_HANDLE, + core::ptr::null_mut(), + Some(image.as_slice()), + ) + }; assert!(status.is_ok()); }); } @@ -1682,11 +1813,17 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - assert!( - PI_DISPATCHER - .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(image.as_slice()),) - .is_ok() - ); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let status = unsafe { + PI_DISPATCHER.load_image( + false, + protocol_db::DXE_CORE_HANDLE, + core::ptr::null_mut(), + Some(image.as_slice()), + ) + }; + + assert!(status.is_ok()); }); } @@ -1702,12 +1839,15 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let result = PI_DISPATCHER.load_image( - false, - protocol_db::DXE_CORE_HANDLE, - core::ptr::null_mut(), - Some(image.as_slice()), - ); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let result = unsafe { + PI_DISPATCHER.load_image( + false, + protocol_db::DXE_CORE_HANDLE, + core::ptr::null_mut(), + Some(image.as_slice()), + ) + }; assert!(matches!(result, Err(ImageStatus::LoadError(EfiError::Unsupported)))); }); @@ -1731,12 +1871,15 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let result = PI_DISPATCHER.load_image( - false, - protocol_db::DXE_CORE_HANDLE, - core::ptr::null_mut(), - Some(image.as_slice()), - ); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let result = unsafe { + PI_DISPATCHER.load_image( + false, + protocol_db::DXE_CORE_HANDLE, + core::ptr::null_mut(), + Some(image.as_slice()), + ) + }; assert!(matches!(result, Err(ImageStatus::LoadError(EfiError::Unsupported)))); }); @@ -1753,12 +1896,15 @@ mod tests { static PI_DISPATCHER: PiDispatcher = PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let status = PI_DISPATCHER.load_image( - false, - protocol_db::DXE_CORE_HANDLE, - core::ptr::null_mut(), - Some(image.as_slice()), - ); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let status = unsafe { + PI_DISPATCHER.load_image( + false, + protocol_db::DXE_CORE_HANDLE, + core::ptr::null_mut(), + Some(image.as_slice()), + ) + }; assert!(matches!(status, Err(ImageStatus::LoadError(EfiError::LoadError)))); }); } @@ -1775,12 +1921,15 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let status = PI_DISPATCHER.load_image( - false, - protocol_db::DXE_CORE_HANDLE, - core::ptr::null_mut(), - Some(image.as_slice()), - ); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let status = unsafe { + PI_DISPATCHER.load_image( + false, + protocol_db::DXE_CORE_HANDLE, + core::ptr::null_mut(), + Some(image.as_slice()), + ) + }; assert!(matches!(status, Err(ImageStatus::LoadError(EfiError::LoadError)))); }); } @@ -1797,12 +1946,15 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let status = PI_DISPATCHER.load_image( - false, - protocol_db::DXE_CORE_HANDLE, - core::ptr::null_mut(), - Some(image.as_slice()), - ); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let status = unsafe { + PI_DISPATCHER.load_image( + false, + protocol_db::DXE_CORE_HANDLE, + core::ptr::null_mut(), + Some(image.as_slice()), + ) + }; assert!(matches!(status, Err(ImageStatus::LoadError(EfiError::LoadError)))); }); } @@ -1819,8 +1971,10 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let status = - PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let status = unsafe { + PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + }; assert!(matches!(status, Err(ImageStatus::LoadError(EfiError::LoadError)))); }); } @@ -1861,9 +2015,12 @@ mod tests { ) .unwrap(); - let image_handle = PI_DISPATCHER - .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) - .unwrap(); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let image_handle = unsafe { + PI_DISPATCHER + .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + .unwrap() + }; assert!(SECURITY_CALL_EXECUTED.load(core::sync::atomic::Ordering::SeqCst)); @@ -1941,9 +2098,12 @@ mod tests { ) .unwrap(); - let image_handle = PI_DISPATCHER - .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) - .unwrap(); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let image_handle = unsafe { + PI_DISPATCHER + .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + .unwrap() + }; assert!(SECURITY2_CALL_EXECUTED.load(core::sync::atomic::Ordering::SeqCst)); @@ -1996,8 +2156,10 @@ mod tests { assert_eq!(PROTOCOL_DB.locate_handles(Some(efi::protocols::loaded_image::PROTOCOL_GUID)).unwrap().len(), 1); assert_eq!(PI_DISPATCHER.image_data.lock().private_image_data.len(), 1); // In this result, we expect to get SECURITY_VIOLATION, but the image_handle is successfully populated. - let status = - PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let status = unsafe { + PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + }; let image_handle = match status { Err(ImageStatus::SecurityViolation(h)) => h, @@ -2048,8 +2210,10 @@ mod tests { // The handle / private data count should be 1, which is the dxe core image. assert_eq!(PROTOCOL_DB.locate_handles(Some(efi::protocols::loaded_image::PROTOCOL_GUID)).unwrap().len(), 1); assert_eq!(PI_DISPATCHER.image_data.lock().private_image_data.len(), 1); - let status = - PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let status = unsafe { + PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + }; assert!(matches!(status, Err(ImageStatus::AccessDenied))); // There should still be only 1 handle @@ -2095,8 +2259,10 @@ mod tests { assert_eq!(PROTOCOL_DB.locate_handles(Some(efi::protocols::loaded_image::PROTOCOL_GUID)).unwrap().len(), 1); assert_eq!(PI_DISPATCHER.image_data.lock().private_image_data.len(), 1); - let status = - PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let status = unsafe { + PI_DISPATCHER.load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + }; assert!(matches!(status, Err(ImageStatus::LoadError(EfiError::InvalidParameter)))); // There should still be only 1 handle @@ -2116,16 +2282,17 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let image_handle = PI_DISPATCHER - .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) - .unwrap(); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let image_handle = unsafe { + PI_DISPATCHER + .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + .unwrap() + }; // Getting the image loaded into a buffer that is executable would require OS-specific interactions. This means that // all the memory backing our test GCD instance is likely to be marked "NX" - which makes it hard for start_image to // jump to it. // To allow testing of start_image, override the image entrypoint pointer so that it points to a stub routine - // in this test - because it is part of the test executable and not part of the "load_image" buffer, it can be - // executed. static ENTRY_POINT_RAN: AtomicBool = AtomicBool::new(false); pub extern "efiapi" fn test_entry_point( _image_handle: *mut core::ffi::c_void, @@ -2140,7 +2307,9 @@ mod tests { image_data.entry_point = test_entry_point; drop(private_data); - PI_DISPATCHER.start_image(image_handle).unwrap(); + // SAFETY: image_handle was obtained from a successful load_image call above and the + // entry point has been overridden to point to valid executable test code. + unsafe { PI_DISPATCHER.start_image(image_handle) }.unwrap(); assert!(ENTRY_POINT_RAN.load(core::sync::atomic::Ordering::Relaxed)); let mut private_data = PI_DISPATCHER.image_data.lock(); @@ -2162,10 +2331,12 @@ mod tests { CORE.pi_dispatcher.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); CORE.override_instance(); - let image_handle = CORE - .pi_dispatcher - .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) - .unwrap(); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let image_handle = unsafe { + CORE.pi_dispatcher + .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + .unwrap() + }; // Getting the image loaded into a buffer that is executable would require OS-specific interactions. This means that // all the memory backing our test GCD instance is likely to be marked "NX" - which makes it hard for start_image to @@ -2190,11 +2361,16 @@ mod tests { let mut exit_data_size = 0; let mut exit_data: *mut u16 = core::ptr::null_mut(); - let status = PiDispatcher::::start_image_efiapi( - image_handle, - core::ptr::addr_of_mut!(exit_data_size), - core::ptr::addr_of_mut!(exit_data), - ); + // SAFETY: image_handle was obtained from a successful load_image call above and the + // entry point has been overridden to point to valid executable test code. + // exit_data_size and exit_data are valid local pointers. + let status = unsafe { + PiDispatcher::::start_image_efiapi( + image_handle, + core::ptr::addr_of_mut!(exit_data_size), + core::ptr::addr_of_mut!(exit_data), + ) + }; assert_eq!(status, efi::Status::UNSUPPORTED); assert!(ENTRY_POINT_RAN.load(core::sync::atomic::Ordering::Relaxed)); @@ -2215,9 +2391,12 @@ mod tests { PiDispatcher::::new(patina_ffs_extractors::NullSectionExtractor); PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); - let image_handle = PI_DISPATCHER - .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) - .unwrap(); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let image_handle = unsafe { + PI_DISPATCHER + .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + .unwrap() + }; PI_DISPATCHER.unload_image(image_handle, false).unwrap(); @@ -2230,7 +2409,8 @@ mod tests { fn locate_image_metadata_by_file_path_should_fail_if_no_file_support() { with_locked_state(|| { assert_eq!( - ImageData::locate_image_metadata_by_file_path(true, core::ptr::null_mut()), + // SAFETY: Testing null pointer handling - function checks for null and returns InvalidParameter. + unsafe { ImageData::locate_image_metadata_by_file_path(true, core::ptr::null_mut()) }, Err(EfiError::InvalidParameter) ); @@ -2265,7 +2445,11 @@ mod tests { ]; let device_path_ptr = device_path_bytes.as_mut_ptr() as *mut efi::protocols::device_path::Protocol; - assert_eq!(ImageData::locate_image_metadata_by_file_path(true, device_path_ptr), Err(EfiError::NotFound)); + assert_eq!( + // SAFETY: device_path_ptr points to a valid device path constructed above. + unsafe { ImageData::locate_image_metadata_by_file_path(true, device_path_ptr) }, + Err(EfiError::NotFound) + ); }); } @@ -2470,7 +2654,8 @@ mod tests { test_file.read_to_end(&mut image).expect("failed to read test file"); assert_eq!( - ImageData::locate_image_metadata_by_file_path(true, device_path_ptr), + // SAFETY: device_path_ptr points to a valid device path constructed above. + unsafe { ImageData::locate_image_metadata_by_file_path(true, device_path_ptr) }, Ok((image, false, handle, 0)) ); }); @@ -2533,7 +2718,8 @@ mod tests { test_file.read_to_end(&mut image).expect("failed to read test file"); assert_eq!( - ImageData::locate_image_metadata_by_file_path(true, device_path_ptr), + // SAFETY: device_path_ptr points to a valid device path constructed above. + unsafe { ImageData::locate_image_metadata_by_file_path(true, device_path_ptr) }, Ok((image, false, handle, 0)) ); }); @@ -2560,9 +2746,12 @@ mod tests { PI_DISPATCHER.init(&create_dxe_core_hob(), SYSTEM_TABLE.lock().as_mut().unwrap()); // Test 1: Full load_image flow - let image_handle = PI_DISPATCHER - .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) - .unwrap(); + // SAFETY: Test code - file_path is null; image is loaded from the provided buffer. + let image_handle = unsafe { + PI_DISPATCHER + .load_image(false, protocol_db::DXE_CORE_HANDLE, core::ptr::null_mut(), Some(&image)) + .unwrap() + }; // Verify the image was loaded successfully with correct properties let private_data = PI_DISPATCHER.image_data.lock(); diff --git a/patina_dxe_core/src/protocols.rs b/patina_dxe_core/src/protocols.rs index 9693e9c4f..25aa57af6 100644 --- a/patina_dxe_core/src/protocols.rs +++ b/patina_dxe_core/src/protocols.rs @@ -28,6 +28,10 @@ use crate::{ pub static PROTOCOL_DB: SpinLockedProtocolDb = SpinLockedProtocolDb::new(); +/// Installs a protocol interface on a handle. +/// +/// This function is safe because `interface` is an opaque pointer that is stored but never +/// dereferenced. All other parameters are value types. pub fn core_install_protocol_interface( handle: Option, protocol: efi::Guid, @@ -50,7 +54,15 @@ pub fn core_install_protocol_interface( Ok(handle) } -extern "efiapi" fn install_protocol_interface( +/// Installs a protocol interface on a handle. +/// +/// # Safety +/// +/// `handle` must be a valid pointer to an `efi::Handle` (may point to a null handle for new handle +/// creation). `protocol` must be a valid pointer to an `efi::Guid`. Both are null checked, but +/// validity of the referenced memory is the caller's responsibility. Throughout the lifetime of the +/// interface reference, the caller must ensure it remains valid. +unsafe extern "efiapi" fn install_protocol_interface( handle: *mut efi::Handle, protocol: *mut efi::Guid, interface_type: efi::InterfaceType, @@ -77,6 +89,10 @@ extern "efiapi" fn install_protocol_interface( efi::Status::SUCCESS } +/// Uninstalls a protocol interface on a handle. +/// +/// This function is safe because `interface` is an opaque pointer used for +/// comparision but never dereferenced. All other parameters are value types. pub fn core_uninstall_protocol_interface( handle: efi::Handle, protocol: efi::Guid, @@ -167,7 +183,14 @@ pub fn core_uninstall_protocol_interface( PROTOCOL_DB.uninstall_protocol_interface(handle, protocol, interface) } -extern "efiapi" fn uninstall_protocol_interface( +/// Uninstalls a protocol interface from a handle. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid`. It is null checked, but validity of the +/// referenced memory is the caller's responsibility. NOTE: `interface` is not dereferenced in this +/// function, so its validity is not required for safety. +unsafe extern "efiapi" fn uninstall_protocol_interface( handle: efi::Handle, protocol: *mut efi::Guid, interface: *mut c_void, @@ -198,7 +221,14 @@ fn uninstall_dummy_interface(handle: efi::Handle) -> Result<(), EfiError> { PROTOCOL_DB.uninstall_protocol_interface(handle, PRIVATE_DUMMY_INTERFACE_GUID, core::ptr::null_mut()) } -extern "efiapi" fn reinstall_protocol_interface( +/// Reinstalls a protocol interface on a handle. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid`. It is null checked, but validity of the +/// referenced memory is the caller's responsibility. Throughout the lifetime of the interface +/// reference, the caller must ensure it remains valid. +unsafe extern "efiapi" fn reinstall_protocol_interface( handle: efi::Handle, protocol: *mut efi::Guid, old_interface: *mut c_void, @@ -218,7 +248,9 @@ extern "efiapi" fn reinstall_protocol_interface( } // Call uninstall to close all agents that are currently consuming old_interface. - match uninstall_protocol_interface(handle, protocol, old_interface) { + // SAFETY: `protocol` is checked for null above. `handle` and `old_interface` are passed + // through from the caller per the function-level safety contract. + match unsafe { uninstall_protocol_interface(handle, protocol, old_interface) } { efi::Status::SUCCESS => (), err => { let result = uninstall_dummy_interface(handle); @@ -251,7 +283,14 @@ extern "efiapi" fn reinstall_protocol_interface( efi::Status::SUCCESS } -extern "efiapi" fn register_protocol_notify( +/// Registers for notification when a protocol interface is installed. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid`. `registration` must be a valid pointer +/// to receive the registration key. Both are null checked, but validity of the referenced memory +/// is the caller's responsibility. +unsafe extern "efiapi" fn register_protocol_notify( protocol: *mut efi::Guid, event: efi::Event, registration: *mut *mut c_void, @@ -273,7 +312,16 @@ extern "efiapi" fn register_protocol_notify( } } -extern "efiapi" fn locate_handle( +/// Locates handles that support a specified protocol. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid` when `search_type` is `BY_PROTOCOL`. +/// `search_key` must be a valid registration key when `search_type` is `BY_REGISTER_NOTIFY`. +/// `buffer_size` must be a valid pointer to a `usize`. +/// `handle_buffer` must be valid for writes of the number of handles indicated by `buffer_size`. +/// All pointers are null checked, but validity of the referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn locate_handle( search_type: efi::LocateSearchType, protocol: *mut efi::Guid, search_key: *mut c_void, @@ -340,22 +388,42 @@ extern "efiapi" fn locate_handle( } } -pub extern "efiapi" fn handle_protocol( +/// Retrieves a protocol interface from a handle. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid`. `interface` must be a valid pointer to +/// receive the protocol interface pointer. Both are null checked by the underlying `open_protocol` +/// implementation, but validity of the referenced memory is the caller's responsibility. +pub unsafe extern "efiapi" fn handle_protocol( handle: efi::Handle, protocol: *mut efi::Guid, interface: *mut *mut c_void, ) -> efi::Status { - open_protocol( - handle, - protocol, - interface, - DXE_CORE_HANDLE, - core::ptr::null_mut(), - efi::OPEN_PROTOCOL_BY_HANDLE_PROTOCOL, - ) + // SAFETY: `protocol` and `interface` are forwarded from the caller per this function's safety + // contract. `DXE_CORE_HANDLE` is a valid static handle. A null controller handle and + // `BY_HANDLE_PROTOCOL` attributes are safe constant values. + unsafe { + open_protocol( + handle, + protocol, + interface, + DXE_CORE_HANDLE, + core::ptr::null_mut(), + efi::OPEN_PROTOCOL_BY_HANDLE_PROTOCOL, + ) + } } -extern "efiapi" fn open_protocol( +/// Opens a protocol interface on a handle with usage tracking. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid`. `interface` must be a valid pointer to +/// receive the protocol interface pointer (may be null only when `attributes` is +/// `OPEN_PROTOCOL_TEST_PROTOCOL`). Both are null checked, but validity of the referenced memory +/// is the caller's responsibility. +unsafe extern "efiapi" fn open_protocol( handle: efi::Handle, protocol: *mut efi::Guid, interface: *mut *mut c_void, @@ -437,7 +505,13 @@ extern "efiapi" fn open_protocol( efi::Status::SUCCESS } -extern "efiapi" fn close_protocol( +/// Closes a protocol on a handle that was previously opened. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid`. It is null checked, but validity of the +/// referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn close_protocol( handle: efi::Handle, protocol: *mut efi::Guid, agent_handle: efi::Handle, @@ -471,7 +545,15 @@ extern "efiapi" fn close_protocol( } } -extern "efiapi" fn open_protocol_information( +/// Retrieves the list of agents that currently have a protocol interface opened. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid`. `entry_buffer` must be a valid pointer +/// to receive an allocated array of `OpenProtocolInformationEntry`. `entry_count` must be a valid +/// pointer to receive the number of entries. All pointers are null checked, but validity of the +/// referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn open_protocol_information( handle: efi::Handle, protocol: *mut efi::Guid, entry_buffer: *mut *mut efi::OpenProtocolInformationEntry, @@ -512,6 +594,11 @@ extern "efiapi" fn open_protocol_information( } } +/// # Safety +/// +/// `handle` must be a valid pointer to an `efi::Handle` (may point to a null handle for new +/// handle creation). `args` must consist of paired `(*mut efi::Guid, *mut c_void)` entries +/// terminated by a null `*mut efi::Guid` sentinel. unsafe extern "C" fn install_multiple_protocol_interfaces(handle: *mut efi::Handle, mut args: ...) -> efi::Status { // The UEFI spec does not indicate whether the protocols installed here are atomic with respect to notify - i.e. // whether any registered notifies should be invoked between the installation of the multiple protocols, or only @@ -538,10 +625,12 @@ unsafe extern "C" fn install_multiple_protocol_interfaces(handle: *mut efi::Hand let interface: *mut c_void = unsafe { args.arg() }; // SAFETY: protocol is checked for null above before dereferencing. if unsafe { *protocol } == efi::protocols::device_path::PROTOCOL_GUID - && let Ok((remaining_path, handle)) = core_locate_device_path( + // SAFETY: `interface` is provided by the caller as a device path pointer per the + // function-level safety contract. + && let Ok((remaining_path, handle)) = unsafe { core_locate_device_path( efi::protocols::device_path::PROTOCOL_GUID, interface as *const efi::protocols::device_path::Protocol, - ) + ) } && PROTOCOL_DB.validate_handle(handle).is_ok() && { // SAFETY: remaining_path is returned from core_locate_device_path and is a valid device path pointer. @@ -556,14 +645,19 @@ unsafe extern "C" fn install_multiple_protocol_interfaces(handle: *mut efi::Hand let mut interfaces_to_uninstall_on_error = Vec::new(); for (protocol, interface) in interfaces_to_install { - match install_protocol_interface(handle, protocol, efi::NATIVE_INTERFACE, interface) { + // SAFETY: `handle` is null checked above. `protocol` is null checked when building + // interfaces_to_install. `interface` validity is the caller's responsibility per the + // function level safety contract. + match unsafe { install_protocol_interface(handle, protocol, efi::NATIVE_INTERFACE, interface) } { efi::Status::SUCCESS => interfaces_to_uninstall_on_error.push((protocol, interface)), err => { //on error, attempt to uninstall all the previously installed interfaces. best-effort, errors are ignored. for (protocol, interface) in interfaces_to_uninstall_on_error { // SAFETY: handle is validated for null above. let handle_value = unsafe { *handle }; - let _ = uninstall_protocol_interface(handle_value, protocol, interface); + // SAFETY: `protocol` was null-checked when building interfaces_to_install. + // Best-effort rollback; errors are ignored. + let _ = unsafe { uninstall_protocol_interface(handle_value, protocol, interface) }; } return err; } @@ -573,6 +667,12 @@ unsafe extern "C" fn install_multiple_protocol_interfaces(handle: *mut efi::Hand efi::Status::SUCCESS } +/// Uninstalls multiple protocol interfaces from a handle. +/// +/// # Safety +/// +/// `args` must consist of paired `(*mut efi::Guid, *mut c_void)` entries terminated by a null +/// `*mut efi::Guid` sentinel. unsafe extern "C" fn uninstall_multiple_protocol_interfaces(handle: efi::Handle, mut args: ...) -> efi::Status { if handle.is_null() { return efi::Status::INVALID_PARAMETER; @@ -592,7 +692,9 @@ unsafe extern "C" fn uninstall_multiple_protocol_interfaces(handle: efi::Handle, let mut interfaces_to_reinstall_on_error = Vec::new(); for (protocol, interface) in interfaces_to_uninstall { - match uninstall_protocol_interface(handle, protocol, interface) { + // SAFETY: `protocol` was null-checked when building interfaces_to_uninstall. + // `handle` is validated for null above. `interface` is passed through from the caller. + match unsafe { uninstall_protocol_interface(handle, protocol, interface) } { efi::Status::SUCCESS => interfaces_to_reinstall_on_error.push((protocol, interface)), _err => { //on error, attempt to re-install all the previously uninstall interfaces. best-effort, errors are ignored. @@ -609,7 +711,12 @@ unsafe extern "C" fn uninstall_multiple_protocol_interfaces(handle: efi::Handle, efi::Status::SUCCESS } -extern "efiapi" fn protocols_per_handle( +/// # Safety +/// +/// `protocol_buffer` must be a valid pointer to receive an allocated array of GUID pointers. +/// `protocol_buffer_count` must be a valid pointer to receive the number of entries. Both are +/// null checked, but validity of the referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn protocols_per_handle( handle: efi::Handle, protocol_buffer: *mut *mut *mut efi::Guid, protocol_buffer_count: *mut usize, @@ -655,7 +762,14 @@ extern "efiapi" fn protocols_per_handle( } } -extern "efiapi" fn locate_handle_buffer( +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid` when `search_type` is `BY_PROTOCOL`. +/// `search_key` must be a valid registration key when `search_type` is `BY_REGISTER_NOTIFY`. +/// `no_handles` must be a valid pointer to receive the handle count. `buffer` must be a valid +/// pointer to receive the allocated handle array. Both are null checked, but validity of the +/// referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn locate_handle_buffer( search_type: efi::LocateSearchType, protocol: *mut efi::Guid, search_key: *mut c_void, @@ -666,8 +780,8 @@ extern "efiapi" fn locate_handle_buffer( return efi::Status::INVALID_PARAMETER; } - //EDK2 C reference code unconditionally sets no_handles and buffer to default values regardless of success or failure - //of the function, and some callers expect this behavior (and don't check return status before using no_handles). + // EDK2 C reference code unconditionally sets no_handles and buffer to default values regardless of success or failure + // of the function, and some callers expect this behavior (and don't check return status before using no_handles). // SAFETY: Caller must ensure that no_handles and buffer are valid pointers. They are null-checked above. unsafe { no_handles.write_unaligned(0); @@ -703,7 +817,7 @@ extern "efiapi" fn locate_handle_buffer( if handles.is_empty() { efi::Status::NOT_FOUND } else { - //caller is supposed to free the handle buffer using free pool, so we need to allocate it using allocate pool. + // caller is supposed to free the handle buffer using free pool, so we need to allocate it using allocate pool. let buffer_size = handles.len() * size_of::(); match core_allocate_pool(efi::BOOT_SERVICES_DATA, buffer_size) { Err(err) => err.into(), @@ -718,7 +832,15 @@ extern "efiapi" fn locate_handle_buffer( } } -extern "efiapi" fn locate_protocol( +/// Locates the first protocol instance matching a protocol GUID. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid`. `interface` must be a valid pointer to +/// receive the protocol interface pointer. `registration` must be null or a valid registration key +/// from a prior call to `register_protocol_notify`. All pointers are null checked, but validity +/// of the referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn locate_protocol( protocol: *mut efi::Guid, registration: *mut c_void, interface: *mut *mut c_void, @@ -759,7 +881,13 @@ extern "efiapi" fn locate_protocol( efi::Status::SUCCESS } -pub fn core_locate_device_path( +/// Locates the best matching handle for a device path that supports a specified protocol. +/// +/// # Safety +/// +/// `device_path` must point to a valid UEFI device path in readable memory. It is null checked, +/// but validity of the referenced device path structure is the caller's responsibility. +pub unsafe fn core_locate_device_path( protocol: efi::Guid, device_path: *const r_efi::protocols::device_path::Protocol, ) -> Result<(*mut r_efi::protocols::device_path::Protocol, efi::Handle), EfiError> { @@ -777,7 +905,10 @@ pub fn core_locate_device_path( for handle in handles { let mut temp_device_path: *mut r_efi::protocols::device_path::Protocol = core::ptr::null_mut(); let temp_device_path_ptr: *mut *mut c_void = &mut temp_device_path as *mut _ as *mut *mut c_void; - let status = handle_protocol(handle, device_path_protocol_guid, temp_device_path_ptr); + // SAFETY: `handle` comes from `locate_handles` and is valid. `device_path_protocol_guid` + // points to a valid static GUID. `temp_device_path_ptr` is derived from a local variable + // and is valid for writes. + let status = unsafe { handle_protocol(handle, device_path_protocol_guid, temp_device_path_ptr) }; if status != efi::Status::SUCCESS { continue; } @@ -804,7 +935,15 @@ pub fn core_locate_device_path( Ok((best_remaining_path as *mut r_efi::protocols::device_path::Protocol, best_device)) } -extern "efiapi" fn locate_device_path( +/// Locates a handle for a device on a device path that supports a specified protocol. +/// +/// # Safety +/// +/// `protocol` must be a valid pointer to an `efi::Guid`. `device_path` must be a valid pointer to +/// a `*mut Protocol` pointer that references a valid UEFI device path. `device` must be a valid +/// pointer to receive the located handle. All pointers are null checked, but validity of the +/// referenced memory is the caller's responsibility. +unsafe extern "efiapi" fn locate_device_path( protocol: *mut efi::Guid, device_path: *mut *mut r_efi::protocols::device_path::Protocol, device: *mut efi::Handle, @@ -825,10 +964,13 @@ extern "efiapi" fn locate_device_path( // SAFETY: protocol is null-checked above. unsafe { protocol.read_unaligned() } }; - let (best_remaining_path, best_device) = match core_locate_device_path(protocol_guid, current_device_path) { - Err(err) => return err.into(), - Ok((path, device)) => (path, device), - }; + // SAFETY: `current_device_path` was read from the caller-provided `device_path` pointer + // (null-checked above) and verified non-null. + let (best_remaining_path, best_device) = + match unsafe { core_locate_device_path(protocol_guid, current_device_path) } { + Err(err) => return err.into(), + Ok((path, device)) => (path, device), + }; if device.is_null() { return efi::Status::INVALID_PARAMETER; } @@ -844,12 +986,12 @@ extern "efiapi" fn locate_device_path( pub fn init_protocol_support(st: &mut EfiSystemTable) { let mut bs = st.boot_services().get(); - //This bit of trickery is needed because r_efi definition of (Un)InstallMultipleProtocolInterfaces - //is not variadic, due to rust only supporting variadic for "unsafe extern C" and not "efiapi" - //until rust 1.91. For our purposes "efiapi" and "extern C" match, so we can get away with a - //transmute here. Fixing it properly would require an upstream change in r_efi to pick up. There is also a bug in - //the r_efi definition for uninstall_multiple_protocol_interfaces - per spec, the first argument is a handle, but - //r_efi has it as *mut handle. + // This bit of trickery is needed because r_efi definition of (Un)InstallMultipleProtocolInterfaces + // is not variadic, due to rust only supporting variadic for "unsafe extern C" and not "efiapi" + // until rust 1.91. For our purposes "efiapi" and "extern C" match, so we can get away with a + // transmute here. Fixing it properly would require an upstream change in r_efi to pick up. There is also a bug in + // the r_efi definition for uninstall_multiple_protocol_interfaces - per spec, the first argument is a handle, but + // r_efi has it as *mut handle. // SAFETY: Transmute bridges r_efi signature mismatch for variadic interface. ABI matches for efiapi/extern C. bs.install_multiple_protocol_interfaces = unsafe { let ptr = install_multiple_protocol_interfaces as *const (); diff --git a/sdk/patina/src/boot_services.rs b/sdk/patina/src/boot_services.rs index 5c4003407..7694ae94e 100644 --- a/sdk/patina/src/boot_services.rs +++ b/sdk/patina/src/boot_services.rs @@ -180,8 +180,8 @@ pub trait BootServices { event_type, notify_tpl, mem::transmute::< - Option, - Option>::Type)>, + Option, + Option>::Type)>, >(notify_function), notify_context.into_ptr() as *mut T::Type, ) @@ -228,8 +228,8 @@ pub trait BootServices { event_type, notify_tpl, mem::transmute::< - Option, - Option>::Type)>, + Option, + Option>::Type)>, >(notify_function), notify_context.into_ptr() as *mut ::Type, event_group, @@ -307,8 +307,15 @@ pub trait BootServices { /// Frees memory pages. /// - /// [UEFI Spec Documentation: 7.2.2. EFI_BOOT_SERVICES.FreePages()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-freepages) - fn free_pages(&self, address: usize, nb_pages: usize) -> Result<(), efi::Status>; + /// [UEFI Spec Documentation: 7.2.2. + /// EFI_BOOT_SERVICES.FreePages()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-freepages) + /// + /// # Safety + /// + /// `address` must be a valid physical address corresponding to a previously + /// allocated page range from [`Self::allocate_pages`]. The caller must have + /// exclusive ownership of the memory being freed. + unsafe fn free_pages(&self, address: usize, nb_pages: usize) -> Result<(), efi::Status>; /// Returns the current memory map. /// @@ -329,7 +336,14 @@ pub trait BootServices { /// Returns pool memory to the system. /// /// [UEFI Spec Documentation: 7.2.5. EFI_BOOT_SERVICES.FreePool()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-freepool) - fn free_pool(&self, buffer: *mut u8) -> Result<(), efi::Status>; + /// + /// # Safety + /// + /// `buffer` must be a valid pointer pointing to valid readable memory previously allocated from + /// [`Self::allocate_pool`]. This is needed to locate allocation signatures. The current + /// implementation does not track allocations, so the caller is responsible for ensuring the + /// pointer is valid. + unsafe fn free_pool(&self, buffer: *mut u8) -> Result<(), efi::Status>; /// Installs a protocol interface on a device handle. /// If the handle does not exist, it is created and added to the list of handles in the system. @@ -381,7 +395,8 @@ pub trait BootServices { /// # Safety /// /// When calling this method, you have to make sure that if *interface* pointer is non-null, it is adhereing to - /// the structure associated with the protocol. + /// the structure associated with the protocol. Throughout the lifetime of the interface reference, the caller + /// must ensure it remains valid. unsafe fn install_protocol_interface_unchecked( &self, handle: Option, @@ -683,6 +698,8 @@ pub trait BootServices { /// /// When calling this method, you have to make sure that *driver_image_handle*'s last entry is null per UEFI specification. /// + /// This function assumes that all driver bindings managing the controller remain valid for the duration of this call. + /// /// [UEFI Spec Documentation: 7.3.12. EFI_BOOT_SERVICES.ConnectController()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-connectcontroller) unsafe fn connect_controller( &self, @@ -694,8 +711,12 @@ pub trait BootServices { /// Disconnects one or more drivers from a controller. /// + /// # Safety + /// + /// This function assumes that all driver bindings managing the controller remain valid for the duration of this call. + /// /// [UEFI Spec Documentation: 7.3.13. EFI_BOOT_SERVICES.DisconnectController()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-disconnectcontroller) - fn disconnect_controller( + unsafe fn disconnect_controller( &self, controller_handle: efi::Handle, driver_image_handle: Option, @@ -812,32 +833,45 @@ pub trait BootServices { /// /// This uses [`Self::load_image`] behind the scene. This function assume that the request is not originating from the boot manager. /// - fn load_image_from_source( + /// # Safety + /// + /// If `device_path` is non-null, it must point to a valid UEFI device path in readable memory. + unsafe fn load_image_from_source( &self, parent_image_handle: efi::Handle, device_path: *mut efi::protocols::device_path::Protocol, source_buffer: &[u8], ) -> Result { - self.load_image(false, parent_image_handle, device_path, Some(source_buffer)) + // SAFETY: The caller guarantees `device_path` is null or valid. + unsafe { self.load_image(false, parent_image_handle, device_path, Some(source_buffer)) } } /// Load an EFI image from a file. /// /// This uses [`Self::load_image`] behind the scene. This function assume that the request is not originating from the boot manager. /// - fn load_image_from_file( + /// # Safety + /// + /// `file_device_path` must point to a valid UEFI device path in readable memory. + unsafe fn load_image_from_file( &self, parent_image_handle: efi::Handle, file_device_path: NonNull, ) -> Result { - self.load_image(false, parent_image_handle, file_device_path.as_ptr(), None) + // SAFETY: `file_device_path` is a `NonNull`, so `as_ptr()` yields a valid, non-null device + // path pointer as required by `load_image`. + unsafe { self.load_image(false, parent_image_handle, file_device_path.as_ptr(), None) } } /// Loads an EFI image into memory. /// /// [UEFI Spec Documentation: 7.4.1. EFI_BOOT_SERVICES.LoadImage()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-loadimage) /// - fn load_image<'a>( + /// # Safety + /// + /// If `device_path` is non-null, it must point to a valid UEFI device path in readable memory. + /// If `source_buffer` is provided, its contents must be a valid PE/COFF image. + unsafe fn load_image<'a>( &self, boot_policy: bool, parent_image_handle: efi::Handle, @@ -847,11 +881,15 @@ pub trait BootServices { /// Transfers control to a loaded image’s entry point. /// + /// [UEFI Spec Documentation: 7.4.2. EFI_BOOT_SERVICES.StartImage()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-startimage) /// - /// [UEFI Spec Documentation: 7.4.2. EFI_BOOT_SERVICES.StartImage()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-startimage) + /// # 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. #[allow(clippy::type_complexity)] - fn start_image<'a>( + unsafe fn start_image<'a>( &'a self, image_handle: efi::Handle, ) -> Result<(), (efi::Status, Option>)>; @@ -864,9 +902,17 @@ pub trait BootServices { /// Terminates a loaded EFI image and returns control to boot services. /// + /// `image_handle` is validated by the firmware; if not found, + /// an error is returned. + /// /// [UEFI Spec Documentation: 7.4.5. EFI_BOOT_SERVICES.Exit()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-exit) /// - fn exit<'a>( + /// # Safety + /// + /// If `image_handle` is valid and if `exit_data` is `Some`, the contained + /// buffer is passed to the firmware as a raw pointer and size. The caller + /// must ensure the buffer remains valid until the firmware consumes it. + unsafe fn exit<'a>( &'a self, image_handle: efi::Handle, exit_status: efi::Status, @@ -877,6 +923,13 @@ pub trait BootServices { /// /// [UEFI Spec Documentation: EFI_BOOT_SERVICES.ExitBootServices()](https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-exitbootservices) /// + /// # Safety Considerations + /// + /// This function is not marked `unsafe` because both parameters are safe to accept from + /// unverified sources: + /// - `image_handle` is passed through to the firmware which validates it internally. + /// - `map_key` is validated against the current memory map key; an invalid key causes the call + /// to fail gracefully with an error status rather than undefined behavior. fn exit_boot_services(&self, image_handle: efi::Handle, map_key: usize) -> Result<(), efi::Status>; /// Sets the system’s watchdog timer. @@ -987,6 +1040,11 @@ macro_rules! efi_boot_services_fn { } impl BootServices for StandardBootServices { + /// # Safety + /// + /// If `notify_context` is non-null, the caller must ensure it is properly aligned, + /// dereferenceable as type `T`, and remains a valid pointer for the entire lifetime of the + /// created event. unsafe fn create_event_unchecked( &self, event_type: EventType, @@ -1006,25 +1064,34 @@ impl BootServices for StandardBootServices { // verified (via CRC) before use with an error returned on mismatch. Present use cases for external modification // of boot services don't merit such complexity at this time. let create_event = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), create_event) }; - let status = create_event( - event_type.into(), - notify_tpl.into(), - // Safety: Transmuting function pointer types with matching ABIs and compatible signatures. - // Both are extern "efiapi" callbacks taking a context pointer - the generic parameter T - // is erased to c_void for the UEFI FFI interface. - unsafe { + + // SAFETY: `notify_context` validity is the caller's responsibility as documented by the `unsafe` on this + // function. `event` is a local `MaybeUninit` whose address is valid for writes. + let status = unsafe { + create_event( + event_type.into(), + notify_tpl.into(), + // Safety: Transmuting function pointer types with matching ABIs and compatible signatures. Both are + // extern "efiapi" callbacks taking a context pointer - the generic parameter T is erased to c_void for + // the UEFI FFI interface. Using unsafe blocks within an unsafe block triggers a warning; therefore, + // it is not used here. mem::transmute::< - Option, - Option, - >(notify_function) - }, - notify_context as *mut c_void, - event.as_mut_ptr(), - ); + Option, + Option, + >(notify_function), + notify_context as *mut c_void, + event.as_mut_ptr(), + ) + }; // SAFETY: If the UEFI call succeeded, event has been initialized by the firmware. if status.is_error() { Err(status) } else { Ok(unsafe { event.assume_init() }) } } + /// # Safety + /// + /// Same constraints as [`Self::create_event_unchecked`] apply. if `notify_context` is + /// non-null, it must be properly aligned, dereferenceable as type `T`, and must remain a + /// valid pointer for the entire lifetime of the created event. unsafe fn create_event_ex_unchecked( &self, event_type: EventType, @@ -1036,65 +1103,109 @@ impl BootServices for StandardBootServices { let mut event = MaybeUninit::zeroed(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let create_event_ex = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), create_event_ex) }; - let status = create_event_ex( - event_type.into(), - notify_tpl.into(), - // Safety: Transmuting function pointer types with matching ABIs and compatible signatures. - // Both are extern "efiapi" callbacks - the generic parameter T is erased to c_void for FFI. - unsafe { + + // SAFETY: `notify_context` validity is the caller's responsibility as documented by the + // `unsafe` on this function. `event_group` is a valid static reference cast to a pointer. + // `event` is a local `MaybeUninit` whose address is valid for writes. + let status = unsafe { + create_event_ex( + event_type.into(), + notify_tpl.into(), + // Safety: Transmuting function pointer types with matching ABIs and compatible signatures. Both are + // extern "efiapi" callbacks - the generic parameter T is erased to c_void for FFI. mem::transmute::< - Option, - Option, - >(notify_function) - }, - notify_context as *mut c_void, - event_group as *const _, - event.as_mut_ptr(), - ); + Option, + Option, + >(notify_function), + notify_context as *mut c_void, + event_group as *const _, + event.as_mut_ptr(), + ) + }; // SAFETY: If the call succeeded, it is considered initialized. if status.is_error() { Err(status) } else { Ok(unsafe { event.assume_init() }) } } + // This is triggered by the fact that efi::Event aliases to *mut c_void, but + // it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn close_event(&self, event: efi::Event) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let close_event = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), close_event) }; - match close_event(event) { + // SAFETY: This function is safe to call with an unverified `event` + // parameter. The underlying implementation looks up the event by ID in + // the database and returns a failure status if it is not found. The + // call requires an unsafe block because r-efi defines close_event as + // accepting an unsafe extern "efiapi" function pointer. + match unsafe { close_event(event) } { s if s.is_error() => Err(s), _ => Ok(()), } } + // This is triggered by the fact that efi::Event aliases to *mut c_void, but + // it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn signal_event(&self, event: efi::Event) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let signal_event = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), signal_event) }; - match signal_event(event) { + // SAFETY: This function is safe to call with an unverified `event` + // parameter. The underlying implementation looks up the event by ID in + // the database and returns a failure status if it is not found. The + // call requires an unsafe block because r-efi defines signal_event as + // accepting an unsafe extern "efiapi" function pointer. + match unsafe { signal_event(event) } { s if s.is_error() => Err(s), _ => Ok(()), } } + // This is triggered by the fact that efi::Event aliases to *mut c_void, but + // it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn wait_for_event(&self, events: &mut [efi::Event]) -> Result { let mut index = MaybeUninit::zeroed(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let wait_for_event = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), wait_for_event) }; - let status = wait_for_event(events.len(), events.as_mut_ptr(), index.as_mut_ptr()); + // SAFETY: This function is safe to call with an unverified `event` + // parameter. The underlying implementation looks up the event by ID in + // the database and returns a failure status if it is not found. The + // call requires an unsafe block because r-efi defines wait_for_event as + // accepting an unsafe extern "efiapi" function pointer. + let status = unsafe { wait_for_event(events.len(), events.as_mut_ptr(), index.as_mut_ptr()) }; // SAFETY: If the call succeeded, index has been initialized with the event index. if status.is_error() { Err(status) } else { Ok(unsafe { index.assume_init() }) } } + // This is triggered by the fact that efi::Event aliases to *mut c_void, but + // it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn check_event(&self, event: efi::Event) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let check_event = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), check_event) }; - match check_event(event) { + // SAFETY: This function is safe to call with an unverified `event` + // parameter. The underlying implementation looks up the event by ID in + // the database and returns a failure status if it is not found. The + // call requires an unsafe block because r-efi defines check_event as + // accepting an unsafe extern "efiapi" function pointer. + match unsafe { check_event(event) } { s if s.is_error() => Err(s), _ => Ok(()), } } + // This is triggered by the fact that efi::Event aliases to *mut c_void, but + // it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn set_timer(&self, event: efi::Event, timer_type: EventTimerType, trigger_time: u64) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let set_timer = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), set_timer) }; - match set_timer(event, timer_type.into(), trigger_time) { + // SAFETY: This function is safe to call with an unverified `event` + // parameter. The underlying implementation looks up the event by ID in + // the database and returns a failure status if it is not found. The + // call requires an unsafe block because r-efi defines set_timer as + // accepting an unsafe extern "efiapi" function pointer. + match unsafe { set_timer(event, timer_type.into(), trigger_time) } { s if s.is_error() => Err(s), _ => Ok(()), } @@ -1103,13 +1214,17 @@ impl BootServices for StandardBootServices { fn raise_tpl(&self, new_tpl: Tpl) -> Tpl { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let raise_tpl = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), raise_tpl) }; - raise_tpl(new_tpl.into()).into() + // SAFETY: The underlying EFI call is safe to call with any TPL value. + // The function will return the previous TPL value, which is safe to + // convert into the Rust Tpl type. + unsafe { raise_tpl(new_tpl.into()).into() } } fn restore_tpl(&self, old_tpl: Tpl) { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let restore_tpl = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), restore_tpl) }; - restore_tpl(old_tpl.into()) + // SAFETY: The underlying EFI call is safe to call with any TPL value. + unsafe { restore_tpl(old_tpl.into()) } } fn allocate_pages( @@ -1125,26 +1240,46 @@ impl BootServices for StandardBootServices { }; // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let allocate_pages = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), allocate_pages) }; - match allocate_pages( - alloc_type.into(), - memory_type.into(), - nb_pages, - ptr::addr_of_mut!(memory_address) as *mut u64, - ) { - s if s.is_error() => Err(s), + + // SAFETY: Although the current function `allocate_pages` is a safe Rust wrapper, the + // address contained in `AllocType::Address` or `AllocType::MaxAddress` will be dereferenced + // by the `core_allocate_pages()` during allocation. It is the caller's responsibility to + // ensure that any address provided in `alloc_type` is valid. + let status = unsafe { + allocate_pages( + alloc_type.into(), + memory_type.into(), + nb_pages, + ptr::addr_of_mut!(memory_address) as *mut u64, + ) + }; + match status { + status if status.is_error() => Err(status), _ => Ok(memory_address), } } - fn free_pages(&self, address: usize, nb_pages: usize) -> Result<(), efi::Status> { + /// # Safety + /// + /// `address` must be a valid physical address corresponding to a previously allocated page + /// range from [`BootServices::allocate_pages`], and `nb_pages` must match the number of pages + /// originally allocated at that address. The caller must have exclusive ownership of the memory + /// being freed and must not access it after this call. + unsafe fn free_pages(&self, address: usize, nb_pages: usize) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let free_pages = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), free_pages) }; - match free_pages(address as u64, nb_pages) { - s if s.is_error() => Err(s), + // SAFETY: `address` must be a valid physical address corresponding to a previously allocated + // page range. The caller must have exclusive ownership of the memory being freed. + match unsafe { free_pages(address as u64, nb_pages) } { + status if status.is_error() => Err(status), _ => Ok(()), } } + /// This is a pure Rust function that does not rely on external input. All + /// pointer arguments to the underlying `get_memory_map` call are derived + /// from local variables, so the caller does not need to uphold any safety + /// invariants. fn get_memory_map(&self) -> Result, (efi::Status, usize)> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let get_memory_map = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), get_memory_map) }; @@ -1154,26 +1289,35 @@ impl BootServices for StandardBootServices { let mut descriptor_size = 0; let mut descriptor_version = 0; - match get_memory_map( - ptr::addr_of_mut!(memory_map_size), - ptr::null_mut(), - ptr::addr_of_mut!(map_key), - ptr::addr_of_mut!(descriptor_size), - ptr::addr_of_mut!(descriptor_version), - ) { + // SAFETY: All pointer arguments are derived from local variables declared above, + // so they are guaranteed to be valid and writable. The memory_map pointer is null, + // which is valid for a size-query call. + match unsafe { + get_memory_map( + ptr::addr_of_mut!(memory_map_size), + ptr::null_mut(), + ptr::addr_of_mut!(map_key), + ptr::addr_of_mut!(descriptor_size), + ptr::addr_of_mut!(descriptor_version), + ) + } { s if s == efi::Status::BUFFER_TOO_SMALL => memory_map_size += 0x400, // add more space in case allocation makes the memory map bigger. _ => (), }; let buffer = self.allocate_pool(EfiMemoryType::BootServicesData, memory_map_size).map_err(|s| (s, 0))?; - match get_memory_map( - ptr::addr_of_mut!(memory_map_size), - buffer as *mut _, - ptr::addr_of_mut!(map_key), - ptr::addr_of_mut!(descriptor_size), - ptr::addr_of_mut!(descriptor_version), - ) { + // SAFETY: All pointer arguments are derived from local variables. The memory_map + // buffer was allocated via allocate_pool above with sufficient size for the map. + match unsafe { + get_memory_map( + ptr::addr_of_mut!(memory_map_size), + buffer as *mut _, + ptr::addr_of_mut!(map_key), + ptr::addr_of_mut!(descriptor_size), + ptr::addr_of_mut!(descriptor_version), + ) + } { s if s == efi::Status::BUFFER_TOO_SMALL => return Err((s, memory_map_size)), s if s.is_error() => return Err((s, 0)), _ => (), @@ -1191,21 +1335,34 @@ impl BootServices for StandardBootServices { let mut buffer = ptr::null_mut(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let allocate_pool = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), allocate_pool) }; - match allocate_pool(memory_type.into(), size, ptr::addr_of_mut!(buffer)) { - s if s.is_error() => Err(s), + // SAFETY: buffer is declared above, we pass the address which guarantees it is a valid pointer. + // still needed because the API itself is declared unsafe in r-efi. + let status = unsafe { allocate_pool(memory_type.into(), size, ptr::addr_of_mut!(buffer)) }; + match status { + status if status.is_error() => Err(status), _ => Ok(buffer as *mut u8), } } - fn free_pool(&self, buffer: *mut u8) -> Result<(), efi::Status> { + /// # Safety + /// + /// `buffer` must be a valid pointer pointing to valid readable memory previously allocated from + /// [`BootServices::allocate_pool`]. The caller must have exclusive ownership of the allocation + /// being freed and must not use `buffer` after this call. + unsafe fn free_pool(&self, buffer: *mut u8) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let free_pool = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), free_pool) }; - match free_pool(buffer as *mut c_void) { - s if s.is_error() => Err(s), + // SAFETY: The caller is responsible for ensuring `buffer` points to a valid allocation. + match unsafe { free_pool(buffer as *mut c_void) } { + status if status.is_error() => Err(status), _ => Ok(()), } } + /// # Safety + /// + /// Throughout the lifetime of the installed interface, the caller must ensure + /// that `interface` remains valid. unsafe fn install_protocol_interface_unchecked( &self, handle: Option, @@ -1216,17 +1373,28 @@ impl BootServices for StandardBootServices { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let install_protocol_interface = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), install_protocol_interface) }; - match install_protocol_interface( - ptr::addr_of_mut!(handle), - protocol as *const _ as *mut _, - efi::NATIVE_INTERFACE, - interface, - ) { - s if s.is_error() => Err(s), + // SAFETY: `handle` is a local variable whose address is valid. `protocol` is a valid + // reference cast to a pointer. `interface` validity is the caller's responsibility as + // documented by the `unsafe` on this function. + let status = unsafe { + install_protocol_interface( + ptr::addr_of_mut!(handle), + protocol as *const _ as *mut _, + efi::NATIVE_INTERFACE, + interface, + ) + }; + match status { + status if status.is_error() => Err(status), _ => Ok(handle), } } + /// # Safety + /// + /// `interface` must be a valid pointer and be of the type expected by the + /// protocol. `handle` must refer to a handle on which `protocol` was + /// previously installed with `interface`. unsafe fn uninstall_protocol_interface_unchecked( &self, handle: efi::Handle, @@ -1236,12 +1404,21 @@ impl BootServices for StandardBootServices { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let uninstall_protocol_interface = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), uninstall_protocol_interface) }; - match uninstall_protocol_interface(handle, protocol as *const _ as *mut _, interface) { - s if s.is_error() => Err(s), + + // SAFETY: `protocol` is a valid reference cast to a pointer. `interface` is not + // dereferenced by the implementation. `handle` validity is the caller's responsibility + // as documented by the `unsafe` on this function. + let status = unsafe { uninstall_protocol_interface(handle, protocol as *const _ as *mut _, interface) }; + match status { + status if status.is_error() => Err(status), _ => Ok(()), } } + /// # Safety + /// + /// Throughout the lifetime of the installed interface, the caller must ensure + /// that `new_protocol_interface` remains valid. unsafe fn reinstall_protocol_interface_unchecked( &self, handle: efi::Handle, @@ -1252,23 +1429,38 @@ impl BootServices for StandardBootServices { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let reinstall_protocol_interface = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), reinstall_protocol_interface) }; - match reinstall_protocol_interface( - handle, - protocol as *const _ as *mut _, - old_protocol_interface, - new_protocol_interface, - ) { - s if s.is_error() => Err(s), + // SAFETY: `protocol` is a valid reference cast to a pointer. `old_protocol_interface` and + // `new_protocol_interface` validity is the caller's responsibility as documented by the + // `unsafe` on this function. + let status = unsafe { + reinstall_protocol_interface( + handle, + protocol as *const _ as *mut _, + old_protocol_interface, + new_protocol_interface, + ) + }; + match status { + status if status.is_error() => Err(status), _ => Ok(()), } } + // This is triggered by the fact that efi::Event aliases to *mut c_void, but + // it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn register_protocol_notify(&self, protocol: &efi::Guid, event: efi::Event) -> Result { let mut registration = MaybeUninit::uninit(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let register_protocol_notify = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), register_protocol_notify) }; - match register_protocol_notify(protocol as *const _ as *mut _, event, registration.as_mut_ptr() as *mut _) { - s if s.is_error() => Err(s), + + // SAFETY: `protocol` is a valid reference cast to a pointer. `event` is validated by the + // implementation. `registration` is a local `MaybeUninit` whose address is valid for writes. + let status = unsafe { + register_protocol_notify(protocol as *const _ as *mut _, event, registration.as_mut_ptr() as *mut _) + }; + match status { + status if status.is_error() => Err(status), // SAFETY: If the call succeeded, registration has been initialized. _ => Ok(unsafe { registration.assume_init() }), } @@ -1292,22 +1484,34 @@ impl BootServices for StandardBootServices { // Expect locate_handle to return BUFFER_TOO_SMALL along with the proper buffer_size let mut buffer_size = 0; - match locate_handle(search_type.into(), protocol, search_key, ptr::addr_of_mut!(buffer_size), ptr::null_mut()) { - s if s == efi::Status::BUFFER_TOO_SMALL => (), - s if s.is_error() => return Err(s), + // SAFETY: `protocol` and `search_key` are derived from `search_type` above. + // `buffer_size` is a local variable whose address is valid. A null handle buffer is + // valid for a size-query call that is expected to return BUFFER_TOO_SMALL. + let status = unsafe { + locate_handle(search_type.into(), protocol, search_key, ptr::addr_of_mut!(buffer_size), ptr::null_mut()) + }; + match status { + status if status == efi::Status::BUFFER_TOO_SMALL => (), + status if status.is_error() => return Err(status), _ => (), } let buffer = self.allocate_pool(EfiMemoryType::BootServicesData, buffer_size)?; - match locate_handle( - search_type.into(), - protocol, - search_key, - ptr::addr_of_mut!(buffer_size), - buffer as *mut efi::Handle, - ) { - s if s.is_error() => Err(s), + // SAFETY: `protocol` and `search_key` are unchanged from the size query above. + // `buffer_size` is a local variable whose address is valid. `buffer` was allocated + // via `allocate_pool` with sufficient size to hold the handles. + let status = unsafe { + locate_handle( + search_type.into(), + protocol, + search_key, + ptr::addr_of_mut!(buffer_size), + buffer as *mut efi::Handle, + ) + }; + match status { + status if status.is_error() => Err(status), // SAFETY: buffer was allocated with allocate_pool to hold buffer_size bytes. // The number of handles is buffer_size divided by the size of each handle. _ => Ok(unsafe { @@ -1316,6 +1520,11 @@ impl BootServices for StandardBootServices { } } + /// # Safety + /// + /// The caller must cast the returned pointer to the correct protocol + /// interface type corresponding to `protocol`. Using an incorrect type + /// leads to undefined behavior when the pointer is dereferenced. unsafe fn handle_protocol_unchecked( &self, handle: efi::Handle, @@ -1324,12 +1533,22 @@ impl BootServices for StandardBootServices { let mut interface = ptr::null_mut(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let handle_protocol = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), handle_protocol) }; - match handle_protocol(handle, protocol as *const _ as *mut _, ptr::addr_of_mut!(interface)) { - s if s.is_error() => Err(s), + // SAFETY: `handle` is validated by the implementation and returns an error if not found. + // `protocol` is a valid reference cast to a pointer. `interface` is a local variable + // whose address is valid for writes. The returned `interface` pointer is the caller's + // responsibility to use correctly, as documented by the `unsafe` on this function. + let status = unsafe { handle_protocol(handle, protocol as *const _ as *mut _, ptr::addr_of_mut!(interface)) }; + match status { + status if status.is_error() => Err(status), _ => Ok(interface), } } + /// # Safety + /// + /// `device_path` must point to a valid `*mut DevicePath` pointer in writable memory. + /// On entry the pointer must be a valid device path; on return it is updated to the + /// remaining (unmatched) portion of the path. unsafe fn locate_device_path( &self, protocol: &efi::Guid, @@ -1338,12 +1557,21 @@ impl BootServices for StandardBootServices { let mut device = ptr::null_mut(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let locate_device_path = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), locate_device_path) }; - match locate_device_path(protocol as *const _ as *mut _, device_path, ptr::addr_of_mut!(device)) { - s if s.is_error() => Err(s), + // SAFETY: `protocol` is a valid reference cast to a pointer. `device_path` validity is + // the caller's responsibility as documented by the `unsafe` on this function. `device` is + // a local variable whose address is valid for writes. + let status = + unsafe { locate_device_path(protocol as *const _ as *mut _, device_path, ptr::addr_of_mut!(device)) }; + match status { + status if status.is_error() => Err(status), _ => Ok(device), } } + /// # Safety + /// + /// The caller must cast the returned pointer to the correct protocol interface type + /// corresponding to `protocol`. `agent_handle` must be a valid handle or null. unsafe fn open_protocol_unchecked( &self, handle: efi::Handle, @@ -1355,19 +1583,32 @@ impl BootServices for StandardBootServices { let mut interface = ptr::null_mut(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let open_protocol = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), open_protocol) }; - match open_protocol( - handle, - protocol as *const _ as *mut _, - ptr::addr_of_mut!(interface), - agent_handle, - controller_handle, - attribute, - ) { - s if s.is_error() => Err(s), + // SAFETY: `protocol` is a valid reference cast to a pointer. `interface` is a local + // variable whose address is valid for writes. `handle`, `agent_handle`, and + // `controller_handle` validity is the caller's responsibility as documented by the + // `unsafe` on this function. + let status = unsafe { + open_protocol( + handle, + protocol as *const _ as *mut _, + ptr::addr_of_mut!(interface), + agent_handle, + controller_handle, + attribute, + ) + }; + match status { + status if status.is_error() => Err(status), _ => Ok(interface), } } + /// Not marked `unsafe` because `protocol` is a Rust reference and is therefore guaranteed + /// to be valid. All other parameters are opaque handles validated internally by the + /// implementation. + // This below lint is triggered by the fact that efi::Handle aliases to *mut + // c_void, but it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn close_protocol( &self, handle: efi::Handle, @@ -1377,12 +1618,22 @@ impl BootServices for StandardBootServices { ) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let close_protocol = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), close_protocol) }; - match close_protocol(handle, protocol as *const _ as *mut _, agent_handle, controller_handle) { - s if s.is_error() => Err(s), + // SAFETY: `protocol` is a valid Rust reference cast to a pointer. `handle`, + // `agent_handle`, and `controller_handle` are opaque handles validated internally + // by the implementation; invalid handles result in an error status, not UB. + let status = unsafe { close_protocol(handle, protocol as *const _ as *mut _, agent_handle, controller_handle) }; + match status { + status if status.is_error() => Err(status), _ => Ok(()), } } + /// Not marked `unsafe` because `protocol` is a Rust reference and is therefore guaranteed + /// to be valid. `handle` is an opaque handle validated internally by the implementation; + /// an invalid handle results in an error status, not undefined behavior. + /// This is triggered by the fact that efi::Handle aliases to *mut c_void, but + /// it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn open_protocol_information( &self, handle: efi::Handle, @@ -1395,19 +1646,36 @@ impl BootServices for StandardBootServices { let mut entry_count = 0; // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let open_protocol_information = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), open_protocol_information) }; - match open_protocol_information( - handle, - protocol as *const _ as *mut _, - ptr::addr_of_mut!(entry_buffer), - ptr::addr_of_mut!(entry_count), - ) { - s if s.is_error() => Err(s), + // SAFETY: `protocol` is a valid Rust reference cast to a pointer. `entry_buffer` and + // `entry_count` are local variables whose addresses are valid for writes. `handle` is + // validated internally by the implementation; an invalid handle results in an error status, + // not UB. The unsafe block is required because r-efi declares open_protocol_information as + // an unsafe extern "efiapi" function pointer. + let status = unsafe { + open_protocol_information( + handle, + protocol as *const _ as *mut _, + ptr::addr_of_mut!(entry_buffer), + ptr::addr_of_mut!(entry_count), + ) + }; + match status { + status if status.is_error() => Err(status), // SAFETY: The firmware allocates entry_buffer and sets entry_count. // from_raw_parts_mut creates a slice with the proper length as specified by the firmware. _ => Ok(unsafe { BootServicesBox::from_raw_parts_mut(entry_buffer, entry_count, self) }), } } + /// # Safety + /// + /// The caller must ensure that: + /// - If `remaining_device_path` is non-null, it must point to a valid + /// device path structure. + /// - The other parameters are either native Rust types or opaque UEFI handles + /// wrapped inside Rust types. Passing an invalid value does not by itself + /// cause undefined behavior; the firmware is expected to reject it by + /// returning an error status. unsafe fn connect_controller( &self, controller_handle: efi::Handle, @@ -1423,13 +1691,24 @@ impl BootServices for StandardBootServices { }; // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let connect_controller = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), connect_controller) }; - match connect_controller(controller_handle, driver_image_handles, remaining_device_path, recursive.into()) { - s if s.is_error() => Err(s), + // SAFETY: The caller must ensure the function's safety contract. + let status = unsafe { + connect_controller(controller_handle, driver_image_handles, remaining_device_path, recursive.into()) + }; + match status { + status if status.is_error() => Err(status), _ => Ok(()), } } - fn disconnect_controller( + /// # Safety + /// + /// - The parameters are either native Rust types or opaque UEFI handles + /// wrapped inside Rust types. Passing an invalid value does not by itself + /// cause undefined behavior; the firmware is expected to reject it by + /// returning an error status. The function is marked `unsafe` due to the + /// underlying contract of core_disconnect_controller() implementation. + unsafe fn disconnect_controller( &self, controller_handle: efi::Handle, driver_image_handle: Option, @@ -1437,16 +1716,31 @@ impl BootServices for StandardBootServices { ) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let disconnect_controller = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), disconnect_controller) }; - match disconnect_controller( - controller_handle, - driver_image_handle.unwrap_or(ptr::null_mut()), - child_handle.unwrap_or(ptr::null_mut()), - ) { - s if s.is_error() => Err(s), + + // SAFETY: `controller_handle` is an opaque handle validated internally by the + // implementation. `driver_image_handle` and `child_handle` are unwrapped to raw + // pointers (null if `None`), which the firmware accepts per the UEFI spec. + let status = unsafe { + disconnect_controller( + controller_handle, + driver_image_handle.unwrap_or(ptr::null_mut()), + child_handle.unwrap_or(ptr::null_mut()), + ) + }; + + match status { + status if status.is_error() => Err(status), _ => Ok(()), } } + /// Not marked `unsafe` because `handle` is an opaque handle validated internally by the + /// implementation; an invalid handle results in an error status, not undefined behavior. + /// The output pointers are derived from local variables. + /// + /// This is triggered by the fact that efi::Event aliases to *mut c_void, but + /// it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn protocols_per_handle( &self, handle: efi::Handle, @@ -1455,9 +1749,15 @@ impl BootServices for StandardBootServices { let mut protocol_buffer_count = 0; // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let protocols_per_handle = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), protocols_per_handle) }; - match protocols_per_handle(handle, ptr::addr_of_mut!(protocol_buffer), ptr::addr_of_mut!(protocol_buffer_count)) - { - s if s.is_error() => Err(s), + + // SAFETY: `handle` is validated internally by the implementation. `protocol_buffer` and + // `protocol_buffer_count` are local variables whose addresses are valid for writes. + let status = unsafe { + protocols_per_handle(handle, ptr::addr_of_mut!(protocol_buffer), ptr::addr_of_mut!(protocol_buffer_count)) + }; + + match status { + status if status.is_error() => Err(status), // SAFETY: The firmware allocates protocol_buffer and sets protocol_buffer_count. // from_raw_parts_mut creates a slice with the proper length as specified. _ => Ok(unsafe { @@ -1466,6 +1766,9 @@ impl BootServices for StandardBootServices { } } + /// Not marked `unsafe` because all pointer arguments to the underlying call are derived + /// from local variables or safe Rust types (`HandleSearchType`). The output buffer and count + /// are allocated by the firmware and wrapped in a `BootServicesBox` before being returned. fn locate_handle_buffer( &self, search_type: HandleSearchType, @@ -1485,14 +1788,20 @@ impl BootServices for StandardBootServices { }; // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let locate_handle_buffer = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), locate_handle_buffer) }; - match locate_handle_buffer( - search_type.into(), - protocol, - search_key, - ptr::addr_of_mut!(buffer_count), - ptr::addr_of_mut!(buffer), - ) { - s if s.is_error() => Err(s), + // SAFETY: `protocol` and `search_key` are derived from the safe `HandleSearchType` enum. + // `buffer_count` and `buffer` are local variables whose addresses are valid for writes. + // The firmware populates them on success. + let status = unsafe { + locate_handle_buffer( + search_type.into(), + protocol, + search_key, + ptr::addr_of_mut!(buffer_count), + ptr::addr_of_mut!(buffer), + ) + }; + match status { + status if status.is_error() => Err(status), // SAFETY: The firmware allocates buffer and sets buffer_count. // from_raw_parts_mut creates a slice with the proper length as specified. _ => Ok(unsafe { @@ -1501,6 +1810,11 @@ impl BootServices for StandardBootServices { } } + /// # Safety + /// + /// `registration` must be null or a valid pointer obtained from a prior call to + /// `register_protocol_notify`. The caller must cast the returned pointer to the correct + /// protocol interface type corresponding to `protocol`. unsafe fn locate_protocol_unchecked( &self, protocol: &'static efi::Guid, @@ -1509,13 +1823,22 @@ impl BootServices for StandardBootServices { let mut interface = ptr::null_mut(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let locate_protocol = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), locate_protocol) }; - match locate_protocol(protocol as *const _ as *mut _, registration, ptr::addr_of_mut!(interface)) { - s if s.is_error() => Err(s), + // SAFETY: `protocol` is a valid Rust reference cast to a pointer. `registration` validity + // is the caller's responsibility as documented by the `unsafe` on this function. + // `interface` is a local variable whose address is valid for writes. + let status = + unsafe { locate_protocol(protocol as *const _ as *mut _, registration, ptr::addr_of_mut!(interface)) }; + match status { + status if status.is_error() => Err(status), _ => Ok(interface), } } - fn load_image( + /// # Safety + /// + /// If `device_path` is non-null, it must point to a valid UEFI device path in readable memory. + /// If `source_buffer` is provided, its contents must be a valid PE/COFF image. + unsafe fn load_image( &self, boot_policy: bool, parent_image_handle: efi::Handle, @@ -1528,21 +1851,34 @@ impl BootServices for StandardBootServices { let mut image_handle = MaybeUninit::uninit(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let load_image = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), load_image) }; - match load_image( - boot_policy.into(), - parent_image_handle, - device_path, - source_buffer_ptr, - source_buffer_size, - image_handle.as_mut_ptr(), - ) { + // SAFETY: The caller guarantees that `device_path` is null or points to a valid device path. + // `source_buffer_ptr` and `source_buffer_size` are derived from the `Option<&[u8]>` slice + // which guarantees validity when `Some`. `image_handle` is an out-parameter written by the + // firmware on success and consumed via `assume_init` only after a success check. + let status = unsafe { + load_image( + boot_policy.into(), + parent_image_handle, + device_path, + source_buffer_ptr, + source_buffer_size, + image_handle.as_mut_ptr(), + ) + }; + + match status { s if s.is_error() => Err(s), // SAFETY: If the call succeeded, image_handle has been initialized. _ => Ok(unsafe { image_handle.assume_init() }), } } - fn start_image( + /// # 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. + unsafe fn start_image( &self, image_handle: efi::Handle, ) -> Result<(), (efi::Status, Option>)> { @@ -1550,8 +1886,12 @@ impl BootServices for StandardBootServices { let mut exit_data = MaybeUninit::uninit(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let start_image = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), start_image) }; - match start_image(image_handle, exit_data_size.as_mut_ptr(), exit_data.as_mut_ptr()) { - s if s.is_error() => { + // SAFETY: The caller guarantees that `image_handle` refers to a valid loaded image. + // `exit_data_size` and `exit_data` are out-parameters written by the firmware; they are + // only consumed via `assume_init` after a null/success check. + let status = unsafe { start_image(image_handle, exit_data_size.as_mut_ptr(), exit_data.as_mut_ptr()) }; + match status { + status if status.is_error() => { // SAFETY: If exit_data pointer is not null, it points to valid memory. // exit_data_size contains the size of the allocated data. from_raw_parts_mut creates a proper slice. let data = (!exit_data.as_ptr().is_null()).then(|| unsafe { @@ -1561,22 +1901,41 @@ impl BootServices for StandardBootServices { self, ) }); - Err((s, data)) + Err((status, data)) } _ => Ok(()), } } + /// This is triggered by the fact that efi::Event aliases to *mut c_void, but + /// it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn unload_image(&self, image_handle: efi::Handle) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let unload_image = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), unload_image) }; - match unload_image(image_handle) { - s if s.is_error() => Err(s), + // SAFETY: The unsafe block is required because r-efi declares + // unload_image as an unsafe extern "efiapi" function pointer. The + // Patina implementation is fully safe: `image_handle` is validated + // internally and an invalid handle results in an error status rather + // than undefined behavior. + match unsafe { unload_image(image_handle) } { + status if status.is_error() => Err(status), _ => Ok(()), } } - fn exit<'a>( + /// Terminates a started image and returns control to the caller of + /// `start_image`. + /// + /// `image_handle` is validated against the private image data map; if not + /// found, `INVALID_PARAMETER` is returned. + /// + /// # Safety + /// + /// If `image_handle` is valid and if `exit_data` is `Some`, the contained + /// buffer is passed to the firmware as a raw pointer and size. The caller + /// must ensure the buffer remains valid until the firmware consumes it. + unsafe fn exit<'a>( &'a self, image_handle: efi::Handle, exit_status: efi::Status, @@ -1586,26 +1945,58 @@ impl BootServices for StandardBootServices { let exit_data_size = exit_data.as_ref().map_or(0, |data| data.len()); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let exit = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), exit) }; - match exit(image_handle, exit_status, exit_data_size, exit_data_ptr) { - s if s.is_error() => Err(s), + // SAFETY: `image_handle` is validated by the firmware. When `image_handle` is valid and + // `exit_data` is `Some`, the caller must ensure the contained buffer remains valid until + // the firmware consumes it. + let status = unsafe { exit(image_handle, exit_status, exit_data_size, exit_data_ptr) }; + match status { + status if status.is_error() => Err(status), _ => Ok(()), } } + /// Terminates all boot services. + /// + /// # Safety Considerations + /// + /// This function is not marked `unsafe` because both parameters are safe to accept from + /// unverified sources: + /// - `image_handle` is passed through to the firmware which validates it internally. + /// - `map_key` is validated against the current memory map key; an invalid key causes the call + /// to fail gracefully with an error status rather than undefined behavior. + // This is triggered by the fact that efi::Event aliases to *mut c_void, but + // it is an opaque handle used as a database key. + #[allow(clippy::not_unsafe_ptr_arg_deref)] fn exit_boot_services(&self, image_handle: efi::Handle, map_key: usize) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let exit_boot_services = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), exit_boot_services) }; - match exit_boot_services(image_handle, map_key) { + + // SAFETY: The unsafe block is required because r-efi declares exit_boot_services as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe: both + // `image_handle` and `map_key` are validated internally and invalid values result in an + // error status rather than undefined behavior. + match unsafe { exit_boot_services(image_handle, map_key) } { s if s.is_error() => Err(s), _ => Ok(()), } } + /// Sets the system's watchdog timer. + /// + /// # Safety Considerations + /// + /// This function is not marked `unsafe` because `timeout` is safe to accept from unverified + /// sources. The implementation uses saturating arithmetic when converting the timeout value, + /// and will not cause undefined behavior. fn set_watchdog_timer(&self, timeout: usize) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let set_watchdog_timer = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), set_watchdog_timer) }; - match set_watchdog_timer(timeout, 0, 0, ptr::null_mut()) { - s if s.is_error() => Err(s), + + // SAFETY: The unsafe block is required because r-efi declares set_watchdog_timer as an + // unsafe extern "efiapi" function pointer. The Patina implementation is fully safe. + let status = unsafe { set_watchdog_timer(timeout, 0, 0, ptr::null_mut()) }; + match status { + status if status.is_error() => Err(status), _ => Ok(()), } } @@ -1613,8 +2004,10 @@ impl BootServices for StandardBootServices { fn stall(&self, microseconds: usize) -> Result<(), efi::Status> { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let stall = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), stall) }; - match stall(microseconds) { - s if s.is_error() => Err(s), + // SAFETY: The underlying EFI call is safe to call with any microsecond value. + let status = unsafe { stall(microseconds) }; + match status { + status if status.is_error() => Err(status), _ => Ok(()), } } @@ -1622,21 +2015,28 @@ impl BootServices for StandardBootServices { unsafe fn copy_mem_unchecked(&self, dest: *mut c_void, src: *const c_void, length: usize) { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let copy_mem = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), copy_mem) }; - copy_mem(dest, src as *mut _, length); + // SAFETY: caller must ensure that the source and destination are valid for length bytes. + unsafe { copy_mem(dest, src as *mut _, length) }; } fn set_mem(&self, buffer: &mut [u8], value: u8) { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let set_mem = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), set_mem) }; - set_mem(buffer.as_mut_ptr() as *mut c_void, buffer.len(), value); + // SAFETY: The parameters are safe because `buffer` is a mutable slice + // with an associated length, so the pointer and size are guaranteed + // valid and cannot cause undefined behavior. + unsafe { set_mem(buffer.as_mut_ptr() as *mut c_void, buffer.len(), value) }; } fn get_next_monotonic_count(&self) -> Result { let mut count = MaybeUninit::uninit(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let get_next_monotonic_count = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), get_next_monotonic_count) }; - match get_next_monotonic_count(count.as_mut_ptr()) { - s if s.is_error() => Err(s), + // SAFETY: The unsafe block is required because r-efi declares get_next_monotonic_count as + // an unsafe extern "efiapi" function pointer. The Patina do not provide an implementation + // for this function. + match unsafe { get_next_monotonic_count(count.as_mut_ptr()) } { + status if status.is_error() => Err(status), // SAFETY: If the UEFI call succeeded, count has been initialized. _ => Ok(unsafe { count.assume_init() }), } @@ -1650,8 +2050,10 @@ impl BootServices for StandardBootServices { // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let install_configuration_table = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), install_configuration_table) }; - match install_configuration_table(guid as *const _ as *mut _, table) { - s if s.is_error() => Err(s), + // SAFETY: The caller already passed the guid input as a valid Rust reference, table itself + // is not dereferenced. + match unsafe { install_configuration_table(guid as *const _ as *mut _, table) } { + status if status.is_error() => Err(status), _ => Ok(()), } } @@ -1660,8 +2062,10 @@ impl BootServices for StandardBootServices { let mut crc32 = MaybeUninit::uninit(); // SAFETY: See safety comment in create_event_unchecked for details on corner cases around external modifications. let calculate_crc32 = unsafe { efi_boot_services_fn!(*self.as_mut_ptr(), calculate_crc32) }; - match calculate_crc32(data as *mut _, data_size, crc32.as_mut_ptr()) { - s if s.is_error() => Err(s), + // SAFETY: The caller is responsible for ensuring that `data` points to valid, readable + // memory of at least `data_size` bytes. + match unsafe { calculate_crc32(data as *mut _, data_size, crc32.as_mut_ptr()) } { + status if status.is_error() => Err(status), // SAFETY: If the call succeeded, crc32 has been initialized. _ => Ok(unsafe { crc32.assume_init() }), } @@ -1788,13 +2192,16 @@ mod tests { assert_eq!(efi::TPL_APPLICATION, notify_tpl); // Safety: Test code - transmute from Option function pointer to raw pointer for comparison. assert_eq!(notify_callback as *const fn(), unsafe { - mem::transmute::, *const fn()>(notify_function) + mem::transmute::, *const fn()>( + notify_function, + ) }); assert_ne!(ptr::null_mut(), notify_context); assert_ne!(ptr::null_mut(), event); if let Some(notify_function) = notify_function { - notify_function(ptr::null_mut(), notify_context); + // SAFETY: Test code - invoking the notify callback to verify it was registered correctly. + unsafe { notify_function(ptr::null_mut(), notify_context) }; } efi::Status::SUCCESS } @@ -1863,14 +2270,17 @@ mod tests { assert_eq!(efi::TPL_APPLICATION, notify_tpl); // Safety: Test code - transmute from Option function pointer to raw pointer for comparison. assert_eq!(notify_callback as *const fn(), unsafe { - mem::transmute::, *const fn()>(notify_function) + mem::transmute::, *const fn()>( + notify_function, + ) }); assert_ne!(ptr::null(), notify_context); assert_eq!(ptr::addr_of!(GUID), event_group); assert_ne!(ptr::null_mut(), event); if let Some(notify_function) = notify_function { - notify_function(ptr::null_mut(), notify_context as *mut _); + // SAFETY: Test code - invoking the notify callback to verify it was registered correctly. + unsafe { notify_function(ptr::null_mut(), notify_context as *mut _) }; } efi::Status::SUCCESS } @@ -2100,6 +2510,7 @@ mod tests { #[should_panic = "Boot services function allocate_pages is not initialized."] fn test_allocate_pages_not_init() { let boot_services = boot_services!(); + // The call is expected to panic before reaching the FFI layer because `allocate_pages` is not initialized. let _ = boot_services.allocate_pages(AllocType::AnyPage, EfiMemoryType::ACPIMemoryNVS, 0); } @@ -2160,7 +2571,8 @@ mod tests { #[should_panic = "Boot services function free_pages is not initialized."] fn test_free_pages_not_init() { let boot_services = boot_services!(); - _ = boot_services.free_pages(0, 0); + // SAFETY: parameters are dummy values; the call is expected to panic before reaching the FFI layer. + _ = unsafe { boot_services.free_pages(0, 0) }; } #[test] @@ -2174,7 +2586,8 @@ mod tests { efi::Status::SUCCESS } - let status = boot_services.free_pages(0x100000, 10); + // SAFETY: `0x100000` is a valid test address accepted by the mock; the mock validates the parameters. + let status = unsafe { boot_services.free_pages(0x100000, 10) }; assert!(status.is_ok()); } @@ -2240,11 +2653,13 @@ mod tests { } // positive test - let status = boot_services.free_pool(0xffff0000 as *mut u8); + // SAFETY: `0xffff0000` is a non-null test address accepted by the mock; no real memory is freed. + let status = unsafe { boot_services.free_pool(0xffff0000 as *mut u8) }; assert_eq!(status, Ok(())); // negative test - let status = boot_services.free_pool(ptr::null_mut()); + // SAFETY: null pointer is intentional to test the error path. + let status = unsafe { boot_services.free_pool(ptr::null_mut()) }; assert_eq!(status, Err(efi::Status::INVALID_PARAMETER)); } @@ -2904,7 +3319,8 @@ mod tests { #[should_panic = "Boot services function disconnect_controller is not initialized."] fn test_disconnect_controller_not_init() { let boot_services = boot_services!(); - _ = boot_services.disconnect_controller(ptr::null_mut(), None, None); + // SAFETY: Test code - calling uninitialized disconnect_controller to verify panic behavior. + _ = unsafe { boot_services.disconnect_controller(ptr::null_mut(), None, None) }; } #[test] @@ -2921,7 +3337,8 @@ mod tests { assert_eq!(ptr::null_mut(), child_handle); efi::Status::SUCCESS } - boot_services.disconnect_controller(1_usize as _, None, None).unwrap(); + // SAFETY: Test code - calling disconnect_controller with valid test parameters. + unsafe { boot_services.disconnect_controller(1_usize as _, None, None) }.unwrap(); } #[test] @@ -2938,7 +3355,8 @@ mod tests { assert_eq!(3, child_handle as usize); efi::Status::SUCCESS } - boot_services.disconnect_controller(1_usize as _, Some(2_usize as _), Some(3_usize as _)).unwrap(); + // SAFETY: Test code - calling disconnect_controller with specific driver and child handles. + unsafe { boot_services.disconnect_controller(1_usize as _, Some(2_usize as _), Some(3_usize as _)) }.unwrap(); } #[test] @@ -3040,7 +3458,8 @@ mod tests { #[should_panic = "Boot services function load_image is not initialized."] fn test_load_image_not_init() { let boot_services = boot_services!(); - _ = boot_services.load_image(false, ptr::null_mut(), ptr::null_mut(), None); + // SAFETY: Test code - all pointers are null/None; expected to panic before any dereference. + _ = unsafe { boot_services.load_image(false, ptr::null_mut(), ptr::null_mut(), None) }; } #[test] @@ -3069,14 +3488,17 @@ mod tests { efi::Status::SUCCESS } - let image_handle = boot_services - .load_image( - true, - std::ptr::dangling_mut::(), - 2_usize as *mut device_path::Protocol, - Some(&[1_u8, 2, 3, 4, 5]), - ) - .unwrap(); + // SAFETY: Test code - device_path is a fabricated non-null address; image is loaded from the provided buffer. + let image_handle = unsafe { + boot_services + .load_image( + true, + std::ptr::dangling_mut::(), + 2_usize as *mut device_path::Protocol, + Some(&[1_u8, 2, 3, 4, 5]), + ) + .unwrap() + }; assert_eq!(3, image_handle as usize); } @@ -3103,7 +3525,8 @@ mod tests { efi::Status::SUCCESS } - _ = boot_services.load_image_from_source(1_usize as _, 2_usize as _, &[1_u8, 2, 3, 4, 5]) + // SAFETY: Test code - device_path is a fabricated non-null address; image is loaded from the provided buffer. + _ = unsafe { boot_services.load_image_from_source(1_usize as _, 2_usize as _, &[1_u8, 2, 3, 4, 5]) } } #[test] @@ -3126,14 +3549,16 @@ mod tests { efi::Status::SUCCESS } - _ = boot_services.load_image_from_file(1_usize as _, NonNull::new(2_usize as _).unwrap()); + // SAFETY: Test code - file_device_path is a fabricated non-null address backed by a mock. + _ = unsafe { boot_services.load_image_from_file(1_usize as _, NonNull::new(2_usize as _).unwrap()) }; } #[test] #[should_panic = "Boot services function start_image is not initialized."] fn test_start_image_not_init() { let boot_services = boot_services!(); - _ = boot_services.start_image(ptr::null_mut()); + // SAFETY: Test code - all pointers are null; expected to panic before any dereference. + _ = unsafe { boot_services.start_image(ptr::null_mut()) }; } #[test] @@ -3151,7 +3576,8 @@ mod tests { efi::Status::SUCCESS } - boot_services.start_image(1_usize as _).unwrap(); + // SAFETY: Test code - image_handle is a fabricated non-null address backed by a mock. + unsafe { boot_services.start_image(1_usize as _) }.unwrap(); } #[test] @@ -3177,7 +3603,8 @@ mod tests { #[should_panic = "Boot services function exit is not initialized."] fn test_exit_not_init() { let boot_services = boot_services!(); - _ = boot_services.exit(ptr::null_mut(), efi::Status::SUCCESS, None); + // SAFETY: Test code - all pointers are null/None; expected to panic before any dereference. + _ = unsafe { boot_services.exit(ptr::null_mut(), efi::Status::SUCCESS, None) }; } #[test] @@ -3197,7 +3624,8 @@ mod tests { efi::Status::SUCCESS } - boot_services.exit(1_usize as _, efi::Status::SUCCESS, None).unwrap(); + // SAFETY: Test code - exit_data is None so no buffer validity is required. + unsafe { boot_services.exit(1_usize as _, efi::Status::SUCCESS, None) }.unwrap(); } #[test] diff --git a/sdk/patina/src/boot_services/boxed.rs b/sdk/patina/src/boot_services/boxed.rs index 10f3a3357..5a02b9a91 100644 --- a/sdk/patina/src/boot_services/boxed.rs +++ b/sdk/patina/src/boot_services/boxed.rs @@ -78,7 +78,8 @@ impl<'a, T, B: BootServices> BootServicesBox<'a, [T], B> { impl Drop for BootServicesBox<'_, T, B> { fn drop(&mut self) { - let _ = self.boot_services.free_pool(self.ptr as *mut u8); + // SAFETY: The pointer was allocated by BootServicesBox and is valid. + let _ = unsafe { self.boot_services.free_pool(self.ptr as *mut u8) }; } } diff --git a/sdk/patina/src/boot_services/event.rs b/sdk/patina/src/boot_services/event.rs index f30a52756..946ef7d5d 100644 --- a/sdk/patina/src/boot_services/event.rs +++ b/sdk/patina/src/boot_services/event.rs @@ -12,7 +12,7 @@ use core::ops; use r_efi::efi; /// Function signature for event notify function. -pub type EventNotifyCallback = extern "efiapi" fn(efi::Event, T); +pub type EventNotifyCallback = unsafe extern "efiapi" fn(efi::Event, T); /// The type of time that is specified in TriggerTime. See the timer delay types in “Related Definitions.” #[derive(Debug, Clone, Copy)] diff --git a/sdk/patina/src/boot_services/global_allocator.rs b/sdk/patina/src/boot_services/global_allocator.rs index b195995c1..69cd20279 100644 --- a/sdk/patina/src/boot_services/global_allocator.rs +++ b/sdk/patina/src/boot_services/global_allocator.rs @@ -49,7 +49,9 @@ impl BootServicesGlobalAllocator { unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { match layout.align() { - 0..=8 => _ = self.free_pool(ptr), + // SAFETY: This dealloc call is safe because the caller(GlobalAlloc) + // guarantees that the pointer was allocated with the same layout. + 0..=8 => _ = unsafe { self.free_pool(ptr) }, _ => { let Ok((extended_layout, tracker_offset)) = layout.extend(Layout::new::<*mut *mut u8>()) else { return; @@ -60,7 +62,8 @@ impl BootServicesGlobalAllocator { let original_ptr = unsafe { ptr::read(tracker_ptr) }; // SAFETY: Verifying alignment matches what we calculated during allocation. debug_assert_eq!(ptr, unsafe { original_ptr.add(original_ptr.align_offset(extended_layout.align())) }); - let _ = self.free_pool(original_ptr); + // SAFETY: The pointer was allocated by BootServicesGlobalAllocator and is valid. + let _ = unsafe { self.free_pool(original_ptr) }; } } } diff --git a/sdk/patina/src/component/service/memory.rs b/sdk/patina/src/component/service/memory.rs index 588a893ad..2a3132362 100644 --- a/sdk/patina/src/component/service/memory.rs +++ b/sdk/patina/src/component/service/memory.rs @@ -110,6 +110,7 @@ pub trait MemoryManager { /// // to free it. /// let alloc = memory_manager.allocate_pages(1, AllocationOptions::new())?; /// let ptr = alloc.into_raw_ptr::().unwrap(); + /// // SAFETY: ptr was just obtained from allocate_pages, so it is a valid page-aligned address. /// unsafe { memory_manager.free_pages(ptr as usize, 1)? }; /// /// Ok(()) @@ -246,7 +247,7 @@ pub trait MemoryManager { /// /// # Safety /// - /// Changing tha attributes of a page of memory can result in undefined behavior + /// Changing the attributes of a page of memory can result in undefined behavior /// if the attributes are not correct for the memory usage. The caller is responsible /// for understanding the use of the memory and verifying that all current and /// future accesses of the memory align to the attributes configured. @@ -326,14 +327,24 @@ impl AllocationOptions { /// Specifies the allocation strategy to use for the allocation. See [`PageAllocationStrategy`] /// for more details. + /// + /// # Safety + /// + /// When using [`PageAllocationStrategy::Address`], the caller must ensure that the provided + /// address is a valid physical address that is available for the requested + /// allocation type and number of pages. Passing an invalid or in use address results in + /// undefined behavior when the allocation is subsequently used. + /// + /// When using [`PageAllocationStrategy::MaxAddress`], the caller must ensure the provided + /// upper bound address is a sensible constraint for the target system. #[inline(always)] - pub const fn with_strategy(mut self, allocation_strategy: PageAllocationStrategy) -> Self { + pub const unsafe fn with_strategy(mut self, allocation_strategy: PageAllocationStrategy) -> Self { self.allocation_strategy = allocation_strategy; self } /// Specifies the alignment to use for the allocation. This must be a power - /// of two and greater then the page size. + /// of two and greater than the page size. /// /// Alignment will be ignored if the allocation strategy is [`PageAllocationStrategy::Address`]. #[inline(always)] @@ -1190,10 +1201,14 @@ mod tests { #[test] fn test_allocation_options_config_sticks() { - let options = AllocationOptions::default() - .with_alignment(0x200) - .with_memory_type(EfiMemoryType::PalCode) - .with_strategy(PageAllocationStrategy::Address(0x1000_0000_0000_0004)); + // SAFETY: Test code - the address is never passed to allocate_pages; this only verifies + // that the strategy value is stored correctly in AllocationOptions. + let options = unsafe { + AllocationOptions::default() + .with_alignment(0x200) + .with_memory_type(EfiMemoryType::PalCode) + .with_strategy(PageAllocationStrategy::Address(0x1000_0000_0000_0004)) + }; assert_eq!(options.alignment(), 0x200); assert_eq!(options.memory_type(), EfiMemoryType::PalCode); diff --git a/sdk/patina/src/performance/table.rs b/sdk/patina/src/performance/table.rs index 3d68e161b..8de1fa1bc 100644 --- a/sdk/patina/src/performance/table.rs +++ b/sdk/patina/src/performance/table.rs @@ -123,7 +123,11 @@ impl FBPT { }) .map_or_else( || { - // Allocate at a new address if no address found or if the previous address allocation failed. + // Allocate at a new address if no address found or if the previous address + // allocation failed. `AllocType::MaxAddress` requests any physical address + // below the given bound (u32::MAX = 4 GiB). The firmware chooses the actual + // address, so no specific physical address is supplied by the caller and the + // safety contract of `allocate_pages` is trivially satisfied. boot_services.allocate_pages( AllocType::MaxAddress(u32::MAX as usize), EfiMemoryType::ReservedMemoryType, @@ -133,7 +137,7 @@ impl FBPT { Result::Ok, )? as *mut u8; - // SAFETY: the allocation at this addres was of size `allocation_size` + // SAFETY: the allocation at this address was of size `allocation_size` Ok(unsafe { slice::from_raw_parts_mut(address, allocation_size) }) } } diff --git a/sdk/patina/src/runtime_services.rs b/sdk/patina/src/runtime_services.rs index 15ca52b1d..9db0ad5bc 100644 --- a/sdk/patina/src/runtime_services.rs +++ b/sdk/patina/src/runtime_services.rs @@ -302,8 +302,8 @@ pub trait RuntimeServices { /// /// # Safety /// - /// Ensure name isn't empty. It can be an empty string, - /// but there must be some data. + /// Ensure name slice isn't empty(length == 0). It can be an empty + /// string(null terminated slice, length == 1), but there must be some data. /// unsafe fn get_next_variable_name_unchecked( &self, @@ -315,6 +315,10 @@ pub trait RuntimeServices { } impl RuntimeServices for StandardRuntimeServices { + /// # Safety + /// + /// `name` must be null-terminated. Passing a non-null-terminated name results in + /// undefined behavior at the FFI boundary. unsafe fn set_variable_unchecked( &self, name: &mut [u16], @@ -338,17 +342,25 @@ impl RuntimeServices for StandardRuntimeServices { return Err(efi::Status::NOT_FOUND); } - let status = set_variable( - name.as_mut_ptr(), - namespace as *const _ as *mut _, - attributes, - data.len(), - data.as_ptr() as *mut c_void, - ); + // SAFETY: It is the caller's responsibility to ensure that the name is + // always null-terminated. + let status = unsafe { + set_variable( + name.as_mut_ptr(), + namespace as *const _ as *mut _, + attributes, + data.len(), + data.as_ptr() as *mut c_void, + ) + }; if status.is_error() { Err(status) } else { Ok(()) } } + /// # Safety + /// + /// `name` must be null-terminated. Passing a non-null-terminated name results in + /// undefined behavior at the FFI boundary. unsafe fn get_variable_unchecked( &self, name: &mut [u16], @@ -368,16 +380,20 @@ impl RuntimeServices for StandardRuntimeServices { }; let mut attributes: u32 = 0; - let status = get_variable( - name.as_mut_ptr(), - namespace as *const _ as *mut _, - ptr::addr_of_mut!(attributes), - ptr::addr_of_mut!(data_size), - match data { - Some(d) => d.as_ptr() as *mut c_void, - None => ptr::null_mut(), - }, - ); + // SAFETY: It is the caller's responsibility to ensure that the name is + // always null-terminated. + let status = unsafe { + get_variable( + name.as_mut_ptr(), + namespace as *const _ as *mut _, + ptr::addr_of_mut!(attributes), + ptr::addr_of_mut!(data_size), + match data { + Some(d) => d.as_ptr() as *mut c_void, + None => ptr::null_mut(), + }, + ) + }; if status == efi::Status::BUFFER_TOO_SMALL { return GetVariableStatus::BufferTooSmall { data_size, attributes }; @@ -388,6 +404,10 @@ impl RuntimeServices for StandardRuntimeServices { GetVariableStatus::Success { data_size, attributes } } + /// # Safety + /// + /// The null terminator is required by the UEFI firmware; passing a non-null-terminated name + /// results in undefined behavior at the FFI boundary. unsafe fn get_next_variable_name_unchecked( &self, prev_name: &[u16], @@ -416,8 +436,12 @@ impl RuntimeServices for StandardRuntimeServices { // the appropriate size that the buffer should be resized to for the second call. let mut first_try: bool = true; loop { - let status = - get_next_variable_name(ptr::addr_of_mut!(next_name_size), next_name.as_mut_ptr(), next_namespace); + // SAFETY: It is the caller's responsibility to ensure that the prev_name is always + // null-terminated. Which inturn ensures that next_name is null-terminated on the call + // to get_next_variable_name. Otherwise will lead to undefined behavior. + let status = unsafe { + get_next_variable_name(ptr::addr_of_mut!(next_name_size), next_name.as_mut_ptr(), next_namespace) + }; if status == efi::Status::BUFFER_TOO_SMALL && first_try { first_try = false; @@ -455,12 +479,17 @@ impl RuntimeServices for StandardRuntimeServices { maximum_variable_size: 0, }; - let status = query_variable_info( - attributes, - ptr::addr_of_mut!(var_info.maximum_variable_storage_size), - ptr::addr_of_mut!(var_info.remaining_variable_storage_size), - ptr::addr_of_mut!(var_info.maximum_variable_size), - ); + // SAFETY: `attributes` is a safe value type. The three output pointers are derived from + // fields of the local `var_info` struct via `addr_of_mut!`, so they are guaranteed to be + // valid and writable. + let status = unsafe { + query_variable_info( + attributes, + ptr::addr_of_mut!(var_info.maximum_variable_storage_size), + ptr::addr_of_mut!(var_info.remaining_variable_storage_size), + ptr::addr_of_mut!(var_info.maximum_variable_size), + ) + }; if status.is_error() { Err(status) } else { Ok(var_info) } }