Re: [PATCH v16 3/3] rust: leds: add multicolor classdev abstractions

Markus Probst <[email protected]> Fri, 01 May 2026 16:08:05 +0000
Newsgroups	dev.linux.lists.sashiko
Message-ID	<[email protected]>
On Fri, 2026-05-01 at 16:00 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [Critical] Information Leak and Array Out-of-Bounds via FFI Layout Mismatch
> - [High] Data Race and Undefined Behavior via Aliasing Violation on subled fields
> - [High] Memory Leak and Use-After-Free via FwNode Reference Mismanagement
> - [High] Undefined Behavior and Memory Corruption via `KVec` Capacity Mismatch
> - [Low] Missing `#[inline]` attribute on `MultiColorDevice::from_raw`
> --
> 
> commit 519046430edb434e8aa4a9b617987bbaa0db6c71
> Author: Markus Probst <[email protected]>
> Subject: rust: leds: add multicolor classdev abstractions
> 
> This commit implements the abstractions needed for multicolor led class
> devices. It provides types for the multicolor led mode, the subleds, and
> the device builder to register a new multicolor led class device.
> 
> > diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> > new file mode 100644
> > index 000000000000..900ef050d2d3
> > --- /dev/null
> > +++ b/rust/kernel/led/multicolor.rs
> > @@ -0,0 +1,405 @@
> 
> [ ... ]
> 
> > +#[repr(C)]
> > +#[derive(Copy, Clone, Debug)]
> > +#[non_exhaustive]
> > +pub struct MultiColorSubLed {
> > +    /// the color of the sub led
> > +    pub color: Color,
> 
> Does this structure have an identical memory layout to the C mc_subled
> struct? The Color enum doesn't have a #[repr(u32)] or #[repr(C)]
> attribute, so the Rust compiler will likely size it as 1 byte.
> 
> To align the subsequent brightness field, the compiler would insert 3 bytes
> of uninitialized padding.
> 
> When the C LED core accesses mc_led_cdev->subled_info[i].color_index (which
> is 4 bytes), could it read the 1-byte color value combined with the
> uninitialized padding? This might produce a large index on little-endian
> architectures, potentially leading to out-of-bounds reads when the C
> subsystem indexes arrays like led_colors.
Thats a new one.

false positive.
`Color` does have a #[repr(u32)].

Thanks
- Markus Probst

> 
> [ ... ]
> 
> > +    pub fn build_multicolor<T: LedOps<Mode = MultiColor>>(
> [ ... ]
> > +                    let mut subleds_vec = KVec::new();
> > +                    subleds_vec.extend_from_slice(subleds, GFP_KERNEL)?;
> > +                    let (subled_info, num_colors, capacity) = subleds_vec.into_raw_parts();
> > +                    debug_assert_eq!(num_colors, capacity);
> 
> Could this assertion fail if the allocator sizes the allocation larger than
> num_colors? extend_from_slice can yield a capacity strictly larger than
> the requested size.
> 
> > +
> > +                    let subled_guard = ScopeGuard::new(|| {
> > +                        // SAFETY: `subled_info` is guaranteed to be a valid array pointer to
> > +                        // `mc_subled` with the length and capacity of `num_colors`.
> > +                        drop(unsafe { KVec::from_raw_parts(subled_info, num_colors, num_colors) });
> > +                    });
> 
> Is it safe to reconstruct the vector with num_colors as the capacity?
> 
> If extend_from_slice originally allocated a capacity greater than
> num_colors, passing a mismatched capacity back to the allocator during
> cleanup could lead to memory corruption.
> 
> [ ... ]
> 
> > +                    // SAFETY:
> > +                    // - `parent.as_ref().as_raw()` is guaranteed to be a pointer to a valid
> > +                    //    `device`.
> > +                    // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev_mc`.
> > +                    to_result(unsafe {
> > +                        bindings::led_classdev_multicolor_register_ext(
> > +                            parent.as_ref().as_raw(),
> > +                            ptr,
> > +                            if self.name.is_none() {
> > +                                &raw mut init_data
> > +                            } else {
> > +                                core::ptr::null_mut()
> > +                            },
> > +                        )
> > +                    })?;
> > +
> > +                    subled_guard.dismiss();
> > +
> > +                    core::mem::forget(self.fwnode); // keep the reference count incremented
> 
> Does this reliably transfer the reference count to the C subsystem?
> 
> If self.name.is_some(), the init_data pointer is explicitly passed as
> NULL to led_classdev_multicolor_register_ext. This means the C device is
> never associated with the fwnode, and the reference count leaked here might
> never be recovered since PinnedDrop uses dev.fwnode() to clean it up.
> 
> Conversely, if self.fwnode was None initially, but the C device inherited
> an fwnode from its parent, would the PinnedDrop implementation reconstruct
> an ARef from it and erroneously decrement a reference count it never owned?
> 
> [ ... ]
> 
> > +impl<T: LedOps<Mode = MultiColor>> MultiColorDevice<T> {
> > +    /// # Safety
> > +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> > +    /// `led::MultiColorDevice`.
> > +    unsafe fn from_raw<'a>(led_cdev: *mut bindings::led_classdev) -> &'a Self {
> 
> This isn't a bug, but functions that are small or forwarding to a binding
> call should be annotated with #[inline].
> 
> [ ... ]
> 
> > +    /// Returns the subleds passed to [`Device::new_multicolor`].
> > +    #[inline]
> > +    pub fn subleds(&self) -> &[MultiColorSubLed] {
> > +        // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
> > +        // valid `led_classdev_mc`.
> > +        let raw = unsafe { &*self.classdev.get() };
> > +        // SAFETY: `raw.subled_info` is a valid pointer to `mc_subled[num_colors]`.
> > +        // CAST: The safeguards in the const block ensure that `MultiColorSubLed` has an identical
> > +        // layout to `mc_subled`.
> > +        unsafe {
> > +            core::slice::from_raw_parts(
> > +                raw.subled_info.cast::<MultiColorSubLed>(),
> > +                // CAST: It is guaranteed that `num_colors` fits into an `usize`.
> > +                raw.num_colors as usize,
> > +            )
> > +        }
> > +    }
> 
> Does returning a shared reference &[MultiColorSubLed] violate Rust's
> aliasing rules?
> 
> The C LED subsystem can concurrently mutate the brightness and intensity
> fields of these subleds (e.g., in led_mc_calc_color_components or during
> sysfs writes).
> 
> Mutating memory from C code while a Rust driver holds a shared reference
> could result in undefined behavior. Should these mutable fields be wrapped in
> an Opaque<u32> to permit interior mutability?
> 
> [ ... ]
> 
> > +#[pinned_drop]
> > +impl<T: LedOps<Mode = MultiColor>> PinnedDrop for MultiColorDevice<T> {
> > +    fn drop(self: Pin<&mut Self>) {
> [ ... ]
> > +        // SAFETY: `subled_info` is guaranteed to be a valid array pointer to `mc_subled` with the
> > +        // length and capacity of `led_cdev.num_colors`. See `led::MulticolorDevice::new`.
> > +        drop(unsafe {
> > +            KVec::from_raw_parts(
> > +                led_cdev.subled_info,
> > +                led_cdev.num_colors as usize,
> > +                led_cdev.num_colors as usize,
> > +            )
> > +        });
> > +    }
> > +}
> 
> Similar to the error path above, could reconstructing this vector with
> led_cdev.num_colors as the capacity cause memory corruption if the original
> allocation capacity was larger?
signature.asc (application/pgp-signature, 870 B)