Re: [PATCH 4/7] rust: serdev: remove the unbind() callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] rust: serdev: null pointer dereference in
  receive_buf_callback() [1]
--

commit 957ca5c83392506736074a2417dcd46b9e299666
Author: Danilo Krummrich <dakr@kernel.org>

rust: serdev: remove the unbind() callback

This commit removes the serdev::Driver::unbind() callback and the
corresponding adapter remove_callback(), documenting that device teardown
belongs in Drop or PinnedDrop.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d6..fd998b68b74f3 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
[ ... ]
> @@ -210,29 +209,6 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>          })
>      }
>  
> -    extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
> -        // SAFETY: The serial device bus only ever calls the remove callback with a valid pointer
> -        // to a `struct serdev_device`.
> -        //
> -        // INVARIANT: `sdev` is valid for the duration of `remove_callback()`.
> -        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
> -
> -        // SAFETY: `remove_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: No one has exclusive access to `private_data.driver`.
> -        let data = unsafe { &*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()) };
> -
> -        T::unbind(sdev, data_pinned);
> -    }
> -
>      extern "C" fn receive_buf_callback(
>          sdev: *mut bindings::serdev_device,
>          buf: *const u8,

[Severity: High]
This problem wasn't introduced by this patch, but can receive_buf_callback()
trigger a NULL pointer dereference [1] if T::probe() fails after the
serdev device has been opened?

In probe_callback(), the error-path ScopeGuard calls drvdata_obtain(), which
clears the device's drvdata pointer to NULL before dropping PrivateData and
calling serdev_device_close():

CPU0                                CPU1
probe_callback()
  serdev_device_open()
  T::probe() fails
  ScopeGuard::drop()
    drvdata_obtain() // sets NULL
                                    receive_buf_callback()
                                      drvdata_borrow() <-- NULL deref
    PrivateData::drop()
      serdev_device_close()

rust/kernel/serdev.rs:probe_callback() {
    ...
            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>>() });
            });
    ...
}

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

If a serial RX interrupt arrives between drvdata_obtain() and
serdev_device_close(), receive_buf_callback() dereferences the NULL drvdata
pointer.

[1] https://sashiko.dev/#/bug/linux-790dcb6c-6485-4b14-be30-8851e99a0542

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112005455.1224352-1-dakr@kernel.org?part=4