Re: [PATCH v2 08/11] rust: driver: store pointers in `DeviceId`

[sashiko-bot@kernel.org]

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 <gary@garyguo.net>

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/20260629-id_info-v2-0-56fccbe9c5ef@garyguo.net?part=8