Re: [PATCH] leds: pca9532: check i2c_smbus_read_byte_data() return value

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

Pre-existing issues:
- [High] The patch fixes an unchecked I2C read in `pca9532_getled()` but misses an identical flaw in `pca9532_setled()`, where I2C read failures cause register corruption.
- [High] `pca9532_gpio_get_value()` ignores I2C read errors and silently returns meaningless GPIO states.
- [High] A race condition during device unbinding allows the input device to re-queue work after it has been cancelled, leading to a Use-After-Free of the driver data.
--

--- Patch [1]: [PATCH] leds: pca9532: check i2c_smbus_read_byte_data() return value ---
commit 87ac49dee89ea1bd1132b25134fdbdfad3eb7c48
Author: Haibo Chen <[email protected]>

leds: pca9532: check i2c_smbus_read_byte_data() return value

This commit fixes an integer underflow and overflow issue in pca9532_getled()
by checking the return value of i2c_smbus_read_byte_data(). It ensures that a
failed I2C read returns a fallback state and emits a warning instead of
computing a meaningless LED state.

> 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);

[Severity: High]
This isn't a bug introduced by this patch, but this commit fixes an
unchecked I2C read in pca9532_getled() while missing an identical flaw in
pca9532_setled().

If i2c_smbus_read_byte_data() fails when called from pca9532_set_brightness(),
it returns a negative error code (e.g., -EIO). In pca9532_setled(), this is
truncated to a char, bitwise modified, and blindly written back to the device:

drivers/leds/leds-pca9532.c:pca9532_setled() {
    ...
    reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id));
    /* zero led bits */
    reg = reg & ~LED_MASK(led->id);
    ...
}

Since one I2C register controls four LEDs, doesn't writing this invalid value
back corrupt the state of up to three other LEDs sharing the same register?


[Severity: High]
This is a pre-existing issue, but pca9532_gpio_get_value() also ignores I2C
read errors and silently returns meaningless GPIO states.

When any subsystem or userspace reads the GPIO value, an I2C error from
i2c_smbus_read_byte_data() is truncated to an unsigned byte, and a bit is
extracted and returned as a valid GPIO state:

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

    return !!(reg & (1 << (offset % 8)));
}

Could this mask hardware errors and cause userspace or kernel consumers to
silently receive corrupt data, leading to logic errors?


[Severity: High]
This is a pre-existing issue, but there appears to be a race condition during
device unbinding that allows the input device to re-queue work after it has
been cancelled, leading to a Use-After-Free of the driver data.

The driver uses devm_input_allocate_device() for its input device, so it
remains registered until devres cleanup runs AFTER .remove() finishes. However,
.remove() calls pca9532_destroy_devices() which cancels the workqueue:

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

In the window between cancel_work_sync() returning and devres unregistering the
input device, if userspace injects an EV_SND event, pca9532_event() can
re-schedule data->work:

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

Once devres frees data, won't the workqueue dereference a freed pointer,
causing a kernel crash or memory corruption?

>  	mutex_unlock(&data->update_lock);
>  	return ret;
>  }

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