Re: [PATCH v6 01/13] rust: auxiliary: let registration_data_with() closures return covariant sub-fields

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Safe methods `registration_data_with` and `registration_data` can cause a Use-After-Free because `Registration::drop` frees the registration data but leaves a dangling pointer in the underlying C struct.
--

commit f7dbc67a0cd0bf4fcd048c32f110a8ba459452b3
Author: Alistair Popple <apopple@nvidia.com>

rust: auxiliary: let registration_data_with() closures return covariant sub-fields

The closure passed to registration_data_with() currently receives
Pin<&'a F::Of<'a>> with 'a universally quantified. This prevents the
closure from returning references derived from the registration data,
even for sub-fields that are covariant in their lifetime. By tying the
outer reference to the &self lifetime, the closure can coerce covariant
sub-fields and return them directly.

Link: https://lore.kernel.org/nova-gpu/DL3WPTVM033J.33RWYCZOC67Z1@kernel.org/

> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> index 60dfbec8f3302..06f816420790a 100644
> --- a/rust/kernel/auxiliary.rs
> +++ b/rust/kernel/auxiliary.rs

[ ... ]

> @@ -314,13 +318,14 @@ unsafe fn registration_data_pinned<F: ForLt + 'static>(&self) -> Result<Pin<&F::
>      /// Returns [`ENOENT`] if no registration data has been set, e.g. when the device was
>      /// registered by a C driver.
>      #[inline]
> -    pub fn registration_data_with<F: ForLt + 'static, R>(
> -        &self,
> -        f: impl for<'a> FnOnce(Pin<&'a F::Of<'a>>) -> R,
> +    pub fn registration_data_with<'this, F: ForLt + 'static, R>(
> +        &'this self,
> +        f: impl for<'a> FnOnce(Pin<&'this F::Of<'a>>) -> R,
>      ) -> Result<R> {

[Severity: High]
This is a pre-existing issue, but can this safe method cause a use-after-free
if a child driver or a detached kernel thread retains an ARef<Device> after
the parent's unbind event?

Looking at Registration::drop in rust/kernel/auxiliary.rs, it frees the
registration data but does not clear the registration_data_rust pointer in
the underlying C struct:

Registration::drop() {
    ...
    // SAFETY: registration_data was set in new() via into_foreign().
    drop(unsafe {
        Pin::<KBox<RegistrationData<F::Of<'static>>>>::from_foreign(
            (*self.adev.as_ptr()).registration_data_rust,
        )
    });
    ...
}

If a safely retained Device is then used to call registration_data_with(), the
internal registration_data_pinned() method will bypass the null check and
read from the dangling pointer:

Device::registration_data_pinned() {
    let ptr = unsafe { (*self.as_raw()).registration_data_rust };
    if ptr.is_null() {
        ...
        return Err(ENOENT);
    }
    ...
    let type_id = unsafe { ptr.cast::<TypeId>().read() };
    ...
}

Could Registration::drop be updated to clear registration_data_rust to NULL
before freeing the data to prevent this issue? Or does this also need
additional synchronization to handle concurrent access safely?

> -        // SAFETY: The HRTB closure prevents the caller from smuggling in references with a
> -        // concrete short lifetime, making the round-trip from `'static` sound regardless of
> -        // variance.
> +        // SAFETY: The HRTB on the inner type prevents the caller from exploiting a specific
> +        // choice of `'a`. Covariant sub-fields can be safely coerced to `'this`, while
> +        // invariant fields cannot be coerced and thus cannot escape with an incorrect
> +        // lifetime.
>          let pinned = unsafe { self.registration_data_pinned::<F>()? };
>  
>          Ok(f(pinned))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909064506.910162-1-apopple@nvidia.com?part=1