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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Use-After-Free and Mutable Aliasing in `drvdata_drop` because memory is freed before clearing the driver data pointer.
- [High] `T::receive()` can run concurrently with `T::unbind()`, leading to a Use-After-Unbind race condition.
- [High] Circular deadlock between `probe_callback` and `receive_buf_callback`.
--

commit 67f5ed19e07ddf9c3ef694dbb2c1e962df051525
Author: Markus Probst <markus.probst@posteo.de>

rust: serdev: Fix race condition on driver probe

This commit fixes a race condition on driver probe failures by using
drvdata_drop() instead of drvdata_obtain(), ensuring the serdev device
is closed before the pointer to the driver data is set to null.

Closes: https://lore.kernel.org/linux-serial/20260903222159.70A911F000E9@smtp.kernel.org/

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d6..a12b1dea12aa1 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
> @@ -176,7 +176,7 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>              let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
>              let private_data = ScopeGuard::new_with_data(private_data, |_| {
>                  // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
> -                drop(unsafe { sdev.as_ref().drvdata_obtain::<PrivateData<'_, T>>() });
> +                unsafe { sdev.as_ref().drvdata_drop::<PrivateData<'_, T>>() };

[Severity: Critical]
This is a pre-existing issue, but does calling drvdata_drop() here introduce
a use-after-free and mutable aliasing vulnerability?

Since drvdata_drop() executes drop() on the driver's private data and
deallocates the memory before calling dev_set_drvdata(..., null), the global
device pointer still references the freed memory.

If receive_buf_callback() is invoked concurrently by incoming serial data:

rust/kernel/serdev.rs:receive_buf_callback() {
    ...
    let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
    ...
}

It can fetch the still-non-null dangling pointer before it is cleared, and
block on the active mutex. Once drop() completes, receive_buf_callback()
would wake up and operate on freed memory.

>              });
>              let mut active = private_data.active.lock();

[Severity: High]
This isn't a bug introduced by this patch, but can holding this active mutex
across the T::probe() call result in a circular deadlock?

In probe_callback(), the active mutex is locked before invoking T::probe().
If a driver writes a command to the device and synchronously waits for a
response during probe, the incoming response would trigger
receive_buf_callback():

rust/kernel/serdev.rs:receive_buf_callback() {
    ...
    let active = private_data.active.lock();
    ...
}

Since probe_callback() holds the lock and waits for the response, and
receive_buf_callback() receives the response but blocks indefinitely waiting
for the lock, does this structurally guarantee a deadlock for synchronous
probes?

[ ... ]

[Severity: High]
This is a pre-existing issue, but can T::receive() run concurrently with
T::unbind(), leading to a use-after-unbind race condition?

During device removal, remove_callback() calls T::unbind():

rust/kernel/serdev.rs:remove_callback() {
    ...
    T::unbind(sdev, data_pinned);
}

It does not lock the active mutex, set it to false, or close the serdev
device prior to unbinding.

Consequently, incoming serial data can trigger receive_buf_callback(), which
evaluates the active check as true (since active is not cleared during
unbind):

rust/kernel/serdev.rs:receive_buf_callback() {
    ...
    if !*active {
        return length;
    }
    ...
    T::receive(sdev, data_pinned, buf)
}

This forwards the data to T::receive(), which can execute concurrently with
or after T::unbind(), accessing driver resources that are actively being
destroyed.

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