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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unsound conversion of potentially aliased C pointers to `&mut usize` in `blink_set_callback`.
- [High] Relying on PinnedDrop and unmanaged led_classdev_unregister introduces a Use-After-Free (UAF) vulnerability on driver unbind if the Device instance outlives the parent device.
- [Low] The commit message description starts with a lowercase letter, violating LED subsystem conventions.
--

--- Patch [2]: [PATCH RESEND v23 2/3] rust: leds: add Mode trait ---
commit 8b1c998241db82c99cdf6f28eac6bffb4a27cef4
Author: Markus Probst <[email protected]>

rust: leds: add Mode trait

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

> rust: leds: add Mode trait

[Severity: Low]
Is this commit subject formatted correctly? The LED subsystem guidelines
require capitalizing the description after the subsystem prefix.

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> index 816db7c576894..25190486a62d0 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<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
>      /// Registers a new [`Device`].
> -    pub fn build<'bound: 'init, T: LedOps + 'bound>(
> +    pub fn build<'bound: 'init, T: LedOps<Mode = Normal> + 'bound>(
>          self,
>          parent: &'bound T::Bus,
>          ops: impl PinInit<T, Error> + 'init,

[Severity: High]
Can relying on PinnedDrop to unregister the LED device result in a
use-after-free regression?

In this build function, the code calls bindings::led_classdev_register_ext()
and later relies on bindings::led_classdev_unregister() in PinnedDrop:

rust/kernel/led/normal.rs:DeviceBuilder::build() {
                to_result(unsafe {
                    bindings::led_classdev_register_ext( ... )
                })?;
...

rust/kernel/led/normal.rs:PinnedDrop for Device {
    fn drop(self: Pin<&mut Self>) {
...
        unsafe { bindings::led_classdev_unregister(raw) };
    }

If the Device instance outlives the parent device (for example, if it is
wrapped in an Arc and kept alive by another thread), the Drop
implementation will not execute at driver unbind time.

Because led_classdev_unregister is not called during unbind, the LED sysfs
attributes would remain active. Subsequent sysfs accesses could then invoke
callbacks that dereference the freed parent device pointer.

Should devm_led_classdev_register_ext() be used to explicitly tie the
teardown to the unbind lifecycle?

[ ... ]
> @@ -107,17 +115,17 @@ fn parent(&self) -> &'bound device::Device<Bound> {
>  }
>  
[ ... ]
>  struct Adapter<T: LedOps> {
>      _p: PhantomData<T>,
>  }
>  
> -impl<T: LedOps> Adapter<T> {
> +impl<T: LedOps<Mode = Normal>> Adapter<T> {
>      /// # Safety
>      /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
>      /// `led::Device`.

[Severity: High]
Does this Adapter contain an unsound conversion of potentially aliased
C pointers?

In blink_set_callback(), the function unconditionally converts delay_on and
delay_off pointers into mutable references:

rust/kernel/led/normal.rs:blink_set_callback()
                unsafe { &mut *delay_on },
                ...
                unsafe { &mut *delay_off },

The C function led_blink_set() and its callback signature accept these
pointers without the restrict keyword. This means C callers are legally
permitted to pass the same pointer for both arguments.

Could a C caller aliasing these pointers create overlapping mutable
references and trigger undefined behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2