Re: [PATCH v18 2/3] rust: leds: add Mode trait

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Flawed fwnode reference count management in `led::Device` leads to refcount underflow or memory leaks.
- [High] Inverted lifetime bound `'bound: 'a` in `DeviceBuilder::build` allows the `Device` to outlive its name string, causing a Use-After-Free.
--

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

rust: leds: add Mode trait
    
Add the led::Mode trait to allow for other types of led class devices
in led::LedOps.

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> index 09244133f14c8..2ffe65bb9dc2c 100644
> --- a/rust/kernel/led/normal.rs
> +++ b/rust/kernel/led/normal.rs

[ ... ]

> @@ -20,7 +28,7 @@ pub struct Device<'bound, T: LedOps + 'bound> {
>  
>  impl<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
>      /// Registers a new [`Device`].
> -    pub fn build<'bound: 'a, T: LedOps + 'bound>(
> +    pub fn build<'bound: 'a, T: LedOps<Mode = Normal> + 'bound>(

[Severity: High]
Does this inverted lifetime bound ('bound: 'a) allow the device to outlive its
name string?

It appears that a caller can safely initialize a long-lived device (e.g.
'static) using a short-lived local string, because the returned PinInit does
not capture 'a.

Since the short-lived name pointer is assigned to the persistent C struct:

    name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr),

Does dropping the local string leave a dangling pointer in the C struct,
causing a use-after-free when the LED subsystem accesses led_cdev->name?

[Severity: High]
Can the fwnode reference count management here lead to underflows or memory
leaks?

In DeviceBuilder::build(), if self.fwnode is Some, its ARef is leaked:

    core::mem::forget(self.fwnode); // keep the reference count incremented

But if it was None, no reference is leaked.

>          self,
>          parent: &'bound T::Bus,
>          ops: impl PinInit<T, Error> + 'a,

[ ... ]

> @@ -209,7 +217,7 @@ impl<T: LedOps> Adapter<T> {
>  }
>  
>  #[pinned_drop]
> -impl<'bound, T: LedOps + 'bound> PinnedDrop for Device<'bound, T> {
> +impl<'bound, T: LedOps<Mode = Normal> + 'bound> PinnedDrop for Device<'bound, T> {

[Severity: High]
When <Device as PinnedDrop>::drop attempts to recover the fwnode reference from
the mutable C field dev->fwnode:

    let _fwnode = dev
        .fwnode()
        .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });

If self.fwnode was None, but the C core populates dev->fwnode (e.g. device_add
sets it to parent->fwnode), will PinnedDrop steal and drop a reference it does
not own, causing a refcount underflow?

Conversely, if self.fwnode was Some, but dev->fwnode is later cleared or
overwritten by C code, will the ARef never be dropped, causing a memory leak?

>      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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-rust_leds-v18-0-d07102ba5170@posteo.de?part=2