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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect lifecycle management of the `FwNode` reference count in `Device::drop` leads to reference count underflow and Use-After-Free.
- [Medium] Lifetime erasure in `DeviceBuilder::build` allows a short-lived `CStr` to be used for the LED device name, resulting in a dangling pointer.
- [Low] Commit message format and naming convention violation for the LED subsystem.
--

--- Patch [1]: [PATCH v23 1/3] rust: leds: add basic led classdev abstractions ---
commit e26d9a00db56264a527df58257bc8e40409dcdca
Author: Markus Probst <[email protected]>

rust: leds: add basic led classdev abstractions
    
Implement the core abstractions needed for led class devices, including
LedOps, DeviceBuilder, and Device.

> rust: leds: add basic led classdev abstractions

[Severity: Low]
This isn't a bug, but the commit message violates the mandated subsystem
format. Should it be capitalized after the subsystem prefix and ordered
as "leds: rust: Add basic led classdev abstractions"?

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

[ ... ]

> @@ -33,26 +33,26 @@ impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
>                  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: Medium]
Does this code introduce a dangling pointer by erasing the lifetime of the
name field?

The short-lived 'init lifetime on CStr is converted to a raw pointer and
permanently stored in the led_classdev structure. The returned
Device<'bound, T> entirely erases the 'init lifetime constraint.

This permits safe Rust code to pass a temporary CString, build the device,
and drop the CString while the Device remains registered, leaving a
dangling pointer in led_classdev.name.

>                          ..bindings::led_classdev::default()
>                      })
>                  };

[ ... ]

> @@ -211,20 +211,20 @@ impl<T: LedOps> Adapter<T> {
>  #[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) };
>  
>          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 lifecycle management of the FwNode reference count lead to an
underflow and use-after-free?

If an LED device is built without explicitly providing a firmware node,
self.fwnode is None. The device is then registered, and the kernel driver
core invokes platform notifiers which can dynamically discover and bind a
firmware node to the device.

Later, when the Rust Device is dropped, it unconditionally fetches the node
via dev.fwnode(), converts it to an ARef, and drops it. Because Rust never
incremented the refcount for this dynamically attached node, dropping the
ARef performs an unbalanced decrement.

Would it be safer for the Device struct to explicitly store its own
Option<ARef<FwNode>> instead of blindly trusting dev.fwnode()?

>  
>          // 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/[email protected]?part=1