Re: [PATCH 07/10] 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:
- [Critical] Arbitrary Memory Dereference / Privilege Escalation via PCI dynamic IDs (`new_id`)
- [High] Undefined Behavior via unconditional null reference creation when `driver_data` is 0
--

commit 01871465d12a97f7be006ca329ebda24f458cf1e
Author: Gary Guo <gary@garyguo.net>

rust: driver: store pointers in `DeviceId`

This commit changes Rust device IDs to store pointers directly in the
driver_data field, matching the behavior of C drivers. It works around
the limitation of missing provenance during const evaluation by wrapping
raw IDs in MaybeUninit.

> diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
> index 84852a2d9ad78..59453588df0e7 100644
> --- a/rust/kernel/device_id.rs
> +++ b/rust/kernel/device_id.rs
[ ... ]
> @@ -47,15 +50,48 @@ pub unsafe trait RawDeviceIdIndex: RawDeviceId {
>      /// The offset (in bytes) to the context/data field in the raw device ID.
>      const DRIVER_DATA_OFFSET: usize;
>  
> -    /// The index stored at `DRIVER_DATA_OFFSET` of the implementor of the [`RawDeviceIdIndex`]
> -    /// trait.
> -    fn index(&self) -> usize;
> +    /// Obtain the data pointer stored inside the device ID.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `&Self` must be stored inside a `IdArray<Self, U>`.
> +    unsafe fn info_unchecked<U>(&self) -> &'static U {
> +        // SAFETY: By safety requirement of the trait, this is `self.driver_data as *const U` and by
> +        // the safety requirement of the function, this is stored in `IdArray<Self, U>` so is
> +        // convertible to `&'static U`.
> +        unsafe {
> +            core::ptr::from_ref(self)
> +                .byte_add(Self::DRIVER_DATA_OFFSET)
> +                .cast::<&U>()
> +                .read()
> +        }
> +    }

[Severity: High]
Does this code create an unconditional null reference in Rust if the
driver_data or driver_info field is 0?

Creating a null reference is immediate undefined behavior in Rust, even for
zero-sized types. This 0 value can occur in paths like:

1. IdArray::new_without_index() leaving driver_data fields as 0.

2. When userspace adds a dynamic USB ID via the sysfs new_id interface using
fewer than 5 fields (omitting refVendor), USB core initializes driver_info
to 0.

[ ... ]
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index a3dd48f763532..a630c7fc6a853 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
[ ... ]
> @@ -110,10 +110,13 @@ 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.
> +        let info = unsafe { id.info_unchecked_opt::<T::IdInfo>() };

[Severity: Critical]
Can this allow an arbitrary memory dereference or bypass kernel lockdown?

The PCI subsystem supports dynamic IDs via the sysfs new_id interface
(drivers/pci/pci-driver.c), which parses up to 7 arguments, the last being
driver_data (%lx).

This allows a privileged user (root) to supply an arbitrary pointer value.
The code here will blindly cast this integer to an Option<&'static U> via
info_unchecked_opt() and dereference it if non-zero, allowing arbitrary
kernel memory access.

[ ... ]
> diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
> index 500b5e0ba4ea9..8aeff50117552 100644
> --- a/rust/kernel/usb.rs
> +++ b/rust/kernel/usb.rs
[ ... ]
> @@ -89,7 +89,8 @@ extern "C" fn probe_callback(
>              // 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>`.
> +            let info = unsafe { id.info_unchecked::<T::IdInfo>() };
>              let data = T::probe(intf, id, info);

[Severity: High]
Does this code trigger undefined behavior when the underlying USB device ID
driver_info field is 0?

When the id is constructed using IdArray::new_without_index() or when
dynamically added via sysfs new_id with fewer fields, the driver_info is 0.

This probe unconditionally expects a valid non-null reference and creates
one even for 0-initialized IDs via info_unchecked().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-id_info-v1-0-96af1e559ef9@garyguo.net?part=7