Re: [PATCH v15 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:
- [High] The Kconfig symbol `SYNOLOGY_MICROP` depends on a non-existent symbol `RUST_SERIAL_DEV_BUS_ABSTRACTIONS`.
- [High] The driver is compiled but not linked into a loadable module when configured as `CONFIG_SYNOLOGY_MICROP=m`.
- [High] The `alert` LED's name string is allocated as a local variable and dropped before the LED device finishes its lifetime, causing a Use-After-Free.
- [High] The `power_name` string is dropped before the `power` LED device, creating a dangling pointer during teardown.
- [Medium] Writing a solid brightness value fails to disable hardware blinking.
--

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

platform: Add initial synology microp driver

This patch adds an initial Rust-based driver for the microcontroller found
in Synology NAS devices. It currently supports controlling the power, status,
alert, and USB LEDs.

> diff --git a/drivers/platform/synology_microp/Kconfig b/drivers/platform/synology_microp/Kconfig
> new file mode 100644
> --- /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: High]
Does RUST_SERIAL_DEV_BUS_ABSTRACTIONS exist in the kernel tree? If not, this
driver cannot be enabled under any configuration and becomes dead code.

> diff --git a/drivers/platform/synology_microp/Makefile b/drivers/platform/synology_microp/Makefile
> new file mode 100644
> --- /dev/null
> +++ b/drivers/platform/synology_microp/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-y += synology_microp.o

[Severity: High]
If the driver is configured as a loadable module (CONFIG_SYNOLOGY_MICROP=m),
will this obj-y assignment compile the objects into a built-in archive
instead of linking them into a .ko module?

> diff --git a/drivers/platform/synology_microp/led.rs b/drivers/platform/synology_microp/led.rs
> new file mode 100644
> --- /dev/null
> +++ b/drivers/platform/synology_microp/led.rs
[ ... ]
> @@ -21,11 +21,18 @@ use crate::{
>  
>  #[pin_data]
>  pub(crate) struct Data<'bound> {
>      #[pin]
>      status: led::MultiColorDevice<'bound, StatusLedHandler>,
>      power_name: CString,
>      #[pin]
>      power: led::Device<'bound, LedHandler>,

[Severity: High]
In Rust, struct fields are dropped top-to-bottom. Could power_name be freed
before the power LED device is unregistered? This might leave a dangling
pointer during driver teardown if the LED subsystem accesses the freed name.

[ ... ]
> @@ -71,21 +78,21 @@ impl<'bound> Data<'bound> {
>                  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(
>                                  dev,

[Severity: High]
Does passing a reference to the local name string here cause a use-after-free?

The name variable is dropped at the end of the if let block, but the kernel
LED core may retain and access the pointer after probing finishes.

[ ... ]
> @@ -167,23 +174,23 @@ impl LedOps for LedHandler {
>      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 {
>              State::On
>          })

[Severity: Medium]
When a solid brightness (>0) is requested via sysfs, this logic preserves
the hardware blinking state. Does this violate the LED subsystem API, which
expects setting brightness to disable hardware blinking?

[ ... ]
> @@ -240,23 +247,23 @@ impl LedOps for StatusLedHandler {
>      fn brightness_set<'bound>(
>          &self,
>          dev: &'bound Self::Bus,
>          classdev: &led::MultiColorDevice<'bound, Self>,
>          brightness: u32,
>      ) -> Result<()> {
[ ... ]
>          Command::StatusLed(
>              color,
>              if subled_brightness == 0 {
>                  State::Off
>              } else if *blink {
>                  State::Blink
>              } else {
>                  State::On
>              },
>          )

[Severity: Medium]
Similarly to LedHandler, does this fail to clear the blink state when
subled_brightness > 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-synology_microp_initial-v15-0-4b3d7367c7cf@posteo.de?part=2