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

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

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

On Thu, 2026-09-03 at 22:21 +0000, sashiko-bot@kernel.org wrote:
> 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?
This definitely needs to be addressed, yes.
> 
> [ ... ]
> 
> > @@ -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.
True.

> 
> > +                    // 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.
That would be a driver issue.

And there shouldn't be a reason for a driver to wait for an
acknowledgement.

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

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