Re: [PATCH v21 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 `blink_set_callback` creates multiple mutable references (`&mut usize`) to the same shared memory concurrently, violating Rust's strict aliasing rules.
- [High] The `DeviceBuilder::build` method erases the lifetime of the `name` string, allowing the `led_classdev.name` pointer to outlive the string and causing a Use-After-Free.
- [High] Unsound `FwNode` reference count management in `Device::drop` retrieves the `fwnode` directly from the mutable C structure, leading to refcount underflow and Use-After-Free.
--

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

rust: leds: add basic led classdev abstractions

Implement the core abstractions needed for led class devices, including
LedOps, DeviceBuilder, and Device.

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> new file mode 100644
> index 0000000000000..2769f690bb245
> --- /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 {

[Severity: High]
Is the lifetime bound <'bound: 'a> backwards here?

By specifying 'bound: 'a, the returned Device ('bound) is explicitly permitted
to outlive the builder's string lifetime ('a).

Because the raw pointer to this string is written directly into the
led_classdev.name field, a driver could initialize a Device with a temporary
string, which drops the string while leaving led_cdev->name dangling. Can
this lead to a use-after-free?

[ ... ]

> +    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]
Could this lead to a strict aliasing violation by creating multiple mutable
references to the same shared memory concurrently?

In the C LED subsystem, sysfs write operations to delay_on and delay_off can
invoke led_blink_set() concurrently across different threads without holding
locks on the device.

This passes pointers to the shared led_cdev->blink_delay_on and
led_cdev->blink_delay_off fields directly into the blink_set callback. By
unconditionally casting these raw C pointers to &mut usize in Rust, the
callback can create overlapping mutable references to the exact same memory
locations.

[ ... ]

> +#[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 unsoundly steal a reference count from the C field dev->fwnode?

In DeviceBuilder::build, self.fwnode is passed to C and its refcount is
leaked via core::mem::forget. However, if self.fwnode was initially None, no
reference count is leaked.

If dev->fwnode is subsequently populated by external C code, this
PinnedDrop implementation blindly constructs an ARef and drops it. Since
Rust never incremented this refcount, could this result in an underflow
and an eventual use-after-free of the FwNode?

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