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

Markus Probst <[email protected]> Fri, 24 Apr 2026 11:58:55 +0000
Newsgroups	dev.linux.lists.sashiko
Message-ID	<[email protected]>
On Thu, 2026-04-23 at 21:58 +0000, [email protected] wrote:
> Sashiko AI review found 7 potential issue(s):
> - [Critical] Use-After-Free of the dynamically allocated Alert LED CString.
> - [High] Logic errors in LED brightness control prevent clearing the hardware blink state.
> - [High] Use-After-Free of `power_name` during teardown due to incorrect struct field declaration order.
> - [High] Concurrent accesses to the shared serdev bus are not serialized, leading to corrupted command sequences.
> - [Medium] The SYNOLOGY_MICROP config depends on a non-existent symbol RUST_SERIAL_DEV_BUS_ABSTRACTIONS.
> - [Medium] Typo in the compatible string for the DS223 model prevents driver binding.
> - [Low] Missing Device Tree Binding YAML file referenced in MAINTAINERS.
> --
Since this is an AI reply, I am not sure if I am supposed to reply to
it. Anyway, the answers:

> 
> commit 47c26ce6bffc3c75c3df285181d595c18b827e6e
> Author: Markus Probst <[email protected]>
> 
> 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.
> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -25546,6 +25546,13 @@ F:	drivers/dma-buf/sync_*
> >  F:	include/linux/sync_file.h
> >  F:	include/uapi/linux/sync_file.h
> >  
> > +SYNOLOGY MICROP DRIVER
> > +M:	Markus Probst <[email protected]>
> > +L:	[email protected]
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/embedded-controller/synology,ds918p-microp.yaml
> 
> Is this YAML file missing from the patch? It doesn't appear to be included
> in the commit, which will cause Device Tree binding checks to miss the new
> schema.
It was added in the previous patch.

> 
> > diff --git a/drivers/platform/synology_microp/Kconfig b/drivers/platform/synology_microp/Kconfig
> > --- /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
> 
> Does RUST_SERIAL_DEV_BUS_ABSTRACTIONS exist anywhere in the tree?
> 
> Depending on a non-existent symbol causes a silent build failure where the
> dependency can never be satisfied, rendering the driver unbuildable.
Its in the serdev dependency of the patch.

> 
> > diff --git a/drivers/platform/synology_microp/command.rs b/drivers/platform/synology_microp/command.rs
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/command.rs
> [ ... ]
> > +impl Command {
> > +    pub(crate) fn write(self, dev: &serdev::Device<Bound>) -> Result {
> > +        dev.write_all(
> 
> Does this code prevent concurrent writes to the serial bus?
> 
> If user space modifies the state of multiple LEDs concurrently, the LED
> subsystem will invoke their callbacks simultaneously. Without a shared lock
> protecting the single serdev::Device instance, the multi-byte command
> sequences can become interleaved and corrupt the serial protocol.
If I send it as one buffer, I expect it to be sent in this order, even
if another thread trys to do the same.

Even if the whole buffer is not accepted at once. serdev internally
locks a mutex.

> 
> > diff --git a/drivers/platform/synology_microp/led.rs b/drivers/platform/synology_microp/led.rs
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/led.rs
> [ ... ]
> > +#[pin_data]
> > +pub(crate) struct Data {
> > +    #[pin]
> > +    status: Devres<led::MultiColorDevice<StatusLedHandler>>,
> > +    power_name: CString,
> > +    #[pin]
> > +    power: Devres<led::Device<LedHandler>>,
> > +}
> 
> Does this struct declaration order create a use-after-free during driver
> unbind?
> 
> In Rust, struct fields are dropped strictly in declaration order. Because
> power_name is declared before power, its backing string buffer is freed
> before the Devres destructor unregisters the LED from the C subsystem.
> 
> This creates a time window where the active led_classdev holds a dangling
> pointer to its name.
The CString is only ever used on register and is copied by the led
subsystem. The field does not have to outlive the `led::Device`.

> 
> [ ... ]
> > +impl Data {
> > +    pub(super) fn register<'a>(
> > +        dev: &'a serdev::Device<Bound>,
> > +        model: &'a Model,
> > +    ) -> impl PinInit<Self, Error> + 'a {
> > +        pin_init_scope(move || {
> > +            if let Some(color) = model.led_alert {
> > +                let name = CString::try_from_fmt(fmt!("{}:alarm", color.as_c_str().to_str()?))?;
> > +                devres::register(
> > +                    dev.as_ref(),
> > +                    led::DeviceBuilder::new().color(color).name(&name).build(
> 
> Could this leak a dangling pointer to the LED subsystem?
> 
> The name variable is strictly local to this if let block. When the block
> ends, the string is dropped and its backing memory is freed, but the kernel's
> LED framework expects the name string pointer to remain valid for the
> lifetime of the device.
The CString is only ever used on register and is copied by the led
subsystem. The field does not have to outlive the `led::Device`.

> 
> [ ... ]
> > +    fn brightness_set(
> > +        &self,
> > +        dev: &Self::Bus,
> > +        _classdev: &led::Device<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 {
> > +            State::On
> > +        })
> 
> If brightness is non-zero, shouldn't this disable any active blinking and set
> the LED solidly ON?
> 
> Evaluating else if *blink when brightness > 0 means the LED continues to
> blink and the internal boolean flag is never cleared.
No, only brightness = 0 disables blinking.
> 
> [ ... ]
> > +    fn blink_set(
> > +        &self,
> > +        dev: &Self::Bus,
> > +        _classdev: &led::Device<Self>,
> > +        delay_on: &mut usize,
> > +        delay_off: &mut usize,
> > +    ) -> Result<()> {
> > +        let mut blink = self.blink.lock();
> > +
> > +        (self.command)(if *delay_on == 0 && *delay_off != 0 {
> > +            State::Off
> 
> Should this path also reset *blink to false when blinking is explicitly
> stopped?
You are right on this one.

> 
> [ ... ]
> > +    fn brightness_set(
> > +        &self,
> > +        dev: &Self::Bus,
> > +        classdev: &led::MultiColorDevice<Self>,
> > +        brightness: u32,
> > +    ) -> Result<()> {
> > +        let mut blink = self.blink.lock();
> > +        if brightness == 0 {
> > +            *blink = false;
> > +        }
> > +
> > +        let (color, subled_brightness) = if classdev.subleds()[1].intensity == 0 {
> > +            (StatusLedColor::Green, classdev.subleds()[0].brightness)
> > +        } else {
> > +            (StatusLedColor::Orange, classdev.subleds()[1].brightness)
> > +        };
> > +
> > +        Command::StatusLed(
> > +            color,
> > +            if subled_brightness == 0 {
> > +                State::Off
> > +            } else if *blink {
> > +                State::Blink
> > +            } else {
> > +                State::On
> > +            },
> 
> Similarly, does this fail to turn a blinking LED solidly ON when the
> brightness is non-zero? It looks like it will just keep blinking because the
> blink state isn't cleared.
No, only brightness = 0 disables blinking.
> 
> > diff --git a/drivers/platform/synology_microp/synology_microp.rs b/drivers/platform/synology_microp/synology_microp.rs
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/synology_microp.rs
> [ ... ]
> > +        // rtd1619b
> > +        (DeviceId::new(c"synolody,ds223-microp"), Model::new().led_usb_copy()),
> 
> This isn't a bug, but there's a typo in the vendor prefix ("synolody"). This
> will prevent the driver from matching the device tree node for the ds223.
Yes, that is not supposed to be there.

Thanks
- Markus Probst
signature.asc (application/pgp-signature, 870 B)