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 allocated via `devm_input_allocate_device()` violates input subsystem managed resource integration guidelines.
- [Low] The commit message description is not capitalized.

Pre-existing issues:
- [High] Unprotected read and write access to shared driver state (`data->pwm`, `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 device tree data to panic the kernel.
--

--- Patch [4]: [PATCH v2 4/4] leds: pca9532: fix use-after-free on unbind with 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 explicitly
unregistering the input device before cancelling the workqueue. This prevents
new events from requeuing the work after it has been cancelled but before the
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_data *data, int n_devs)
>  			break;
>  		case PCA9532_TYPE_N2100_BEEP:
>  			if (data->idev != 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_reset()
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 = 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] = 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] = psc;
    data->pwm[PCA9532_PWM_ID_1] = (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 device
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?

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