Re: [PATCH v2 1/4] leds: pca9532: check i2c_smbus_read_byte_data() return value

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Commit message description capitalization violates LED subsystem standards.

Pre-existing issues:
- [High] The patch addresses an unhandled I2C read error in `pca9532_getled()` but leaves identical, critical vulnerabilities unpatched in `pca9532_setled()` and `pca9532_gpio_get_value()`.
- [High] Input event workqueue race condition leads to a Use-After-Free during device removal.
- [High] A malformed Device Tree with duplicate beeper definitions will trigger a BUG_ON() and crash the kernel.
--

--- Patch [1]: [PATCH v2 1/4] leds: pca9532: check i2c_smbus_read_byte_data() return value ---
commit 815cd02e98d4a21b790e3fbfdbff33af4f337f2d
Author: Haibo Chen <haibo.chen@nxp.com>

leds: pca9532: check i2c_smbus_read_byte_data() return value

This commit fixes an integer underflow in pca9532_getled() by checking the
return value of i2c_smbus_read_byte_data().

> leds: pca9532: check i2c_smbus_read_byte_data() return value

[Severity: Low]
Does this commit message violate the LED subsystem capitalization rules?

The subsystem guideline requires capitalizing the description after the
subsystem prefix, but "check" is lowercase.

> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index 2d37e00e459de..e8ee17b8bd547 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -298,12 +298,15 @@ static enum pca9532_state pca9532_getled(struct pca9532_led *led)
>  	struct i2c_client *client = led->client;
>  	struct pca9532_data *data = i2c_get_clientdata(client);
>  	u8 maxleds = data->chip_info->num_leds;
> -	char reg;
> -	enum pca9532_state ret;
> +	int reg;
> +	enum pca9532_state ret = PCA9532_OFF;
>  
>  	mutex_lock(&data->update_lock);
>  	reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id));
> -	ret = (reg & LED_MASK(led->id)) >> LED_SHIFT(led->id);
> +	if (reg >= 0)
> +		ret = (reg & LED_MASK(led->id)) >> LED_SHIFT(led->id);
> +	else
> +		dev_warn(&client->dev, "failed to read LED register: %d\n", reg);
>  	mutex_unlock(&data->update_lock);
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this same issue still
exist in pca9532_setled() and pca9532_gpio_get_value()?

In pca9532_setled(), if i2c_smbus_read_byte_data() returns a negative error,
the error is truncated into the char reg variable. This value is bitwise
modified and written back to the device via i2c_smbus_write_byte_data():

drivers/leds/leds-pca9532.c:pca9532_setled() {
    ...
	char reg;
    ...
	reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id));
	/* zero led bits */
	reg = reg & ~LED_MASK(led->id);
	/* set the new value */
	reg = reg | (led->state << LED_SHIFT(led->id));
	i2c_smbus_write_byte_data(client, LED_REG(maxleds, led->id), reg);
    ...
}

Are the other 3 innocent LEDs sharing the same I2C register overwritten?

Similarly, in pca9532_gpio_get_value(), the error is cast to unsigned char
and evaluated in a boolean context:

drivers/leds/leds-pca9532.c:pca9532_gpio_get_value() {
    ...
	unsigned char reg;
    ...
	reg = i2c_smbus_read_byte_data(data->client, PCA9532_REG_INPUT(offset));
    ...
}

Should these regressions be fixed as well?

[Severity: High]
This is a pre-existing issue, but does pca9532_destroy_devices() have a
race condition leading to a use-after-free regression during device removal?

During driver unbind, pca9532_remove() calls pca9532_destroy_devices(),
which cancels the work:

drivers/leds/leds-pca9532.c:pca9532_destroy_devices() {
    ...
	cancel_work_sync(&data->work);
	data->idev = NULL;
    ...
}

However, the managed input device data->idev remains active until devres
cleanup. If an input event arrives after the work is cancelled but before
devres cleanup, does this code schedule the work again?

drivers/leds/leds-pca9532.c:pca9532_event() {
    ...
	schedule_work(&data->work);
    ...
}

If devres then frees the data structure, does the system workqueue execute
the scheduled work on a freed pointer?

[Severity: High]
This isn't a bug introduced by this patch, but can a malformed Device Tree
cause a BUG_ON() regression in pca9532_configure()?

If the device tree contains more than one node with type PCA9532_TYPE_N2100_BEEP,
the driver allocates data->idev on the first iteration. On the second iteration,
would this BUG_ON() trigger?

drivers/leds/leds-pca9532.c:pca9532_configure() {
    ...
		case PCA9532_TYPE_N2100_BEEP:
			/* PWM1 is reserved for beeper so blink will not use hardware */
			data->hw_blink = false;
			BUG_ON(data->idev);
    ...
}

Should this fail gracefully instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-led-fix-v2-0-09ad218457bd@nxp.com?part=1