Re: [PATCH v20 1/3] 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] FwNode reference count leak and underflow (Use-After-Free) due to retrieving fwnode from C device in drop
- [High] FwNode reference count is decremented before unregistering the LED device, leading to a Use-After-Free during unregistration.
- [High] Lifetime erasure in `DeviceBuilder::build` allows `Device` to outlive the `name` string, causing a dangling pointer in `led_classdev.name`.
- [High] Creating `&mut usize` from shared C-provided pointers in `blink_set_callback` violates Rust's exclusive mutability guarantee (Undefined Behavior).
--

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

rust: leds: add basic led classdev abstractions
    
Implement the core abstractions needed for led class devices, including:

* `led::LedOps` - the trait for handling leds, including
  `brightness_set`, `brightness_get` and `blink_set`

* `led::DeviceBuilder` - the builder for the led class device

* `led::Device` - a safe wrapper around `led_classdev`

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> new file mode 100644
> index 0000000000000..a294c96b34dc2
> --- /dev/null
> +++ b/rust/kernel/led/normal.rs
[ ... ]
> +impl<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
> +    /// Registers a new [`Device`].
> +    pub fn build<'bound: 'a, T: LedOps + 'bound>(
> +        self,
> +        parent: &'bound T::Bus,
> +        ops: impl PinInit<T, Error> + 'a,
> +    ) -> impl PinInit<Device<'bound, T>, Error> + 'a {
> +        const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET);
> +
> +        try_pin_init!(Device {
> +            ops <- ops,
> +            classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
> +                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
> +                // `led_classdev` gets fully initialized in-place by
> +                // `led_classdev_register_ext` including `mutex` and `list_head`.
> +                unsafe {
> +                    ptr.write(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]
Does this allow `Device` to outlive the `name` string, leaving a dangling
pointer in `led_classdev.name`?

The `DeviceBuilder` accepts a short-lived `name: &'a CStr`, and the pointer
is copied into the C `led_classdev` struct. The returned `Device<'bound, T>`
only enforces `'bound: 'a` (meaning `'bound` outlives `'a`). Because the
resulting `Device` type itself does not capture the `'a` lifetime, the Rust
borrow checker allows the `Device` to outlive the `name` string.

If a driver uses a dynamically allocated local string or a short-lived format
buffer for the device name, will this leave a dangling pointer that gets
triggered upon system suspend/resume?

> +                        ..bindings::led_classdev::default()
> +                    })
> +                };
[ ... ]
> +    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 },
                 ^^^^

[Severity: High]
Does creating `&mut usize` from these shared C-provided pointers violate
Rust's exclusive mutability guarantees?

In `Adapter::blink_set_callback`, the raw pointers `delay_on` and `delay_off`
are unconditionally cast to exclusive Rust references (`&mut usize`). These
pointers are passed from the C LED core, where they directly point to the
`led_cdev->blink_delay_on` and `blink_delay_off` fields.

Since these same fields are concurrently readable by lockless sysfs functions
like `led_delay_on_show()`, they can be read concurrently while the `&mut`
reference is alive. Does this result in undefined behavior since they are not
exclusively owned by the callback?

> +                // 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 },
> +            )?;
> +            Ok(0)
> +        })
> +    }
[ ... ]
> +#[pinned_drop]
> +impl<'bound, T: LedOps + '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) };
> +
> +        if let Some(fwnode) = dev.fwnode() {
> +            // SAFETY: the reference count of `fwnode` has previously been incremented in
> +            // `led::DeviceBuilder::build`.
> +            unsafe { FwNode::dec_ref(NonNull::from(fwnode)) };
                 ^^^^

[Severity: High]
Can this cause a reference count underflow and use-after-free for the fwnode?

If `DeviceBuilder::build()` is called without an explicitly provided `fwnode`,
it does not leak a reference. However, during registration, the C core
`device_add()` automatically inherits the parent's `fwnode` if the device
has none.

Later, in `PinnedDrop::drop()`, this code blindly calls `dev.fwnode()` and
decrements its reference count. Doesn't this erroneously decrement the parent
device's `fwnode` reference count, which it never owned?

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

[Severity: High]
Is it safe to decrement the fwnode reference count before unregistering the
LED device?

The `FwNode::dec_ref()` call happens before
`bindings::led_classdev_unregister()`. If this was the last reference, the
fwnode is immediately freed. Then, `led_classdev_unregister()` calls
`device_unregister()` and subsequently `device_del()`. Inside `device_del()`,
the core driver framework accesses `dev->fwnode`.

Could this result in a use-after-free when the C core attempts to access the
fwnode to clean up software nodes and links?

> +    }
> +}

Note: The issues regarding `DeviceBuilder::build`, `blink_set_callback`, and
`PinnedDrop::drop` appear to also apply to `MultiColorDevice` in
`rust/kernel/led/multicolor.rs`.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-rust_leds-v20-0-f493ab053b4f@posteo.de?part=1