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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Reconstructing `KVec` with `num_colors` as capacity leads to Undefined Behavior due to a capacity and type mismatch.
- [High] Exposing `brightness` and `intensity` as plain `u32` fields in a shared reference `&[MultiColorSubLed]` violates Rust's aliasing rules and introduces data races.
- [High] The `MultiColorDevice` API has a soundness hole that fails to capture the `'a` lifetime of the `name` string, leading to a Use-After-Free of `led_cdev->name`.
--

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

rust: leds: add multicolor classdev abstractions

Implement the abstractions needed for multicolor led class devices,
including MultiColor, MultiColorSubLed, MultiColorDevice, and build_multicolor.

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

[ ... ]

> +#[repr(C)]
> +#[derive(Copy, Clone, Debug)]
> +#[non_exhaustive]
> +pub struct MultiColorSubLed {
> +    /// the color of the sub led
> +    pub color: Color,
> +    /// the brightness of the sub led.
> +    ///
> +    /// The value will be automatically calculated.
> +    pub brightness: u32,
> +    /// the intensity of the sub led.
> +    pub intensity: u32,

[Severity: High]
Will exposing brightness and intensity as plain u32 fields violate Rust's
aliasing rules?

When a driver calls subleds() to get a &[MultiColorSubLed], it receives an
immutable reference which guarantees the underlying memory won't be modified
concurrently. However, the C LED subsystem can mutate these fields
concurrently (for instance, via multi_intensity_store() or
led_mc_calc_color_components()). Should these fields be wrapped in Opaque<u32>
or UnsafeCell<u32> to allow safe concurrent modifications?

> +    /// arbitrary data for the driver to store.
> +    pub channel: u32,
> +}

[ ... ]

> +impl<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
> +    /// Registers a new [`MulticolorDevice`].
> +    pub fn build_multicolor<'bound: 'a, T: LedOps<Mode = MultiColor> + 'bound>(
> +        self,
> +        parent: &'bound T::Bus,
> +        ops: impl PinInit<T, Error> + 'a,
> +        subleds: &'a [MultiColorSubLed],
> +    ) -> impl PinInit<MultiColorDevice<'bound, T>, Error> + 'a {

[Severity: High]
Could the returned initialization type fail to capture the 'a lifetime of
the name string in DeviceBuilder?

Because MultiColorDevice<'bound, T> does not capture 'a, safe code might
pass a short-lived stack-allocated CString to the builder and drop it after
registration, while the registered MultiColorDevice remains alive.

> +        const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET);
> +
> +        try_pin_init!(MultiColorDevice {
> +            ops <- ops,
> +            classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev_mc| {

[ ... ]

> +                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);

[Severity: High]
Can discarding the true capacity from into_raw_parts() lead to undefined
behavior during deallocation?

When extend_from_slice() allocates the underlying buffer, the allocator may
reserve a larger capacity than exactly num_colors. If the true capacity is
discarded, later reconstructing the vector with capacity set to num_colors
violates the from_raw_parts() safety contract and could pass the wrong
layout to the deallocator.

> +
> +                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) });
> +                });

[ ... ]

> +                unsafe {
> +                    ptr.write(bindings::led_classdev_mc {
> +                        led_cdev: bindings::led_classdev {
> +                            brightness_set: (!T::BLOCKING)
> +                                .then_some(Adapter::<T>::brightness_set_callback),
> +                            brightness_set_blocking: T::BLOCKING
> +                                .then_some(Adapter::<T>::brightness_set_blocking_callback),
> +                            brightness_get: T::HAS_BRIGHTNESS_GET
> +                                .then_some(Adapter::<T>::brightness_get_callback),
> +                            blink_set: T::HAS_BLINK_SET
> +                                .then_some(Adapter::<T>::blink_set_callback),
> +                            max_brightness: T::MAX_BRIGHTNESS,
> +                            brightness: self.initial_brightness,
> +                            color: self.color as u32,
> +                            name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr),

[Severity: High]
Might assigning self.name directly to the C struct without copying result in
a use-after-free?

If the DeviceBuilder was provided a short-lived string, this pointer will
become dangling once the string is dropped, as mentioned above. Should the API
require &'static CStr or duplicate the string to avoid a use-after-free?

> +                            ..bindings::led_classdev::default()
> +                        },

[ ... ]

> +    /// Returns the subleds passed to [`Device::new_multicolor`].
> +    #[inline]
> +    pub fn subleds(&self) -> &[MultiColorSubLed] {

[Severity: High]
Is it safe to return a shared reference &[MultiColorSubLed] when C code may
concurrently mutate fields?

Because MultiColorDevice is Sync, drivers might retain this slice and
concurrently read the brightness and intensity fields. Since they are plain
u32 fields, this could introduce data races.

> +        // 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() };

[ ... ]

> +#[pinned_drop]
> +impl<'bound, T: LedOps<Mode = MultiColor> + 'bound> PinnedDrop for MultiColorDevice<'bound, T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        let raw = self.classdev.get();

[ ... ]

> +        // 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,
> +            )
> +        });
> +    }
> +}

[Severity: High]
Does this KVec reconstruction cause undefined behavior upon deallocation?

Similar to the ScopeGuard above, passing num_colors as the capacity
instead of the original capacity violates the from_raw_parts() contract.
Additionally, reconstructing using KVec::from_raw_parts with
bindings::mc_subled instead of the original MultiColorSubLed type might
violate allocator provenance rules.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-rust_leds-v19-0-2a71085deae3@posteo.de?part=3