Re: [PATCH 5/5] rust: serdev: Pause receive callback before calling unbind

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing memory barrier between setting drvdata and resuming RX allows concurrent NULL pointer dereference.
--

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

rust: serdev: Pause receive callback before calling unbind
    
The receive callback and unbind callback now have exclusive access to
the drivers private data. Provide mutable references in callbacks to
avoid the need for locks in the private data. Remove the Sync
requirement.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 66543108ec2fa..7d47d91e3bc35 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
[ ... ]
> @@ -200,10 +203,14 @@ extern "C" fn receive_buf_callback(
>          // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
>          let sdev = unsafe { &*sdev.cast::<Device<device::BoundInternal>>() };
>  
> -        // SAFETY: `receive_buf_callback` is only ever called after a successful call to
> -        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> -        // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
> -        let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
> +        // SAFETY:
> +        // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
> +        //   hence it's guaranteed that `Device::set_drvdata()` has been called and stored a
> +        //   `Pin<KBox<PrivateData<'_, T>>>`.
> +        // - `unbind_callback` calls `serdev_device_pause_rx` before accessing the driver data,
> +        //   which guarantees that this function will not overlap with it. Thus we have exclusive
> +        //   access.
> +        let private_data = unsafe { sdev.as_ref().drvdata_borrow_mut::<PrivateData<'_, T>>() };

[Severity: Critical]
This is a pre-existing issue, but does this code allow a concurrent NULL pointer
dereference?

In probe_callback() in rust/kernel/serdev.rs, set_drvdata() writes the
driver data pointer to memory, followed by serdev_device_resume_rx() which
clears the SERPORT_PAUSE_RX flag using a relaxed clear_bit() operation:

    unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };

On weakly ordered architectures like ARM64, the CPU can reorder the plain data
store to occur after the flag is cleared. A concurrent tty receive path
observing the cleared flag will enter receive_buf_callback() and read
dev->driver_data before the store propagates, obtaining a NULL pointer.

Calling drvdata_borrow_mut() on this NULL pointer creates a reference to NULL,
triggering undefined behavior and a kernel panic. Can this be triggered
externally if the serial port receives data from an attached hardware device
during the narrow window of probe_callback()?

>  
>          T::receive(sdev, private_data.driver_data(), buf)
>      }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de?part=5