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

[sashiko-bot@kernel.org]

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()) };

[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?

> +    }
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905-rust_serdev_fix-v2-0-35dfcd06ef2e@posteo.de?part=1