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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Refcount underflow and use-after-free of `fwnode` via incorrect lifecycle management.
- [High] Lifetime erasure in `DeviceBuilder::build` leaves a dangling pointer to the LED name string, causing Use-After-Free.
- [High] Mutable aliasing of shared C pointers in `blink_set_callback` triggers Undefined Behavior.
--

--- Patch [1]: [PATCH RESEND v23 1/3] rust: leds: add basic led classdev abstractions ---
commit 6f6f8b461cc55c0944a56c43b46dbc985385115e
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, led::DeviceBuilder, and led::Device.

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> index 0000000000000..816db7c576894
> --- /dev/null
> +++ b/rust/kernel/led/normal.rs
> @@ -11,9 +11,6 @@
>  /// This structure represents the Rust abstraction for a led class device.
>  #[pin_data(PinnedDrop)]
>  pub struct Device<'bound, T: LedOps + 'bound> {
>      #[pin]
>      ops: T,
>      #[pin]
>      classdev: Opaque<bindings::led_classdev>,

[Severity: High]
Does this struct need to carry the 'init lifetime from DeviceBuilder?

If DeviceBuilder::name takes a &'init CStr and stores it, and then 
DeviceBuilder::build extracts this as a raw pointer:

rust/kernel/led/normal.rs:DeviceBuilder::build() {
    ...
    name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr),
    ...
}

But the resulting Device<'bound, T> does not carry the 'init lifetime. 
This could allow the Rust borrow checker to drop the original string while 
the Device remains fully active. If a driver uses a local string for the name, 
will it be freed, leaving led_classdev.name pointing to unallocated memory 
and causing a use-after-free?

[ ... ]
> @@ -180,18 +180,11 @@ impl<T: LedOps> Adapter<T> {
>              // 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 },

[Severity: High]
Does this code create overlapping mutable references when concurrent sysfs 
writes occur?

The LED core's led_blink_set is invoked directly from sysfs writes 
(delay_on_store and delay_off_store) without locking. When sysfs writes occur 
concurrently, multiple threads can pass pointers to the shared 
led_cdev->blink_delay_off or led_cdev->blink_delay_on members into 
led_blink_set simultaneously.

Casting these raw pointers into exclusive mutable references (&mut usize) 
without synchronization could result in two active overlapping &mut 
references existing simultaneously on different threads, which is undefined 
behavior in Rust.

>              )?;
>              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) };
>  
>          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]
Will this lead to a reference count underflow if DeviceBuilder was created 
without an fwnode (self.fwnode = None)?

If the kernel dynamically assigns an fwnode to the device (e.g., via ACPI or 
parent inheritance), dev.fwnode() during drop will return Some(fwnode). 
The PinnedDrop implementation unconditionally wraps this pointer in 
ARef::from_raw and drops it, stealing a reference count it never owned. 

Since the C device_unregister function natively cleans up its own fwnode 
reference, could this additional drop cause a critical underflow and 
subsequent use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-rust_leds-v23-0-b2fea4290ee6@posteo.de?part=1