Skip to content

Update child handle validation in core_disconnect_controller() #1045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
makubacki wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update child handle validation in core_disconnect_controller() #1045

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
301 changes: 294 additions & 7 deletions patina_dxe_core/src/driver_services.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(|| {
Expand Down