Re: [PATCH v27 1/4] 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] rust: leds: UB from concurrent &mut usize to shared blink delays
- [High] rust: leds: UAF due to unbalanced fwnode reference decrement in Device::drop
- [Low] rust: missing #[inline] on small abstraction methods Deref::deref and TryFrom::try_from
--

commit fdbf61398aae03c902e4cb268e9551172f6901a1
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` - the trait for handling leds, including
      `brightness_set`, `brightness_get` and `blink_set`
    
    * `led::DeviceBuilder` - the builder for the led class device
    
    * `led::Device` - a safe wrapper around `led_classdev`
    
    Signed-off-by: Markus Probst <markus.probst@posteo.de>

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

[ ... ]

> +impl TryFrom<u32> for Color {
> +    type Error = Error;
> +
> +    fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {

[Severity: Low]
This is a small conversion function that should be inlined. The Rust subsystem
guidelines mandate that small functions and methods in abstractions be annotated
with `#[inline]` to allow the compiler to optimize them effectively across
crate boundaries. Should we add `#[inline]` here?

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

[ ... ]

> +impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
> +    /// Registers a new [`Device`].
> +    pub fn build<'bound: 'init, T: LedOps + 'bound>(
> +        self,
> +        parent: &'bound device::Device<Bound>,
> +        ops: impl PinInit<T, Error> + 'init,
> +    ) -> impl PinInit<Device<'bound, T>, Error> + 'init {

[ ... ]

> +                // SAFETY:
> +                // - `parent.as_raw()` is guaranteed to be a pointer to a valid `device`.
> +                // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev`.
> +                to_result(unsafe {
> +                    bindings::led_classdev_register_ext(
> +                        parent.as_raw(),
> +                        ptr,
> +                        if self.name.is_none() {
> +                            &raw mut init_data
> +                        } else {
> +                            core::ptr::null_mut()
> +                        },
> +                    )
> +                })?;
> +
> +                core::mem::forget(self.fwnode); // keep the reference count incremented

[Severity: High]
If `self.fwnode` is `None`, `core::mem::forget` does nothing and no reference
count is incremented. Could this lead to a reference count underflow later
during `Device::drop` if `dev->fwnode` is automatically populated during
`led_classdev_register_ext` by subsystems like ACPI?

[ ... ]

> +impl<'bound, T: LedOps + 'bound> Deref for Device<'bound, T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {

[Severity: Low]
This is a small trait implementation method missing the `#[inline]` annotation.
Since the Rust subsystem guidelines mandate that small methods in abstractions
be annotated with `#[inline]`, should we add it here?

[ ... ]

> +    /// # Safety
> +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> +    /// `led::Device`.
> +    /// `delay_on` and `delay_off` must be valid pointers to `usize` and have
> +    /// exclusive access for the period of this function.
> +    /// This function is called on enabling hardware accelerated blinking.
> +    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) };
> +
> +            classdev.blink_set(
> +                // 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 cause undefined behavior from strict aliasing violations?

A user can write to the LED `delay_on` or `delay_off` sysfs attributes
concurrently. The sysfs store callbacks pass pointers to shared `led_classdev`
fields (such as `&led_cdev->blink_delay_off` or `&led_cdev->blink_delay_on`)
to `led_blink_set()`.

These fields can be concurrently read by the asynchronous
`led_timer_function()`, or passed concurrently as identically-aliased pointers
if multiple threads write to the sysfs attributes simultaneously, since there is
no concurrency serialization in the sysfs store paths.

This callback unconditionally casts these pointers to exclusive mutable
references (`&mut usize`), violating Rust's strict aliasing rules because the
underlying memory is accessed and aliased concurrently without exclusivity.

[ ... ]

> +#[pinned_drop]
> +impl<'bound, T: '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 safely balance the fwnode reference count?

If `self.fwnode` was `None` during `build()`, no reference count was
incremented by the Rust abstraction. However, during registration, subsystems
like ACPI can automatically bind a `fwnode` to `dev->fwnode`.

When unregistering, this blindly retrieves `dev.fwnode()`, assumes it was the
one leaked by the builder, converts it to an `ARef<FwNode>` using
`ARef::from_raw()`, and drops it. This decrements the reference count of a
`fwnode` that the Rust abstraction never incremented, leading to a
use-after-free.

Would it be better to store the `Option<ARef<FwNode>>` safely inside the
`Device` struct instead of re-reading the mutable C field?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-rust_leds-v27-0-2c296d8a8e5b@posteo.de?part=1