Re: [PATCH v2] leds: pca9532: Fix inverted GPIO output polarity

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Silent register state corruption due to unchecked I2C errors in pca9532_setled().
- [High] I2C read errors are silently masked and truncated in pca9532_gpio_get_value(), reporting invalid GPIO states.
- [High] A race condition during device removal can cause a use-after-free if an input event occurs after the workqueue is cancelled.
--

commit b1c20f70bc549eb29df798aedbdcc982c3294090
Author: Cosmo Chou <chou.cosmo@gmail.com>

leds: pca9532: Fix inverted GPIO output polarity

This patch corrects the GPIO output polarity in the pca9532 driver.
It fixes an issue where a requested logical high incorrectly enabled
the LED output driver, pulling the open-drain pin low, and updates
the input direction configuration to match.

> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index f3bf59495b68d..2d37e00e459de 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -327,9 +327,9 @@ static int pca9532_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
>  	struct pca9532_led *led = &data->leds[offset];
>  
>  	if (val)
> -		led->state = PCA9532_ON;
> -	else
>  		led->state = PCA9532_OFF;
> +	else
> +		led->state = PCA9532_ON;
>  
>  	pca9532_setled(led);

[Severity: High]
This is a pre-existing issue, but does pca9532_setled() handle I2C read
failures safely?

When pca9532_setled() is called here, it performs an unchecked read:

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);
	/* set the new value */
	reg = reg | (led->state << LED_SHIFT(led->id));
	i2c_smbus_write_byte_data(client, LED_REG(maxleds, led->id), reg);
    ...
}

If i2c_smbus_read_byte_data() fails and returns a negative error code
like -EIO, this value is stored directly into a char, truncating it.

Could this result in masking the corrupted data and blindly writing it back,
permanently corrupting the state of the other three pins sharing this same
8-bit register?

> @@ -349,7 +349,7 @@ static int pca9532_gpio_get_value(struct gpio_chip *gc, unsigned offset)

[Severity: High]
This isn't a bug introduced by this patch, but does pca9532_gpio_get_value()
silently mask I2C read errors and report invalid GPIO states?

Looking at pca9532_gpio_get_value():

drivers/leds/leds-pca9532.c:pca9532_gpio_get_value() {
    ...
	unsigned char reg;

	reg = i2c_smbus_read_byte_data(data->client, PCA9532_REG_INPUT(offset));

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

The result of i2c_smbus_read_byte_data() is stored in an unsigned char.
If the I2C read fails, the negative error code is truncated into a positive
8-bit value, which is then evaluated as valid GPIO input data.

Since the gpiolib .get callback returns an int and supports returning
negative error codes, could this truncation cause downstream drivers to
act on logically invalid input states instead of properly handling the
I2C failure?

>  static int pca9532_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
>  {
>  	/* To use as input ensure pin is not driven */
> -	pca9532_gpio_set_value(gc, offset, 0);
> +	pca9532_gpio_set_value(gc, offset, 1);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but is there a use-after-free race condition
during device removal if an input event occurs after the workqueue is
cancelled?

In pca9532_destroy_devices() (called from pca9532_remove), the workqueue is
cancelled:

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

However, because data->idev is managed by devm_input_allocate_device(), it
remains fully registered until devres cleanup begins after pca9532_remove()
returns.

If an input event (like SND_BELL) is triggered in this window,
pca9532_event() will reschedule the work:

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

	return 0;
}

When devres subsequently unregisters the input device and frees the data
structure, won't the unexpectedly scheduled work later execute and
dereference the freed data pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703014201.69829-1-chou.cosmo@gmail.com?part=1