Re: [PATCH v14 2/2] platform: Add initial synology microp driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Use-after-free of dynamically allocated LED device names.
- [High] Concurrent operations across different LEDs can interleave multi-byte commands over the shared serial bus.
- [Medium] Setting a non-zero brightness fails to clear the hardware blink state.
- [Medium] StatusLedHandler::blink_set fails to update its internal blink state when stopping a blink while subled intensity is 0.
- [Low] Dependency on a non-existent Kconfig symbol 'RUST_SERIAL_DEV_BUS_ABSTRACTIONS'.
--

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

platform: Add initial synology microp driver

Add a initial synology microp driver, written in Rust.
The driver targets a microcontroller found in Synology NAS devices. It
currently only supports controlling of the power led, status led, alert
led and usb led.

> diff --git a/drivers/platform/synology_microp/Kconfig b/drivers/platform/synology_microp/Kconfig
> new file mode 100644
> index 0000000000000..8878cfb7bcddb
> --- /dev/null
> +++ b/drivers/platform/synology_microp/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config SYNOLOGY_MICROP
> +	tristate "Synology Microp driver"
> +	depends on LEDS_CLASS && LEDS_CLASS_MULTICOLOR
> +	depends on RUST_SERIAL_DEV_BUS_ABSTRACTIONS

[Severity: Low]
Does the dependency RUST_SERIAL_DEV_BUS_ABSTRACTIONS exist?

Searching the tree doesn't show this symbol, which might cause silent build
failures or make the driver unselectable.

[ ... ]
> diff --git a/drivers/platform/synology_microp/command.rs b/drivers/platform/synology_microp/command.rs
[ ... ]
> +impl Command {
> +    pub(crate) fn write(self, dev: &serdev::Device<Bound>) -> Result {
> +        dev.write_all(
> +            match self {

[Severity: High]
Can concurrent LED operations interleave multi-byte commands over the shared
serial bus?

Since LedHandler and StatusLedHandler can be invoked concurrently via sysfs,
they lock their own local blink mutex but do not lock a shared driver-level
mutex before calling Command::write().

Since dev.write_all() and dev.wait_until_sent() are called concurrently
without mutual exclusion for multi-byte sequences (for example, the 3-byte
sequence for AlertLed), the bytes could interleave.

Could this send corrupted command streams to the hardware?

[ ... ]
> diff --git a/drivers/platform/synology_microp/led.rs b/drivers/platform/synology_microp/led.rs
[ ... ]
> +#[pin_data]
> +pub(crate) struct Data<'bound> {
> +    #[pin]
> +    status: led::MultiColorDevice<'bound, StatusLedHandler>,
> +    power_name: CString,
> +    #[pin]
> +    power: led::Device<'bound, LedHandler>,

[Severity: Critical]
Does declaring power_name before power lead to a use-after-free during
device unbind?

Rust drops struct fields strictly in declaration order. If power_name is
dropped before power, the underlying C struct led_classdev (which only
stores the raw string pointer) might be left with a dangling pointer before
the LED device is finally unregistered.

[ ... ]
> +                alert: {
> +                    if let Some(color) = model.led_alert {
> +                        let name = CString::try_from_fmt(fmt!(
> +                            "synology:{}:alarm",
> +                            color.as_c_str().to_str()?
> +                        ))?;
> +                        Some(KBox::pin_init(
> +                            led::DeviceBuilder::new().color(color).name(&name).build(

[Severity: Critical]
Is the name variable dropped at the end of this block, resulting in a
dangling pointer?

The C struct led_classdev does not copy the string assigned to its name
field, it merely stores the raw pointer.

Once this block ends, the local CString goes out of scope, which leaves a
dangling pointer in the LED device for its entire lifetime. This could cause
memory corruption when the device name is accessed via sysfs.

[ ... ]
> +    fn brightness_set<'bound>(
> +        &self,
> +        dev: &'bound Self::Bus,
> +        _classdev: &led::Device<'bound, Self>,
> +        brightness: u32,
> +    ) -> Result<()> {
> +        let mut blink = self.blink.lock();
> +        (self.command)(if brightness == 0 {
> +            *blink = false;
> +            State::Off
> +        } else if *blink {
> +            State::Blink
> +        } else {

[Severity: Medium]
Should setting a non-zero brightness clear the active hardware blink state?

According to the LED subsystem semantics, setting a non-zero brightness
should clear any active hardware blinking and set the LED to solid on.

Here, if the internal blink state is true, the driver commands State::Blink
instead of solid on, and leaves the internal blink variable as true. This
prevents userspace from stopping a blink by simply setting the brightness.

[ ... ]
> +        Command::StatusLed(
> +            color,
> +            if subled_brightness == 0 {
> +                State::Off
> +            } else if *blink {
> +                State::Blink
> +            } else {

[Severity: Medium]
Similarly, for the multicolor status LED, should a non-zero subled
brightness clear the active hardware blink state instead of continuing
to command State::Blink?

[ ... ]
> +    fn blink_set<'bound>(
> +        &self,
> +        dev: &'bound Self::Bus,
> +        classdev: &led::MultiColorDevice<'bound, Self>,
> +        delay_on: &mut usize,
> +        delay_off: &mut usize,
> +    ) -> Result<()> {
> +        let mut blink = self.blink.lock();
> +        *blink = true;
> +
> +        let (color, subled_intensity) = if classdev.subleds()[1].intensity == 0 {
> +            (StatusLedColor::Green, classdev.subleds()[0].intensity)
> +        } else {
> +            (StatusLedColor::Amber, classdev.subleds()[1].intensity)
> +        };
> +        Command::StatusLed(
> +            color,
> +            if *delay_on == 0 && *delay_off != 0 {
> +                *blink = false;
> +                State::Off
> +            } else if subled_intensity == 0 {
> +                State::Off
> +            } else if *delay_on != 0 && *delay_off == 0 {

[Severity: Medium]
If a user attempts to stop blinking but the subled intensity is 0, does the
internal blink state diverge from the requested state?

The blink state is unconditionally set to true at the beginning of the
function. If the subled_intensity == 0 branch is taken, it commands
State::Off but fails to set blink to false.

Later, if the subled intensity increases, brightness_set will see that blink
is still true and erroneously command the LED to blink instead of staying
solidly on.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-synology_microp_initial-v14-0-1377d425b24b@posteo.de?part=2