Re: [PATCH v3 13/27] rust: usb: make Driver trait lifetime-parameterized

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Use-After-Free vulnerability due to incorrect `'bound` lifetime on dynamic `DeviceId`.
- [High] Soundness violation and FFI lifetime bypass in driver data retrieval.
- [Low] Commit message incorrectly references `unbind()` instead of `disconnect()`.
--

commit 92ace0018c4af4c9a4ccdae5f9dd912c5a7f6ab0
Author: Danilo Krummrich <dakr@kernel.org>

rust: usb: make Driver trait lifetime-parameterized

This commit introduces a 'bound lifetime parameter to the usb::Driver::Data
associated type. This allows driver private data to store references directly
instead of relying on Devres. The probe and disconnect callbacks are updated
to use this lifetime.

> The probe() and unbind() callbacks thus gain a 'bound lifetime parameter
> on the methods themselves; avoiding a global lifetime on the trait impl.

This isn't a bug, but the commit message references unbind() instead of
disconnect(). The USB subsystem specifically uses disconnect(), whereas
unbind() is typically used by other bus types like platform drivers.

> diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
> index 995221d327b89..ec67e4d2416f1 100644
> --- a/rust/kernel/usb.rs
> +++ b/rust/kernel/usb.rs

[ ... ]

> @@ -110,7 +110,7 @@ extern "C" fn disconnect_callback(intf: *mut bindings::usb_interface) {
>          // SAFETY: `disconnect_callback` is only ever called after a successful call to
>          // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
>          // and stored a `Pin<KBox<T::Data>>`.
> -        let data = unsafe { dev.drvdata_borrow::<T::Data>() };
> +        let data = unsafe { dev.drvdata_borrow::<T::Data<'_>>() };
>  
>          T::disconnect(intf, data);
>      }

Does this violate the type matching contract of ForeignOwnable?

The data is originally stored in probe_callback() with a local 'probe lifetime
but is retrieved here with a newly instantiated local '_ lifetime.

If a driver implements Data<'bound> such that it is invariant over 'bound
(for example, by using interior mutability like Cell<&'bound Interface>), then
T::Data<'probe> and T::Data<'_> are formally different types in Rust.

Transmuting between invariant types with different lifetimes could violate the
safety preconditions of ForeignOwnable and bypass the borrow checker's ability
to verify constraints across the FFI boundary.

[ ... ]

> @@ -315,16 +319,19 @@ pub trait Driver {
>      ///
>      /// Called when a new USB interface is bound to this driver.
>      /// Implementers should attempt to initialize the interface here.
> -    fn probe(
> -        interface: &Interface<device::Core>,
> -        id: &DeviceId,
> -        id_info: &Self::IdInfo,
> -    ) -> impl PinInit<Self::Data, Error>;
> +    fn probe<'bound>(
> +        interface: &'bound Interface<device::Core>,
> +        id: &'bound DeviceId,
> +        id_info: &'bound Self::IdInfo,
> +    ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound;

Can tying the id parameter to the 'bound lifetime result in a use-after-free?

When a device is bound using a dynamic ID (added via the new_id sysfs
attribute), the USB core passes a pointer to a dynamically allocated
struct usb_dynid to the probe function via usb_match_dynamic_id().

Userspace can remove dynamic IDs at any time using the remove_id sysfs
attribute, which unlinks and frees the usb_dynid without checking if any
currently bound devices matched against it.

If a Rust driver safely stores the id reference in its private Data<'bound>
based on this lifetime guarantee, it could hold a dangling pointer once the
dynamic ID is removed, potentially causing a use-after-free later during
the device's lifecycle.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517000149.3226762-1-dakr@kernel.org?part=13