Re: [PATCH v15 1/3] rust: leds: add basic led classdev abstractions

Markus Probst <[email protected]> Fri, 01 May 2026 11:46:59 +0000
Newsgroups	dev.linux.lists.sashiko
Message-ID	<[email protected]>
On Wed, 2026-04-29 at 20:56 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] DeviceBuilder::name lifetime erasure causes Use-After-Free
> - [High] Unsound fwnode reference count management causes underflow and Use-After-Free
> - [High] Aliasing violation and Null pointer dereference (Undefined Behavior) in blink_set_callback
> - [Medium] LedOps::brightness_get prevents propagating hardware errors
> --
> 
> commit a78848106d199e2b6993093a6b1e1aad9d0557e7
> Author: Markus Probst <[email protected]>
> 
> rust: leds: add basic led classdev abstractions
> 
> This commit introduces basic Rust abstractions for the LED subsystem,
> including a LedOps trait for device operations, a DeviceBuilder to register
> class devices, and a safe wrapper Device around led_classdev.
> 
> > diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> > new file mode 100644
> > index 000000000000..6cbaa917deb4
> > --- /dev/null
> > +++ b/rust/kernel/led.rs
> 
> [ ... ]
> 
> > +impl<'a> DeviceBuilder<'a, Named> {
> > +    /// Sets the name of the led.
> > +    ///
> > +    /// Setting this will prevent the fwnode from being used and prevents automatic name
> > +    /// composition.
> > +    #[inline]
> > +    pub fn name(self, name: &'a CStr) -> Self {
> > +        Self {
> > +            name: Some(name),
> > +            ..self
> > +        }
> > +    }
> > +}
> 
> Is it possible for the name string to be freed while the C subsystem still
> holds a pointer to it? DeviceBuilder::name accepts a &'a CStr and assigns
> its raw pointer to the name field of bindings::led_classdev.
> 
> Because Device<T> has no lifetime parameters, DeviceBuilder::build returns a
> Devres<Device<T>> that outlives the 'a lifetime.
> 
> If a caller dynamically allocates a string, registers the device, and drops
> the string, the C subsystem might retain a dangling pointer in led_cdev->name
> since it does not copy the name when init_data is bypassed. This could lead
> to a use-after-free when sysfs or led_classdev_unregister accesses the name.
> 
> [ ... ]
> 
> > +    /// Gets the current brightness level.
> > +    fn brightness_get(&self, dev: &Self::Bus, classdev: &Device<Self>) -> u32 {
> > +        let _ = (dev, classdev);
> > +        build_error!(VTABLE_DEFAULT_ERROR)
> > +    }
> 
> Does this signature prevent Rust LED drivers from safely propagating hardware
> read errors?
> 
> The C callback brightness_get returns an enum led_brightness, which the C
> subsystem treats as an int capable of holding negative error codes like
> -EIO.
> 
> Functions in the C core explicitly check for negative return values to
> gracefully handle hardware failures. Returning a u32 forces drivers to
> return bogus brightness values instead of an error. Could this be changed
> to return a Result<u32>?
Yes.
Bindgen interprets `enum led_brightness` as u32, so it will require
casting.

Only this issue is new. All other reports have already been dealt with
the last revision.

Thanks
- Markus Probst

> 
> > diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> > new file mode 100644
> > index 000000000000..b457ddb742c7
> > --- /dev/null
> > +++ b/rust/kernel/led/normal.rs
> 
> [ ... ]
> 
> > +    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) };
> > +            // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
> > +            let parent = unsafe { T::Bus::from_device(classdev.parent()) };
> > +
> > +            classdev.ops.blink_set(
> > +                parent,
> > +                classdev,
> > +                // 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 },
> > +            )?;
> 
> The C API permits callers to pass NULL for these arguments to request
> default blink delays. Would dereferencing a null pointer here to create a
> Rust reference cause immediate undefined behavior?
> 
> Additionally, when the C core passes pointers to the blink_delay_on and
> blink_delay_off fields inside the led_classdev struct, the callback
> simultaneously creates an immutable reference &Device<T> to the entire
> struct.
> 
> Does creating a mutable reference to a field while holding an immutable
> reference to the parent struct violate strict aliasing rules?
> 
> [ ... ]
> 
> > +#[pinned_drop]
> > +impl<T: LedOps> PinnedDrop for Device<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::Device::new`.
> > +            .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });
> 
> Can this underflow the fwnode reference count if self.fwnode was None
> during device building?
> 
> In DeviceBuilder::build, the code attempts to leak the FwNode reference by
> calling core::mem::forget(self.fwnode). If self.fwnode was None, nothing is
> leaked.
> 
> If the C driver core later automatically assigns an fwnode to the device,
> dev.fwnode() will return Some. This would cause PinnedDrop to construct an
> ARef and drop it, decrementing a reference count that it never incremented
> or owned.
signature.asc (application/pgp-signature, 870 B)