Re: [PATCH v3 17/27] rust: auxiliary: generalize Registration over ForLt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unsound lifetime erasure in `Registration::new` allows safe code to trigger Use-After-Free.
- [Critical] Lockless read and premature teardown of `registration_data_rust` causes Use-After-Free.
--

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

rust: auxiliary: generalize Registration over ForLt
    
Generalize Registration<T> to Registration<F: ForLt> and
Device::registration_data<F: ForLt>() to return Pin<&F::Of<'_>>.
The stored 'static lifetime is shortened to the borrow lifetime of &self
via ForLt::cast_ref; ForLt's covariance guarantee makes this sound.

> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> index 3f1e2966c03aa..db3d56106ea28 100644
> --- a/rust/kernel/auxiliary.rs
> +++ b/rust/kernel/auxiliary.rs
> @@ -271,12 +272,16 @@ pub fn parent(&self) -> &device::Device<device::Bound> {
>  
>      /// Returns a pinned reference to the registration data set by the registering (parent) driver.
>      ///
> -    /// Returns [`EINVAL`] if `T` does not match the type used by the parent driver when calling
> +    /// `F` is the [`ForLt`](trait@ForLt) encoding of the data type. The returned
> +    /// reference has its lifetime shortened from `'static` to `&self`'s borrow lifetime via
> +    /// [`ForLt::cast_ref`].
> +    ///
> +    /// Returns [`EINVAL`] if `F` does not match the type used by the parent driver when calling
>      /// [`Registration::new()`].
>      ///
>      /// Returns [`ENOENT`] if no registration data has been set, e.g. when the device was
>      /// registered by a C driver.
> -    pub fn registration_data<T: 'static>(&self) -> Result<Pin<&T>> {
> +    pub fn registration_data<F: ForLt>(&self) -> Result<Pin<&F::Of<'_>>> {
>          // SAFETY: By the type invariant, `self.as_raw()` is a valid `struct auxiliary_device`.
>          let ptr = unsafe { (*self.as_raw()).registration_data_rust };

Does this lockless read of registration_data_rust allow a use-after-free?

While this wasn't introduced by this patch, it appears to remain a
vulnerability in the code modified here. In Registration::drop(), the memory
backing this pointer is freed via Pin::from_foreign(...), but dropping the
Registration (when the parent device unbinds) does not guarantee the
struct auxiliary_device is destroyed.

Other kernel threads might still hold a reference to the auxiliary device
(e.g., via ARef<Device>). When Registration::drop() frees the data, it
doesn't set registration_data_rust to NULL.

Without synchronization, could another thread with an ARef<Device> call
the safe registration_data() method after the devres has unwound and access
freed memory?

Should the registration data be freed in the auxiliary device's release
callback instead, or should proper synchronization be introduced?

[ ... ]

> @@ -389,43 +399,51 @@ struct RegistrationData<T> {
> -impl<T: Send + Sync + 'static> Registration<T> {
> +impl<F: ForLt> Registration<F>
> +where
> +    for<'a> F::Of<'a>: Send + Sync,
> +{
>      /// Create and register a new auxiliary device with the given registration data.
>      ///
>      /// The `data` is owned by the registration and can be accessed through the auxiliary device
>      /// via [`Device::registration_data()`].
> -    pub fn new<E>(
> -        parent: &device::Device<device::Bound>,
> +    pub fn new<'bound, E>(
> +        parent: &'bound device::Device<device::Bound>,
>          name: &CStr,
>          id: u32,
>          modname: &CStr,
> -        data: impl PinInit<T, E>,
> +        data: impl PinInit<F::Of<'bound>, E>,
>      ) -> Result<Devres<Self>>
>      where
>          Error: From<E>,
>      {
>          let data = KBox::pin_init::<Error>(
>              try_pin_init!(RegistrationData {
> -                type_id: TypeId::of::<T>(),
> +                type_id: TypeId::of::<F::Of<'static>>(),
>                  data <- data,
>              }),
>              GFP_KERNEL,
>          )?;
>  
> +        // SAFETY: Lifetimes are erased and do not affect layout, so RegistrationData<F::Of<'bound>>
> +        // and RegistrationData<F::Of<'static>> have identical representation.
> +        let data: Pin<KBox<RegistrationData<F::Of<'static>>>> =
> +            unsafe { core::mem::transmute(data) };

Can this transmute allow safe code to trigger a use-after-free?

Since Registration::new accepts parent: &'bound device::Device and
data: impl PinInit<F::Of<'bound>, E>, Rust's lifetime covariance allows callers
to borrow parent for an arbitrarily short lifetime (e.g., the scope of a
stack variable inside the probe function) and pass data containing
references bounded by that short lifetime.

This function unsafely uses core::mem::transmute to erase this lifetime
to 'static. Since it returns a Devres<Registration<F>> which has no lifetime
parameters, it is treated as 'static, allowing safe Rust code to place the
Devres into the parent device's devres list.

When the local stack variables go out of scope, won't the Devres still hold
the data, causing an exploitable use-after-free when
Device::registration_data() is called or when Registration::drop() cleans up?

Should Registration::new be marked unsafe so the caller guarantees the
references outlive the device's bound state, or should the API be redesigned
to structurally capture the lifetime?

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