Re: [PATCH v19 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] The `PinnedDrop` implementation for `Device` unconditionally retrieves the `FwNode` from the C device and constructs an `ARef<FwNode>` to decrement its refcount, which can lead to refcount underflow and use-after-free.
- [High] `DeviceBuilder::name` accepts a temporary string reference that is embedded into the C `led_classdev` struct, but the resulting `Device` does not capture this lifetime, allowing the string to be freed while the kernel retains a dangling pointer.
- [High] The `blink_set_callback` concurrently creates exclusive mutable references (`&mut usize`) from shared memory pointers without locks, triggering Undefined Behavior.
--

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

rust: leds: add basic led classdev abstractions

This commit introduces the core abstractions needed for LED class devices in
Rust. It implements traits for handling LEDs, a builder for the LED class
device, and a safe wrapper around led_classdev.

> diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> new file mode 100644
> --- /dev/null
> +++ b/rust/kernel/led.rs

[ ... ]

> +impl<'a> DeviceBuilder<'a, Named> {
> +    /// Sets the name of the led.
> +    ///
> +    /// Setting this will prevent the fwnode from being used and prevents automatic name
> +    /// composition.
> +    #[inline]
> +    pub fn name(self, name: &'a CStr) -> Self {

[Severity: High]
Does this builder API allow the C string to be freed while the kernel retains
a dangling pointer?

The name method accepts a string reference bounded by a lifetime, but the
resulting Device struct does not capture this lifetime. The raw string pointer
is written into the long-lived led_classdev C struct in DeviceBuilder::build.

If the caller dynamically allocates a CString and drops it immediately after
pin-initialization, the C LED subsystem could later access a dangling pointer
when sysfs triggers or error messages run.

> +        Self {
> +            name: Some(name),
> +            ..self
> +        }
> +    }
> +}

[ ... ]

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

[ ... ]

> +    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 },
> +                // 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]
Can this lead to concurrent aliasing mutable references and undefined
behavior?

The C LED subsystem can invoke the blink callback without locks. For example,
when users write to the sysfs delay_on file, led_delay_on_store() calls
led_blink_set() and passes pointers to shared struct fields like
&led_cdev->blink_delay_off.

If multiple CPUs write to the sysfs file concurrently, they could invoke this
callback simultaneously with the exact same pointer. Creating exclusive mutable
references from these shared pointers concurrently violates Rust's mutable
aliasing rules.

> +            )?;
> +            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::Device::new`.
> +            .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });

[Severity: High]
Can this lead to a refcount underflow and use-after-free of the firmware node?

DeviceBuilder::build transfers ownership and leaks the self.fwnode refcount
only if it was provided. However, this drop implementation unconditionally
retrieves the FwNode from the C device and constructs an ARef to decrement its
refcount.

The C driver core can independently populate dev->fwnode, such as via ACPI
bindings during device_add. If the C code populates the node while
self.fwnode was not set, this code will blindly construct an ARef and drop it,
underflowing the refcount of an active firmware node.

> +
> +        // 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/20260603-rust_leds-v19-0-2a71085deae3@posteo.de?part=1