Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Undefined Behavior (null reference creation) due to uninitialized `driver_data` in `IdArray::new_without_index`. Pre-existing issues: - [High] Use-After-Free in PCI and USB subsystem cores due to TOCTOU race condition when matching dynamic IDs. -- commit 6b9468ff67342f4c1fd6f7bf72f1fef7fa35aa2e Author: Gary Guo <[email protected]> rust: driver: store pointers in DeviceId The common practice in C drivers is to store pointers into driver_data field of device IDs. The Rust code is however currently storing indices into the fields and then carry a side table that maps the index to pointers. [ ... ] > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > index 022f0101871f5..50d82bfca9b8c 100644 > --- a/rust/kernel/device_id.rs > +++ b/rust/kernel/device_id.rs [ ... ] > @@ -133,7 +160,7 @@ impl<T: RawDeviceId, const N: usize> IdArray<T, (), N> { > /// If the device implements [`RawDeviceIdIndex`], consider using > [`IdArray::new`] instead. > pub const fn new_without_index(ids: [T; N]) -> Self { > // SAFETY: `T` is layout-wise compatible with `T::RawType`, so is > the array of them. > - let raw_ids: [T::RawType; N] = unsafe { > core::mem::transmute_copy(&ids) }; > + let raw_ids: [MaybeUninit<T::RawType>; N] = unsafe { > core::mem::transmute_copy(&ids) }; [Severity: High] If a driver uses the @none branch in macros like i2c_device_table! or auxiliary_device_table!, it expands to IdArray::new_without_index(). Does this leave the driver_data field uninitialized (0)? When the device probes, the framework calls info_unchecked::<()>() on the matching ID, which unconditionally reads the 0 from driver_data and casts it to a &() reference: device_id.rs:info_unchecked() { unsafe { core::ptr::from_ref(self) .byte_add(Self::DRIVER_DATA_OFFSET) .cast::<&U>() .read() } } Will this cause Undefined Behavior by creating a null reference, even for zero-sized types like ()? [ ... ] > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs > index a3dd48f763532..925cf280dae2a 100644 > --- a/rust/kernel/pci.rs > +++ b/rust/kernel/pci.rs > @@ -110,10 +110,14 @@ extern "C" fn probe_callback( > // SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `struct > pci_device_id` and > // does not add additional invariants, so it's safe to transmute. > let id = unsafe { &*id.cast::<DeviceId>() }; > - let info = T::ID_TABLE.info(id.index()); > + > + // SAFETY: `id` comes from `T::ID_TABLE` which is of type > `IdArray<_, T::IdInfo>` or > + // `pci_device_id_any` which has 0 as driver_data. It can also come > from dynamic IDs, which > + // will ensure that `driver_data` exists in `T::ID_TABLE`. > + let info = unsafe { id.info_unchecked_opt::<T::IdInfo>() }; [Severity: High] This is a pre-existing issue, but does this dereference a potentially freed id pointer if there is a concurrent sysfs write to remove_id? In the C PCI core, pci_match_device() searches the dynamic ID list under dynids.lock. When a match is found, it drops the lock and returns a raw pointer to the pci_device_id embedded within the dynid structure: drivers/pci/pci-driver.c:pci_match_device() { spin_lock(&drv->dynids.lock); list_for_each_entry(dynid, &drv->dynids.list, node) { if (pci_match_one_device(&dynid->id, dev)) { found_id = &dynid->id; break; } } spin_unlock(&drv->dynids.lock); if (found_id) return found_id; } If a concurrent sysfs write to remove_id occurs, it unlinks and frees the dynid: drivers/pci/pci-driver.c:remove_id_store() { list_del(&dynid->node); kfree(dynid); } Since the lock is released before pci_match_device() returns, could this leave the found_id pointer unprotected and lead to a Use-After-Free when the probe callback dereferences it here? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
