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

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 [off-list ref]

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/ (local)

[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