Re: [PATCH] rust: serdev: Synchronize receive callback before calling unbind

[sashiko-bot@kernel.org]

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

New issues:
- [High] Holding the `active` mutex during `T::unbind` causes an inescapable deadlock for drivers that synchronously communicate with the device.
- [Low] Logically dead code inside the error-handling `ScopeGuard` in `probe_callback`.

Pre-existing issues:
- [High] Race condition between drvdata_obtain and receive_buf_callback causes NULL pointer dereference or Use-After-Free.
--

commit 6f561db35778bb34c19e46b1a5df116263c0adbc
Author: Markus Probst <markus.probst@posteo.de>
Subject: rust: serdev: Synchronize receive callback before calling unbind

The commit changes the receive and unbind callbacks to provide exclusive
access to driver private data by handing out mutable references. It removes
the Sync requirement for Data and introduces locking for synchronization.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d6..44f029ed93fd1 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs

[ ... ]

> @@ -127,16 +127,6 @@ pub struct PrivateData<'bound, T: Driver> {
>  #[pinned_drop]
>  impl<T: Driver> PinnedDrop for PrivateData<'_, T> {
>      fn drop(self: Pin<&mut Self>) {
> -        let mut active = self.active.lock();
> -        if *active {
> -            // SAFETY:
> -            // - We have exclusive access to `self.driver`.
> -            // - `self.driver` is guaranteed to be initialized.
> -            unsafe { (*self.driver.get()).assume_init_drop() };
> -            *active = false;
> -        }
> -        drop(active);
> -
>          // SAFETY: We have exclusive access to `self.open`.
>          if unsafe { *self.open.get() } {

[Severity: High]
This isn't a bug introduced by this patch, but does the timing of closing the
device cause a use-after-free or NULL pointer dereference?

If probe fails after the device is opened, or when the driver is unbound,
drvdata_obtain() clears the underlying C driver_data pointer to NULL and
drops the data. However, bindings::serdev_device_close(self.sdev.as_raw())
is only called later inside PrivateData::drop().

Because the device is still open while the pointer is cleared, incoming
serial data can trigger receive_buf_callback() in the TTY workqueue.

receive_buf_callback() calls drvdata_borrow(), which reads the NULL pointer
and blindly dereferences it, or reads it right before it's cleared and
accesses freed memory.

Should the device be closed before clearing the drvdata pointer?

[ ... ]

> @@ -176,7 +166,20 @@ 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>>() });
> +                let private_data = unsafe {
> +                    sdev.as_ref()
> +                        .drvdata_obtain::<PrivateData<'_, T>>()
> +                        .unwrap_unchecked()
> +                };
> +
> +                let mut active = private_data.active.lock();
> +                if *active {

[Severity: Low]
Is this block of code logically dead?

The ScopeGuard closure checks if *active is true to selectively call
assume_init_drop(). However, *active is only set to true if the driver
initialization succeeds.

Immediately after setting *active = true, private_data.dismiss() is called,
which consumes the guard and prevents this closure from running.

Therefore, this closure only runs on the error path, where *active is
strictly false, meaning the assume_init_drop() block will never execute.

> +                    // SAFETY:
> +                    // - We have exclusive access to `private_data.driver`.
> +                    // - `private_data.driver` is guaranteed to be initialized.
> +                    unsafe { (*private_data.driver.get()).assume_init_drop() };
> +                    *active = false;
> +                }
>              });
>              let mut active = private_data.active.lock();

[ ... ]

> @@ -222,15 +225,21 @@ extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
>          // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
>          let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
>  
> -        // SAFETY: No one has exclusive access to `private_data.driver`.
> -        let data = unsafe { &*private_data.driver.get() };
> +        let mut active = private_data.active.lock();
> +
> +        // SAFETY: We have exclusive access to `private_data.driver`.
> +        let data = unsafe { &mut *private_data.driver.get() };
>          // SAFETY:
>          // - `private_data.driver` is pinned.
>          // - `remove_callback` is only ever called after a successful call to `probe_callback`,
>          //   hence it's guaranteed that `private_data.driver` was initialized.
> -        let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
> +        let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_mut()) };
>  
>          T::unbind(sdev, data_pinned);

[Severity: High]
Does holding the active mutex during T::unbind() cause a deadlock for
drivers that synchronously communicate with the device?

If the driver's unbind() callback sends a shutdown command to the device and
synchronously waits for an acknowledgment, the incoming serial data for the
acknowledgment will be routed to receive_buf_callback().

receive_buf_callback() attempts to acquire this same active mutex to process
the response, completing the deadlock because remove_callback() is blocked
waiting for the acknowledgment.

> +
> +        // SAFETY: We already established that `data` is guaranteed to be initialized.
> +        unsafe { data.assume_init_drop() };
> +        *active = false;
>      }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-rust_serdev_ref_mut-v1-1-245db7eb09af@posteo.de?part=1