Re: [PATCH] rust: serdev: Fix race condition on driver probe fail

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition leading to a NULL pointer dereference in `receive_buf_callback` during driver unbind.
--

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

rust: serdev: Fix race condition on driver probe fail

This commit fixes a race condition during driver probe failure where the
driver data pointer is set to NULL while the serdev device is still open.
It uses a ScopeGuard to ensure the device is closed before the pointer is
set to NULL on probe failure.

Link: 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..4f57f4c453292 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
> @@ -190,6 +190,16 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>              // SAFETY: We have exclusive access to `private_data.open`.
>              unsafe { *private_data.open.get() = true };
>  
> +            let open_guard = ScopeGuard::new(|| {
> +                // SAFETY:
> +                // - `private_data.sdev.as_raw()` is guaranteed to be a pointer to a valid
> +                //   `struct serdev_device`.
> +                // - We just opened the device, thus it is guaranteed to be open.
> +                unsafe { bindings::serdev_device_close(private_data.sdev.as_raw()) };
> +                // SAFETY: We have exclusive access to `private_data.open`.
> +                unsafe { *private_data.open.get() = false };
> +            });
> +
>              let data = T::probe(sdev, info);

[ ... ]

> @@ -203,10 +213,12 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>  
>              drop(active);
>  
> -            result.map(|()| {
> -                private_data.dismiss();
> -                0
> -            })
> +            result?;
> +
> +            open_guard.dismiss();
> +            private_data.dismiss();
> +
> +            Ok(0)
>          })
>      }

[Severity: High]
This is a pre-existing issue, but while reviewing this fix for the probe
path, I noticed a very similar race condition exists during driver unbind.
Could this sequence still lead to a NULL pointer dereference in
receive_buf_callback() on device hot-unplug or sysfs unbind?

When a driver is unbound, remove_callback() delegates to T::unbind() and
returns without explicitly closing the serdev device:

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

After remove_callback(), the driver core executes post_unbind_callback(),
which calls drvdata_obtain(). Does this set the drvdata pointer to NULL
while the serdev device is still open and able to receive data?

rust/kernel/driver.rs:post_unbind_callback() {
        ...
        drop(unsafe { dev.drvdata_obtain::<T::DriverData<'_>>() });
}

If data is received on the serial port in this window, before the device
is finally closed, receive_buf_callback() can execute asynchronously. Will
it fetch the NULL drvdata pointer and dereference it when attempting to
lock the active mutex?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905-rust_serdev_fix-v1-1-2ea92b154a6b@posteo.de?part=1