Re: [PATCH v26 1/4] rust: leds: Add basic led classdev abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] leds: mutable aliasing UB in blink_set_callback via concurrent sysfs
- [High] leds: lifetime inversion in DeviceBuilder allows Device to outlive strings
- [High] leds: FwNode use-after-free and leak via incorrect drop logic
- [Low] leds: missing #[inline] on Deref implementations for abstractions
--

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

rust: leds: Add basic led classdev abstractions

This commit implements the core Rust abstractions for LED class devices,
including led::LedOps, led::DeviceBuilder, and led::Device.

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> new file mode 100644
> index 0000000000000..a22c29cfd2628
> --- /dev/null
> +++ b/rust/kernel/led/normal.rs

[ ... ]

> +impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
> +    /// Registers a new [`Device`].
> +    pub fn build<'bound: 'init, T: LedOps + 'bound>(

[Severity: High]
Can this lifetime bound cause a use-after-free? With <'bound: 'init>, the
returned Device<'bound> outlives the temporary strings borrowed in
DeviceBuilder<'init>.

If the safe driver uses temporary strings (like name and devicename) and builds
the device, they are written as raw pointers into the C structure. Since the
returned Device outlives them, the C subsystem could later dereference
dangling pointers if triggers access led_cdev->name.

Should this be inverted to <'init: 'bound>?

> +        self,
> +        parent: &'bound device::Device<Bound>,
> +        ops: impl PinInit<T, Error> + 'init,
> +    ) -> impl PinInit<Device<'bound, T>, Error> + 'init {

[ ... ]

> +impl<'bound, T: LedOps + 'bound> Deref for Device<'bound, T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.ops
> +    }
> +}

[Severity: Low]
This isn't a bug, but this small abstraction function is missing the
recommended #[inline] attribute. The Rust subsystem coding guidelines mandate
that small abstraction functions, such as Deref trait implementations, must be
annotated with #[inline] to ensure proper compiler optimization across crate
boundaries.

[ ... ]

> +    /// # Safety
> +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> +    /// `led::Device`.
> +    /// `delay_on` and `delay_off` must be valid pointers to `usize` and have
> +    /// exclusive access for the period of this function.
> +    /// This function is called on enabling hardware accelerated blinking.
> +    unsafe extern "C" fn blink_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        delay_on: *mut usize,
> +        delay_off: *mut usize,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
> +            // `led_classdev` embedded within a `led::Device`.
> +            let classdev = unsafe { Device::<T>::from_raw(led_cdev) };
> +
> +            classdev.blink_set(
> +                // SAFETY: The function's contract guarantees that `delay_on` points to a `usize`
> +                // and is exclusive for the period of this function.
> +                unsafe { &mut *delay_on },
> +                // SAFETY: The function's contract guarantees that `delay_off` points to a `usize`
> +                // and is exclusive for the period of this function.
> +                unsafe { &mut *delay_off },
> +            )?;

[Severity: High]
Are we sure these mutable references have exclusive access?

The sysfs delay_on and delay_off attributes do not use locking to serialize
writes (for example, in led_delay_on_store() in ledtrig-timer.c). Concurrent
writes from user-space can cause the C core to invoke led_blink_set()
concurrently on multiple threads, passing pointers to shared fields like
&led_cdev->blink_delay_off.

If blink_set_callback() is invoked simultaneously on multiple threads, each
thread will cast the same shared pointer into &mut usize, creating overlapping
mutable references. This violates Rust's exclusive access guarantees and causes
Undefined Behavior.

> +            Ok(0)
> +        })
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<'bound, T: 'bound> PinnedDrop for Device<'bound, T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        let raw = self.classdev.get();
> +        // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
> +        // valid `led_classdev`.
> +        let dev: &device::Device = unsafe { device::Device::from_raw((*raw).dev) };
> +
> +        let _fwnode = dev
> +            .fwnode()
> +            // SAFETY: the reference count of `fwnode` has previously been
> +            // incremented in `led::DeviceBuilder::build`.
> +            .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });

[Severity: High]
Does this drop logic correctly match the FwNode from the builder?

The builder intentionally leaks the Rust FwNode reference using
core::mem::forget(self.fwnode). However, here we unconditionally extract
dev.fwnode() from the C struct and cast it back to ARef<FwNode> to drop it.

If the builder was called with fwnode: None, it forgets None. But if the
C subsystem later attached an fwnode to the device (or if it was modified),
this drop logic will decrement the refcount of an FwNode it never owned,
causing a use-after-free. Additionally, if the C subsystem replaced the
original FwNode, the original node is leaked.

Should the driver store Option<ARef<FwNode>> explicitly within its Rust Device
struct instead of pulling it from the C state?

> +
> +        // SAFETY: The existence of `self` guarantees that `self.classdev` has previously been
> +        // successfully registered with `led_classdev_register_ext`.
> +        unsafe { bindings::led_classdev_unregister(raw) };
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-rust_leds-v26-0-83837331020e@posteo.de?part=1