* RE: [PATCH] leds: pca9532: check i2c_smbus_read_byte_data() return value
2026-07-27 9:57 [PATCH] leds: pca9532: check i2c_smbus_read_byte_data() return value haibo.chen
@ 2026-07-27 10:04 ` Bough Chen
2026-07-27 10:08 ` sashiko-bot
1 sibling, 0 replies; 3+ messages in thread
From: Bough Chen @ 2026-07-27 10:04 UTC (permalink / raw)
To: Bough Chen (OSS), Riku Voipio, Lee Jones, Pavel Machek
Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
imx@lists.linux.dev
> -----Original Message-----
> From: Bough Chen (OSS) <haibo.chen@oss.nxp.com>
> Sent: 2026年7月27日 17:58
> To: Riku Voipio <riku.voipio@iki.fi>; Lee Jones <lee@kernel.org>; Pavel
> Machek <pavel@kernel.org>
> Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org;
> imx@lists.linux.dev; Bough Chen <haibo.chen@nxp.com>
> Subject: [PATCH] leds: pca9532: check i2c_smbus_read_byte_data() return
> value
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> Coverity report INTEGER_OVERFLOW (tainted_data_return -> cast_underflow
> -> overflow) in pca9532_getled().
> pca9532_getled() stored the return value of i2c_smbus_read_byte_data() into
> a char and used it directly to compute the LED state without checking for an
> error. i2c_smbus_read_byte_data() returns a negative error code on failure.
> On an I2C read error the negative value was assigned to an unsigned type,
> triggering an integer underflow/overflow, and a meaningless LED state was
> returned to the caller.
>
> Read the value into an int, check for the error, and fall back to PCA9532_OFF
> while emitting a warning when the read fails.
Sorry, I forget to add the fix tag:
Fixes: 28c5fe99016d ("leds: pca9532: Extend pca9532 device tree support")
Will add it into commit log if need a V2.
Regards
Haibo Chen
>
> Assisted-by: VeroCoder:vero-1 [cli]
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
> drivers/leds/leds-pca9532.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c index
> 2d37e00e459dee9f0313830f4852b01f40977311..e8ee17b8bd547a92372a8
> e74a6781b95ae831fe2 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;
> }
>
> ---
> base-commit: c5e32e86ca02b003f86e095d379b38148999293d
> change-id: 20260727-led-fix-28f085e12feb
>
> Best regards,
> --
> Haibo Chen <haibo.chen@nxp.com>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] leds: pca9532: check i2c_smbus_read_byte_data() return value
2026-07-27 9:57 [PATCH] leds: pca9532: check i2c_smbus_read_byte_data() return value haibo.chen
2026-07-27 10:04 ` Bough Chen
@ 2026-07-27 10:08 ` sashiko-bot
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-07-27 10:08 UTC (permalink / raw)
To: haibo.chen; +Cc: Frank.Li, lee, linux-leds, imx
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 <haibo.chen@nxp.com>
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/20260727-led-fix-v1-1-d635c5037109@nxp.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread