From c2768ec2cd2ca6c6725fd711b5455c470440a53d Mon Sep 17 00:00:00 2001 From: Michael Kubacki Date: Tue, 11 Nov 2025 09:56:33 -0500 Subject: [PATCH] Update child handle validation in core_disconnect_controller() The implementation currently doesn't skip a driver when a specific child handle is given. It might potentially call the driver's stop function and return success instead of returning not found. This change: 1. Adds a flag to track whether the specified child handle was found in the driver's child list. 2. Adds a validation check to skip processing a driver if a specific child was requested but not found in that driver's child list. 3. Updates the disconnect logic to use `total_children == child_handles.len()` to determine if all children should be disconnected. A couple of unit tests are added to test expected behavior based on child state when core_disconnect_controller() is called. The `test_disconnect_specific_child_among_multiple_children()` would have passed previously but tests that a specific child is requested, the child handle is present in the driver's child list, and that the child is not the only child. Under these conditions, it verifies that the driver's stop function is not called (since other child are present) and the driver remains connected to the controller. Signed-off-by: Michael Kubacki --- patina_dxe_core/src/driver_services.rs | 301 ++++++++++++++++++++++++- 1 file changed, 294 insertions(+), 7 deletions(-) diff --git a/patina_dxe_core/src/driver_services.rs b/patina_dxe_core/src/driver_services.rs index 968b2dfd0..26975087c 100644 --- a/patina_dxe_core/src/driver_services.rs +++ b/patina_dxe_core/src/driver_services.rs @@ -390,7 +390,7 @@ extern "efiapi" fn connect_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. /// @@ -481,12 +481,23 @@ pub unsafe fn core_disconnect_controller( child_handles.retain(|x| child_set.insert(*x)); let total_children = child_handles.len(); - let mut is_only_child = false; + let mut child_handle_valid = 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. - child_handles.retain(|x| x == &handle); - is_only_child = total_children == child_handles.len(); + // Check if the specified child handle is in the list of children for this driver. + // If found, filter to only that child. If not found, child_handle_valid stays false. + child_handles.retain(|x| { + if x == &handle { + child_handle_valid = true; + true + } else { + false + } + }); + } + + // If a specific child handle was requested but wasn't found, skip this driver. + if child_handle.is_some() && !child_handle_valid { + continue; } //resolve the handle to the driver_binding. @@ -516,7 +527,7 @@ pub unsafe fn core_disconnect_controller( child_handles.as_mut_ptr(), ); } - if status == efi::Status::SUCCESS && (child_handle.is_none() || is_only_child) { + if status == efi::Status::SUCCESS && (child_handle.is_none() || total_children == child_handles.len()) { status = (driver_binding.stop)(driver_binding_interface, controller_handle, 0, core::ptr::null_mut()); } if status == efi::Status::SUCCESS { @@ -1763,6 +1774,282 @@ mod tests { }); } + #[test] + fn test_disconnect_specific_child_not_managed_by_driver() { + // Verifies that when a specific child handle is requested but NOT managed by + // any driver, the driver is skipped and returns not found + with_locked_state(|| { + use std::sync::atomic::{AtomicUsize, Ordering}; + + static STOP_CALLS: AtomicUsize = AtomicUsize::new(0); + + extern "efiapi" fn mock_stop_tracking( + _this: *mut efi::protocols::driver_binding::Protocol, + _controller_handle: efi::Handle, + _num_children: usize, + _child_handle_buffer: *mut efi::Handle, + ) -> efi::Status { + STOP_CALLS.fetch_add(1, Ordering::SeqCst); + efi::Status::SUCCESS + } + + let guid1 = patina::Guid::from_string("0e896c7a-57dc-4987-bc22-abc3a8263210"); + let interface1: *mut c_void = 0x1234 as *mut c_void; + let (handle1, _) = PROTOCOL_DB.install_protocol_interface(None, guid1.to_efi_guid(), interface1).unwrap(); + + // Create controller handle + let (controller_handle, _) = PROTOCOL_DB + .install_protocol_interface( + None, + efi::protocols::device_path::PROTOCOL_GUID, + 0x1111 as *mut core::ffi::c_void, + ) + .unwrap(); + + // Create driver handle + let (driver_handle, _) = PROTOCOL_DB + .install_protocol_interface( + None, + efi::protocols::device_path::PROTOCOL_GUID, + 0x2222 as *mut core::ffi::c_void, + ) + .unwrap(); + + // Create a child handle that is managed by the driver + let (managed_child_handle, _) = PROTOCOL_DB + .install_protocol_interface( + None, + efi::protocols::device_path::PROTOCOL_GUID, + 0x3333 as *mut core::ffi::c_void, + ) + .unwrap(); + + // Create a child handle that is not managed by the driver + let (unmanaged_child_handle, _) = PROTOCOL_DB + .install_protocol_interface( + None, + efi::protocols::device_path::PROTOCOL_GUID, + 0x4444 as *mut core::ffi::c_void, + ) + .unwrap(); + + // Create driver binding protocol + let binding = Box::new(efi::protocols::driver_binding::Protocol { + version: 10, + supported: mock_supported_success, + start: mock_start_success, + stop: mock_stop_tracking, + driver_binding_handle: driver_handle, + image_handle: DXE_CORE_HANDLE, + }); + let binding_ptr = Box::into_raw(binding) as *mut core::ffi::c_void; + + PROTOCOL_DB + .install_protocol_interface( + Some(driver_handle), + efi::protocols::driver_binding::PROTOCOL_GUID, + binding_ptr, + ) + .unwrap(); + + // Have the driver manage the controller + PROTOCOL_DB + .add_protocol_usage( + controller_handle, + efi::protocols::device_path::PROTOCOL_GUID, + Some(driver_handle), + Some(handle1), + efi::OPEN_PROTOCOL_BY_DRIVER, + ) + .unwrap(); + + // The driver only manages managed_child_handle, not unmanaged_child_handle + PROTOCOL_DB + .add_protocol_usage( + controller_handle, + efi::protocols::device_path::PROTOCOL_GUID, + Some(driver_handle), + Some(managed_child_handle), + efi::OPEN_PROTOCOL_BY_CHILD_CONTROLLER, + ) + .unwrap(); + + STOP_CALLS.store(0, Ordering::SeqCst); + + // Attempt to disconnect unmanaged_child_handle (not managed by this driver) + unsafe { + let result = core_disconnect_controller(controller_handle, None, Some(unmanaged_child_handle)); + // Should return not found since no driver manages this child + assert!(result.is_err(), "disconnect should fail when child not managed by any driver"); + if let Err(err) = result { + assert_eq!(err, EfiError::NotFound, "Should return NotFound for an unmanaged child"); + } + } + + // Driver's stop function should NOT have been called since child wasn't found + assert_eq!( + STOP_CALLS.load(Ordering::SeqCst), + 0, + "Driver stop should not be called when specified child is not managed by driver" + ); + }); + } + + #[test] + fn test_disconnect_specific_child_among_multiple_children() { + // Checks that when a specific child is requested and the driver has multiple children, + // only that specific child is disconnected (not the entire driver). + with_locked_state(|| { + use std::sync::atomic::{AtomicUsize, Ordering}; + + static CHILD_STOP_CALLS: AtomicUsize = AtomicUsize::new(0); + static DRIVER_STOP_CALLS: AtomicUsize = AtomicUsize::new(0); + + extern "efiapi" fn mock_stop_tracking( + _this: *mut efi::protocols::driver_binding::Protocol, + _controller_handle: efi::Handle, + num_children: usize, + _child_handle_buffer: *mut efi::Handle, + ) -> efi::Status { + if num_children == 0 { + DRIVER_STOP_CALLS.fetch_add(1, Ordering::SeqCst); + } else { + CHILD_STOP_CALLS.fetch_add(1, Ordering::SeqCst); + } + efi::Status::SUCCESS + } + + let guid1 = patina::Guid::from_string("0e896c7a-57dc-4987-bc22-abc3a8263210"); + let interface1: *mut c_void = 0x1234 as *mut c_void; + let (handle1, _) = PROTOCOL_DB.install_protocol_interface(None, guid1.to_efi_guid(), interface1).unwrap(); + + // Create controller handle + let (controller_handle, _) = PROTOCOL_DB + .install_protocol_interface( + None, + efi::protocols::device_path::PROTOCOL_GUID, + 0x1111 as *mut core::ffi::c_void, + ) + .unwrap(); + + // Create driver handle + let (driver_handle, _) = PROTOCOL_DB + .install_protocol_interface( + None, + efi::protocols::device_path::PROTOCOL_GUID, + 0x2222 as *mut core::ffi::c_void, + ) + .unwrap(); + + // Create multiple child handles + let (child_handle_a, _) = PROTOCOL_DB + .install_protocol_interface( + None, + efi::protocols::device_path::PROTOCOL_GUID, + 0x3333 as *mut core::ffi::c_void, + ) + .unwrap(); + + let (child_handle_b, _) = PROTOCOL_DB + .install_protocol_interface( + None, + efi::protocols::device_path::PROTOCOL_GUID, + 0x4444 as *mut core::ffi::c_void, + ) + .unwrap(); + + let (child_handle_c, _) = PROTOCOL_DB + .install_protocol_interface( + None, + efi::protocols::device_path::PROTOCOL_GUID, + 0x5555 as *mut core::ffi::c_void, + ) + .unwrap(); + + // Create driver binding protocol + let binding = Box::new(efi::protocols::driver_binding::Protocol { + version: 10, + supported: mock_supported_success, + start: mock_start_success, + stop: mock_stop_tracking, + driver_binding_handle: driver_handle, + image_handle: DXE_CORE_HANDLE, + }); + let binding_ptr = Box::into_raw(binding) as *mut core::ffi::c_void; + + PROTOCOL_DB + .install_protocol_interface( + Some(driver_handle), + efi::protocols::driver_binding::PROTOCOL_GUID, + binding_ptr, + ) + .unwrap(); + + // Have the driver manage the controller + PROTOCOL_DB + .add_protocol_usage( + controller_handle, + efi::protocols::device_path::PROTOCOL_GUID, + Some(driver_handle), + Some(handle1), + efi::OPEN_PROTOCOL_BY_DRIVER, + ) + .unwrap(); + + // Add both child handles as managed by this driver + PROTOCOL_DB + .add_protocol_usage( + controller_handle, + efi::protocols::device_path::PROTOCOL_GUID, + Some(driver_handle), + Some(child_handle_a), + efi::OPEN_PROTOCOL_BY_CHILD_CONTROLLER, + ) + .unwrap(); + + PROTOCOL_DB + .add_protocol_usage( + controller_handle, + efi::protocols::device_path::PROTOCOL_GUID, + Some(driver_handle), + Some(child_handle_b), + efi::OPEN_PROTOCOL_BY_CHILD_CONTROLLER, + ) + .unwrap(); + + PROTOCOL_DB + .add_protocol_usage( + controller_handle, + efi::protocols::device_path::PROTOCOL_GUID, + Some(driver_handle), + Some(child_handle_c), + efi::OPEN_PROTOCOL_BY_CHILD_CONTROLLER, + ) + .unwrap(); + + CHILD_STOP_CALLS.store(0, Ordering::SeqCst); + DRIVER_STOP_CALLS.store(0, Ordering::SeqCst); + + // Disconnect only child_handle_a (one of the two children) + // Because total_children != child_handles.len(), the driver should not be fully stopped + unsafe { + let result = core_disconnect_controller(controller_handle, None, Some(child_handle_a)); + assert!(result.is_ok(), "disconnect should succeed"); + } + + assert_eq!( + CHILD_STOP_CALLS.load(Ordering::SeqCst), + 1, + "Child stop should be called once for the specified child" + ); + assert_eq!( + DRIVER_STOP_CALLS.load(Ordering::SeqCst), + 0, + "Driver stop should NOT be called when other children still exist" + ); + }); + } + #[test] fn test_core_disconnect_controller_invalid_child_handle() { with_locked_state(|| {