Re: [PATCH v3 4/7] rust: kernel: Add greybus abstractions

[Ayush Singh <ayush@beagleboard.org>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] greybus: safe Connection methods allow UAF of parent devices
> - [High] greybus: rust: UAF of driver data due to eager drop in Registration::drop
> - [Low] rust: missing #[inline] on trivial abstraction functions
> - [Low] rust: missing // INVARIANT comment and size validation in GbOperationMsgHdr
> --
>
> commit 75320c1f601296bad85db7371e32bc7e97df53b7
> Author: Ayush Singh <ayush@beagleboard.org>
>
> rust: kernel: Add greybus abstractions
>
> This commit adds Rust abstractions for the Greybus core to support implementing
> Greybus host drivers in Rust. The abstractions cover the APIs used by gb-softsvc
> and gb-uart-node drivers, including host device registration, connection management,
> and SVC protocol structures. It also introduces lifetime and ownership rules for
> Greybus messages and callbacks within the Rust bindings.
>
>> diff --git a/rust/kernel/greybus/hd.rs b/rust/kernel/greybus/hd.rs
>> --- /dev/null
>> +++ b/rust/kernel/greybus/hd.rs
>
> [ ... ]
>
>> +    unsafe extern "C" fn message_send(
>> +        hd: *mut bindings::gb_host_device,
>> +        dest_cport_id: u16,
>> +        msg: *mut bindings::gb_message,
>> +        gfp_mask: bindings::gfp_t,
>> +    ) -> c_int {
>> +        // SAFETY: gb_host_device and HostDevice have the same layout.
>> +        let hd = unsafe { Device::<device::CoreInternal<'_>>::from_raw(hd) };
>> +        // SAFETY: message_send is only ever called after a successful call to
>> +        // gb_hd_add, hence it's guaranteed that Device::set_drvdata() has been called
>> +        // and stored a Pin<KBox<T>>.
>> +        let data = unsafe { hd.as_ref().drvdata_borrow() };
>> +        // SAFETY: The caller guarantees msg is valid for the duration of this call.
>> +        let msg = unsafe { Message::from_raw(msg) };
>> +
>> +        match T::message_send(&data, dest_cport_id, msg, Flags::new(gfp_mask)) {
>
> [Severity: High]
> Does this code allow a use-after-free of the driver data?
>
> Thread A could initiate a request through gb_operation_request_send(), which
> calls this message_send wrapper without taking locks that serialize against
> deletion. This wrapper borrows the driver data T via drvdata_borrow() and
> invokes T::message_send().
>
> If the hardware receives the request and replies immediately, the response can
> be processed by gb_operation_work() in a workqueue, dropping the operation's
> active count to 0. Meanwhile, if Thread B executes Registration::drop and
> calls gb_hd_del(), it cancels operations. Since the active count is 0,
> wait_event(!gb_operation_is_active(operation)) returns immediately without
> waiting for message_send to return.
>
> When gb_hd_del() finishes, Registration::drop eagerly extracts and drops
> the driver data. If Thread A was preempted inside T::message_send or before
> returning to C, it could resume execution and access the freed driver data.
>
> [ ... ]
>
>> +impl<T> Drop for Registration<T> {
>> +    fn drop<'a>(&'a mut self) {
>> +        // SAFETY: By the type invariant of Self, self.as_raw() is a pointer to a valid
>> +        // struct gb_host_device.
>> +        unsafe { bindings::gb_hd_del(self.as_raw()) }
>> +
>> +        {
>> +            // SAFETY: hd is a valid pointer to gb_host_device.
>> +            let hd_dev = unsafe { Device::<device::CoreInternal<'_>>::from_raw(self.as_raw()) };
>> +            // SAFETY: The driver data was set to a T in Registration::new() and has not been
>> +            // taken since, and this is the only place that takes it.
>> +            drop(unsafe { hd_dev.as_ref().drvdata_obtain::<T>() });
>
> [Severity: High]
> Can the eager drop of driver data here lead to a use-after-free if
> message_send is still executing concurrently?
>
> Since gb_hd_del() may return without waiting for message_send if the
> operation's active count drops to 0 (e.g., from an immediate hardware response),
> we might drop the driver data while Thread A is still executing the wrapper
> or driver code referencing it.
>

I feel like this particular race condition should be fixed on the C side
instead of the Rust bindings. Put simply gb_hd_del should first flush all
in-flight operations, and prevent new operations from being executed after
the function returns.

Of course, I might be missing something, so people more familiar with
greybus can correct me if I am wrong. I can work on the patches for what I
described above.

Best Regards,
Ayush Singh

-- 
BeagleBoard.org Foundation
https://www.beagleboard.org