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

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

[Markus Probst]

If `Driver::probe` fails, the pointer to the driver data (`PrivateData`)
will first be set to NULL by `drvdata_obtain` and only after that the
serdev device will be closed by Drop. Thus there is a small window in
which the serdev device is still open, but the pointer to the driver data
is NULL. Therefore it is possible that `receive_buf_callback` might try to
access the `active` mutex on a null pointer.

Add a separate `ScopeGuard` that will close the device before the driver
data will be set to NULL on probe failure.

Fixes: 99f59aa82341 ("rust: add basic serial device bus abstractions")
Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-serial/20260903222159.70A911F000E9@smtp.kernel.org/
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Unfortunately, this also makes the code in probe even more convoluted
than it already is. I will submit a patch (for the next merge cycle)
soon, which will address this concern.
---
 rust/kernel/serdev.rs | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index 17ca504b7f8d..4f57f4c45329 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);
 
             // SAFETY: We have exclusive access to `private_data.driver`.
@@ -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)
         })
     }
 

---
base-commit: e5e04726cdd043e309677071ab1b65a4b18f422b
change-id: 20260904-rust_serdev_fix-be3ff9c8a5e8

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

[sashiko-bot]

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

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

[Markus Probst]

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

On Fri, 2026-09-04 at 23:54 +0000, Markus Probst wrote:
> If `Driver::probe` fails, the pointer to the driver data (`PrivateData`)
> will first be set to NULL by `drvdata_obtain` and only after that the
> serdev device will be closed by Drop. Thus there is a small window in
> which the serdev device is still open, but the pointer to the driver data
> is NULL. Therefore it is possible that `receive_buf_callback` might try to
> access the `active` mutex on a null pointer.
It seems, Sashiko found the same issue in a different unrelated place
too (while reviewing this patch):

https://sashiko.dev/#/patchset/20260905-rust_serdev_fix-v1-1-2ea92b154a6b%40posteo.de

Unfortunately, I cannot fix this one so easily, because the serdev
device needs to be open for the entire lifetime of the "real" driver
data (Driver::Data).

Thanks
- Markus Probst

> 
> Add a separate `ScopeGuard` that will close the device before the driver
> data will be set to NULL on probe failure.
> 
> Fixes: 99f59aa82341 ("rust: add basic serial device bus abstractions")
> Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/linux-serial/20260903222159.70A911F000E9@smtp.kernel.org/
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
> Unfortunately, this also makes the code in probe even more convoluted
> than it already is. I will submit a patch (for the next merge cycle)
> soon, which will address this concern.
> ---
>  rust/kernel/serdev.rs | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d..4f57f4c45329 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);
>  
>              // SAFETY: We have exclusive access to `private_data.driver`.
> @@ -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)
>          })
>      }
>  
> 
> ---
> base-commit: e5e04726cdd043e309677071ab1b65a4b18f422b
> change-id: 20260904-rust_serdev_fix-be3ff9c8a5e8

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

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

[Gary Guo]

On Sat Sep 5, 2026 at 1:22 AM BST, Markus Probst wrote:
> On Fri, 2026-09-04 at 23:54 +0000, Markus Probst wrote:
>> If `Driver::probe` fails, the pointer to the driver data (`PrivateData`)
>> will first be set to NULL by `drvdata_obtain` and only after that the
>> serdev device will be closed by Drop. Thus there is a small window in
>> which the serdev device is still open, but the pointer to the driver data
>> is NULL. Therefore it is possible that `receive_buf_callback` might try to
>> access the `active` mutex on a null pointer.
> It seems, Sashiko found the same issue in a different unrelated place
> too (while reviewing this patch):
>
> https://sashiko.dev/#/patchset/20260905-rust_serdev_fix-v1-1-2ea92b154a6b%40posteo.de
>
> Unfortunately, I cannot fix this one so easily, because the serdev
> device needs to be open for the entire lifetime of the "real" driver
> data (Driver::Data).

What would go wrong if the device is closed before destroying the driver data?

Best,
Gary

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

[Markus Probst]

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

On Sat, 2026-09-05 at 15:24 +0100, Gary Guo wrote:
> On Sat Sep 5, 2026 at 1:22 AM BST, Markus Probst wrote:
> > On Fri, 2026-09-04 at 23:54 +0000, Markus Probst wrote:
> > > If `Driver::probe` fails, the pointer to the driver data (`PrivateData`)
> > > will first be set to NULL by `drvdata_obtain` and only after that the
> > > serdev device will be closed by Drop. Thus there is a small window in
> > > which the serdev device is still open, but the pointer to the driver data
> > > is NULL. Therefore it is possible that `receive_buf_callback` might try to
> > > access the `active` mutex on a null pointer.
> > It seems, Sashiko found the same issue in a different unrelated place
> > too (while reviewing this patch):
> > 
> > https://sashiko.dev/#/patchset/20260905-rust_serdev_fix-v1-1-2ea92b154a6b%40posteo.de
> > 
> > Unfortunately, I cannot fix this one so easily, because the serdev
> > device needs to be open for the entire lifetime of the "real" driver
> > data (Driver::Data).
> 
> What would go wrong if the device is closed before destroying the driver data?
The driver would still have a handle to the Device and could call
`Device::set_baudrate` for instance. This would be a use-after-free by
itself.

I am currently investigating, if I can only stop rx and leave the
device for transmission open. There seems to be the `CREAD` c_cflag I
can remove from the tty. But I am not sure yet if it guarantees that
receive_buf isn't called anymore.

Thanks
- Markus Probst

> 
> Best,
> Gary

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