Re: [PATCH v2 4/4] leds: pca9532: fix use-after-free on unbind with N2100 beeper

Thank you for your contribution! Sashiko AI review found 4 potential issue(=
s) to consider:

New issues:
- [High] Explicitly calling `input_unregister_device()` on a device allocat=
ed via `devm_input_allocate_device()` violates input subsystem managed reso=
urce integration guidelines.
- [Low] The commit message description is not capitalized.

Pre-existing issues:
- [High] Unprotected read and write access to shared driver state (`data->p=
wm`, `data->psc`, and `led->state`) leads to data races between concurrent =
LED operations and input events.
- [High] `pca9532_probe` uses `BUG_ON(data->idev)` which allows malformed d=
evice tree data to panic the kernel.
--

--- Patch [4]: [PATCH v2 4/4] leds: pca9532: fix use-after-free on unbind w=
ith N2100 beeper ---
commit cbc774be651cc4a48a4aeff9054da59aafa6a737
Author: Haibo Chen <[email protected]>
leds: pca9532: fix use-after-free on unbind with N2100 beeper

The commit attempts to fix a use-after-free during device unbind by explici=
tly
unregistering the input device before cancelling the workqueue. This preven=
ts
new events from requeuing the work after it has been cancelled but before t=
he
devm cleanup runs.

> leds: pca9532: fix use-after-free on unbind with N2100 beeper

[Severity: Low]
This isn't a bug, but the subsystem guidelines request that the description
after the prefix is capitalized. Should this be "Fix use-after-free..."?

> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index cee398d8275b8..eeb91b7466419 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -385,6 +385,12 @@ static void pca9532_destroy_devices(struct pca9532_d=
ata *data, int n_devs)
>  			break;
>  		case PCA9532_TYPE_N2100_BEEP:
>  			if (data->idev !=3D NULL) {
> +				/*
> +				 * Unregister the input device first so that no
> +				 * new EV_SND events can re-queue the work after
> +				 * it has been cancelled.
> +				 */
> +				input_unregister_device(data->idev);

[Severity: High]
Does explicitly unregistering a device allocated with
devm_input_allocate_device() violate the managed resource integration
guidelines?

The input subsystem documentation states that explicitly calling
input_unregister_device() on a device allocated via
devm_input_allocate_device() can lead to double-unregistration.

Would it be safer to register a managed action using devm_add_action_or_res=
et()
to cancel the workqueue before calling input_register_device()? Since devres
executes actions in LIFO order, this would guarantee the input device is
cleanly unregistered before the workqueue is automatically canceled.

>  				cancel_work_sync(&data->work);
>  				data->idev =3D NULL;
>  			}

[Severity: High]
This is a pre-existing issue, but can concurrent access to the shared driver
state lead to data corruption of I2C hardware state?

The input event handler pca9532_event() can locklessly modify data->pwm:

pca9532_event() {
    ...
    data->pwm[PCA9532_PWM_ID_1] =3D 127;
}

Concurrently, pca9532_update_hw_blink() can be called from sysfs to set a
blink rate, which also modifies data->pwm locklessly:

pca9532_update_hw_blink() {
    ...
    data->psc[PCA9532_PWM_ID_1] =3D psc;
    data->pwm[PCA9532_PWM_ID_1] =3D (delay_on * PCA9532_PWM_DUTY_DIV) /
                                  (delay_on + delay_off);
}

Additionally, pca9532_set_brightness() reads led->state locklessly in
pca9532_calcpwm(). Could this data race lead to incorrect LED brightness or
blink rates when userspace triggers sysfs writes and input events at the
same time?

[Severity: High]
This is a pre-existing issue, but does pca9532_probe() allow malformed devi=
ce
tree data to panic the kernel?

If a device tree contains more than one LED node configured as
PCA9532_TYPE_N2100_BEEP, the first iteration successfully allocates and
registers data->idev. The second iteration reaches pca9532_configure() and =
hits:

pca9532_configure() {
    ...
    BUG_ON(data->idev);
}

Should the driver log an error and return -EINVAL instead of triggering a
kernel panic?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260729-led-fix-v2=
[email protected]?part=3D4