Re: [PATCH v2 1/2] rust: serdev: Fix race condition on driver unbind

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 3784 bytes --]

On Sat, 2026-09-05 at 13:44 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The function `drvdata_drop` introduces a Use-After-Free window and mutable aliasing vulnerability by dropping the driver data before clearing `dev->driver_data`.
> - [Low] The new helper function `drvdata_drop` is missing the `#[inline]` attribute.
> --
> 
> commit 26ab6ef2b4c35bdc9d899d45004d0c1a7f3fca80
> Author: Markus Probst <markus.probst@posteo.de>
> 
> rust: serdev: Fix race condition on driver unbind
> 
> This commit attempts to fix a race condition where the driver data pointer is
> NULL while the serdev device is still open. It does this by introducing a new
> function drvdata_drop() that leaves the pointer valid until the memory drop
> is complete.
> 
> Closes: https://lore.kernel.org/linux-serial/20260905000836.C8FC91F00A3D@smtp.kernel.org/
> 
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 2291d85b6849e..3886cc713c28b 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> 
> [ ... ]
> 
> > @@ -236,6 +237,32 @@ pub(crate) unsafe fn drvdata_obtain<T>(&self) -> Option<Pin<KBox<T>>> {
> >          //   in `into_foreign()`.
> >          Some(unsafe { Pin::<KBox<T>>::from_foreign(ptr.cast()) })
> >      }
> > +
> > +    /// Drop the private data stored in this [`Device`].
> > +    ///
> > +    /// The pointer to the private data remains valid until the drop is complete.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> > +    ///   [`Device::set_drvdata`].
> > +    pub(crate) unsafe fn drvdata_drop<T>(&self) {
> 
> [Severity: Low]
> This isn't a bug, but should this abstraction function be annotated with
> `#[inline]`?
> 
> The Rust subsystem coding guidelines mandate that small functions forwarding
> to C bindings or performing small abstraction tasks should use `#[inline]`.
> 
> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> > +
> > +        if ptr.is_null() {
> > +            return;
> > +        }
> > +
> > +        // SAFETY:
> > +        // - If `ptr` is not NULL, it comes from a previous call to `into_foreign()`.
> > +        // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
> > +        //   in `into_foreign()`.
> > +        drop(unsafe { Pin::<KBox<T>>::from_foreign(ptr.cast()) });
> > +
> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > +        unsafe { bindings::dev_set_drvdata(self.as_raw(), core::ptr::null_mut()) };

drvdata_obtain didn't have it.

> 
> [Severity: High]
> Does this create a Use-After-Free window?
> 
> By dropping the driver data before clearing `dev->driver_data`, the pointer
> remains valid while and after the memory is freed.
> 
> If a concurrent context, such as a hardware interrupt triggering
> receive_buf_callback() in rust/kernel/serdev.rs, runs at this exact
> moment, it will borrow a dangling pointer:
> 
> rust/kernel/serdev.rs:receive_buf_callback() {
>     ...
>     let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
>     ...
> }
> 
> Could this lead to a memory corruption vulnerability during the unbind path?
Under a normal case, no one should access this pointer anymore. For
serdev, this isn't a normal case. Before it becomes a dangling pointer,
PinnedDrop ensures it won't be used afterwards.


> 
> > +    }
> >  }

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]