[PATCH v2 0/4] leds: pca9532: fix unchecked I2C reads and unbind use-after-free

[PATCH v2 0/4] leds: pca9532: fix unchecked I2C reads and unbind use-after-free

[haibo.chen]

This series hardens the pca9532 LED driver against unchecked
i2c_smbus_read_byte_data() return values and fixes a use-after-free that
can happen on device unbind.

Patch 1 (the only patch in v1) checks the return value of
i2c_smbus_read_byte_data() in pca9532_getled(), fixing a Coverity
INTEGER_OVERFLOW report where an I2C error code was cast into a char and
used to compute a bogus LED state.

While reviewing v1, Sashiko AI review pointed out three additional
pre-existing issues in the same driver. Patches 2-4 address them:

- Patch 2: pca9532_setled() has the identical unchecked-read flaw, but it
  is worse: one register controls four LEDs, so writing the truncated
  error code back corrupts the state of the other three LEDs sharing the
  register. The return value is now checked and propagated to callers.

- Patch 3: pca9532_gpio_get_value() stores the read result in an unsigned
  char, so an I2C error is truncated and returned as a valid GPIO level,
  silently handing corrupt data to kernel/userspace consumers. The read
  is now stored in an int and the error propagated.

- Patch 4: on unbind, the input device allocated with
  devm_input_allocate_device() stays registered until devres cleanup runs
  after .remove(). pca9532_destroy_devices() only cancels the work, so an
  EV_SND event arriving after cancel_work_sync() can re-queue the work and
  dereference the freed driver data. The input device is now explicitly
  unregistered before the work is cancelled.

All patches build cleanly with CONFIG_LEDS_PCA9532 and
CONFIG_LEDS_PCA9532_GPIO enabled and pass checkpatch.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
Changes in v2:
- Patch 1: reworded the commit log and added an Assisted-by trailer.
  No functional change.
- New Patch 2: check pca9532_setled() I2C read return value (pre-existing
  issue reported by Sashiko AI review of v1). Cc: stable.
- New Patch 3: check pca9532_gpio_get_value() I2C read return value
  (pre-existing issue reported by Sashiko AI review of v1). Cc: stable.
- New Patch 4: fix use-after-free on unbind with the N2100 beeper
  (pre-existing issue reported by Sashiko AI review of v1). Cc: stable.
- Link to v1: https://lore.kernel.org/r/20260727-led-fix-v1-1-d635c5037109@nxp.com

---
Haibo Chen (4):
      leds: pca9532: check i2c_smbus_read_byte_data() return value
      leds: pca9532: check return value in pca9532_setled()
      leds: pca9532: check return value in pca9532_gpio_get_value()
      leds: pca9532: fix use-after-free on unbind with N2100 beeper

 drivers/leds/leds-pca9532.c | 47 ++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)
---
base-commit: c5e32e86ca02b003f86e095d379b38148999293d
change-id: 20260727-led-fix-28f085e12feb

Best regards,
-- 
Haibo Chen <haibo.chen@nxp.com>

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

[haibo.chen]

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.

Fixes: 28c5fe99016d ("leds: pca9532: Extend pca9532 device tree support")
Assisted-by: VeroCoder:claude-sonnet-4
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..e8ee17b8bd547a92372a8e74a6781b95ae831fe2 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;
 }

-- 
2.34.1

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

[sashiko-bot]

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

[PATCH v2 2/4] leds: pca9532: check return value in pca9532_setled()

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

pca9532_setled() reads the LED register with i2c_smbus_read_byte_data()
but never checks its return value. On an I2C read failure the function
returns a negative error code (e.g. -EIO) which was silently truncated
into a signed char, then bitwise modified and written back to the
device.

Since one LED register controls four LEDs, writing this bogus value back
corrupts the state of the other three LEDs sharing the same register.

Fixes: e14fa82439d3 ("leds: Add pca9532 led driver")
Cc: stable@vger.kernel.org
Assisted-by: VeroCoder:claude-sonnet-4
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/leds/leds-pca9532.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index e8ee17b8bd547a92372a8e74a6781b95ae831fe2..adfb5aa50e8e1f3ca0deb9261d8c9103903ef928 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -159,21 +159,29 @@ static int pca9532_setpwm(struct i2c_client *client, int pwm)
 }
 
 /* Set LED routing */
-static void pca9532_setled(struct pca9532_led *led)
+static int pca9532_setled(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;
+	int reg;
+	int ret = 0;
 
 	mutex_lock(&data->update_lock);
 	reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id));
+	if (reg < 0) {
+		dev_warn(&client->dev, "failed to read LED register: %d\n", reg);
+		ret = reg;
+		goto out;
+	}
 	/* 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);
+	ret = i2c_smbus_write_byte_data(client, LED_REG(maxleds, led->id), reg);
+out:
 	mutex_unlock(&data->update_lock);
+	return ret;
 }
 
 static int pca9532_set_brightness(struct led_classdev *led_cdev,
@@ -196,8 +204,7 @@ static int pca9532_set_brightness(struct led_classdev *led_cdev,
 	}
 	if (led->state == PCA9532_PWM0)
 		pca9532_setpwm(led->client, PCA9532_PWM_ID_0);
-	pca9532_setled(led);
-	return err;
+	return pca9532_setled(led);
 }
 
 static int pca9532_update_hw_blink(struct pca9532_led *led,
@@ -257,9 +264,7 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
 	if (err)
 		return err;
 
-	pca9532_setled(led);
-
-	return 0;
+	return pca9532_setled(led);
 }
 
 static int pca9532_event(struct input_dev *dev, unsigned int type,
@@ -334,9 +339,7 @@ static int pca9532_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
 	else
 		led->state = PCA9532_ON;
 
-	pca9532_setled(led);
-
-	return 0;
+	return pca9532_setled(led);
 }
 
 static int pca9532_gpio_get_value(struct gpio_chip *gc, unsigned offset)

-- 
2.34.1

Re: [PATCH v2 2/4] leds: pca9532: check return value in pca9532_setled()

[sashiko-bot]

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

New issues:
- [Medium] The newly added error return values from `pca9532_setled()` and `pca9532_gpio_set_value()` are ignored in the configuration and GPIO direction paths.
- [Low] The commit message violates the LED subsystem capitalization rule.

Pre-existing issues:
- [High] Incorrect teardown ordering of a devm-managed input device and a custom workqueue leads to a Use-After-Free during device removal.
--

--- Patch [2]: [PATCH v2 2/4] leds: pca9532: check return value in pca9532_setled() ---
commit 3e826891d5b6286d8b08ddc7c668e35465352554
Author: Haibo Chen <haibo.chen@nxp.com>

leds: pca9532: check return value in pca9532_setled()

This commit updates pca9532_setled() to check the return value of
i2c_smbus_read_byte_data(). By checking for read failures, it prevents
writing bogus bitwise-modified negative error codes back to the device.

> leds: pca9532: check return value in pca9532_setled()

[Severity: Low]
This isn't a bug, but the commit description after the subsystem prefix isn't 
capitalized. It should probably be "Check return value in pca9532_setled()".

> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index e8ee17b8bd547..adfb5aa50e8e1 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
[ ... ]
> @@ -159,21 +159,29 @@ static int pca9532_setpwm(struct i2c_client *client, int pwm)
>  }
>  
>  /* Set LED routing */
> -static void pca9532_setled(struct pca9532_led *led)
> +static int pca9532_setled(struct pca9532_led *led)

[Severity: Medium]
Since this function now returns an error code, do we also need to update
other callers that still ignore it? 

For example, pca9532_gpio_direction_input() calls pca9532_gpio_set_value()
(which returns the error from pca9532_setled()), but the error is discarded:

drivers/leds/leds-pca9532.c:pca9532_gpio_direction_input() {
    ...
    pca9532_gpio_set_value(gc, offset, 1);
    return 0;
}

Similarly, pca9532_configure() invokes pca9532_setled() during initialization
without checking the result, which might leave the hardware in an undefined
state if the I2C bus fails.

[ ... ]
> @@ -257,9 +264,7 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
>  	if (err)
>  		return err;
>  
> -	pca9532_setled(led);
> -
> -	return 0;
> +	return pca9532_setled(led);
>  }
>  
>  static int pca9532_event(struct input_dev *dev, unsigned int type,

[Severity: High]
This is a pre-existing issue, but can this event callback fire after the
workqueue is cancelled during device removal, causing a use-after-free?

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

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

However, because the input device is allocated with
devm_input_allocate_device(), it remains active and registered until
devres cleanup runs after .remove() finishes. 

If userspace injects an EV_SND event concurrently, the input core could
invoke pca9532_event(), rescheduling the work item via
schedule_work(&data->work). When the scheduled work executes later, the data
structure will have already been freed by devres.

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

[PATCH v2 3/4] leds: pca9532: check return value in pca9532_gpio_get_value()

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

pca9532_gpio_get_value() reads the input register with
i2c_smbus_read_byte_data() but stores the result in an unsigned char and
never checks for failure. On an I2C read error the negative error code
(e.g. -EIO) is truncated to an unsigned byte, and a bit is extracted and
returned as a valid GPIO level. This masks hardware errors and lets
kernel or userspace consumers silently receive corrupt data.

The gpio_chip .get callback may return a negative error code, so store
the read result in an int and propagate the error on failure.

Fixes: 3c1ab50d0a31 ("drivers/leds/leds-pca9532.c: add gpio capability")
Cc: stable@vger.kernel.org
Assisted-by: VeroCoder:claude-sonnet-4
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/leds/leds-pca9532.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index adfb5aa50e8e1f3ca0deb9261d8c9103903ef928..cee398d8275b8953b5dbdb741c36161f34bba029 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -345,9 +345,14 @@ static int pca9532_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
 static int pca9532_gpio_get_value(struct gpio_chip *gc, unsigned offset)
 {
 	struct pca9532_data *data = gpiochip_get_data(gc);
-	unsigned char reg;
+	int reg;
 
 	reg = i2c_smbus_read_byte_data(data->client, PCA9532_REG_INPUT(offset));
+	if (reg < 0) {
+		dev_warn(&data->client->dev,
+			 "failed to read input register: %d\n", reg);
+		return reg;
+	}
 
 	return !!(reg & (1 << (offset % 8)));
 }

-- 
2.34.1

Re: [PATCH v2 3/4] leds: pca9532: check return value in pca9532_gpio_get_value()

[sashiko-bot]

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

New issues:
- [Medium] Unratelimited warning log in the GPIO read callback can flood the kernel log.
- [Low] The commit message description after the subsystem prefix is not capitalized.

Pre-existing issues:
- [High] Race condition in driver teardown leaves input workqueue active after device data is freed, causing a Use-After-Free.
--

--- Patch [3]: [PATCH v2 3/4] leds: pca9532: check return value in pca9532_gpio_get_value() ---
commit 40e01af05e0100f229b670bf1dcf0b63a92a90f8
Author: Haibo Chen <haibo.chen@nxp.com>

leds: pca9532: check return value in pca9532_gpio_get_value()

pca9532_gpio_get_value() reads the input register with
i2c_smbus_read_byte_data() but stores the result in an unsigned char and
never checks for failure. On an I2C read error the negative error code
(e.g. -EIO) is truncated to an unsigned byte, and a bit is extracted and
returned as a valid GPIO level. This masks hardware errors and lets
kernel or userspace consumers silently receive corrupt data.

> leds: pca9532: check return value in pca9532_gpio_get_value()

[Severity: Low]
This isn't a bug, but the subsystem guidelines request capitalizing the
description after the prefix. Should "check" be capitalized to "Check"?

> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index adfb5aa50e8e1..cee398d8275b8 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -345,9 +345,14 @@ static int pca9532_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
>  static int pca9532_gpio_get_value(struct gpio_chip *gc, unsigned offset)
>  {
>  	struct pca9532_data *data = gpiochip_get_data(gc);
> -	unsigned char reg;
> +	int reg;
>  
>  	reg = i2c_smbus_read_byte_data(data->client, PCA9532_REG_INPUT(offset));
> +	if (reg < 0) {
> +		dev_warn(&data->client->dev,
> +			 "failed to read input register: %d\n", reg);

[Severity: Medium]
Could this dev_warn() flood the kernel log if the I2C bus fails while
userspace is actively polling the GPIO value? Would it be safer to use
dev_warn_ratelimited() here?

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

[Severity: High]
This is a pre-existing issue, but does driver teardown leave the input
workqueue active after device data is freed, causing a use-after-free?

Looking at pca9532_destroy_devices(), cancel_work_sync() is called but
the input device remains registered because it relies on devres for
unregistration:

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

If an event comes in after pca9532_remove() finishes but before devres
cleans up the input device, could pca9532_event() blindly reschedule
the work?

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

Since devres unregisters the device and frees data, the scheduled work
could be left dangling. When pca9532_input_work() finally executes,
would it dereference the freed data pointer?

drivers/leds/leds-pca9532.c:pca9532_input_work() {
    ...
    mutex_lock(&data->update_lock);
    ...
}

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

[PATCH v2 4/4] leds: pca9532: fix use-after-free on unbind with N2100 beeper

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

The input device for the N2100 beeper is allocated with
devm_input_allocate_device(), so it stays registered until devres
cleanup runs after .remove() returns. pca9532_destroy_devices() only
calls cancel_work_sync() and clears data->idev, but leaves the input
device registered.

In the window between cancel_work_sync() returning and devres
unregistering the input device, a userspace EV_SND event can reach
pca9532_event(), which calls schedule_work() and re-queues data->work
after it was cancelled. Once devres frees the devm-allocated driver
data, the work callback dereferences freed memory, resulting in a
use-after-free.

Explicitly unregister the input device before cancelling the work so
that no new events can be delivered after the work has been cancelled.

Fixes: 8614fb46637a ("leds: pca9532: Convert to devm_input_allocate_device()")
Cc: stable@vger.kernel.org
Assisted-by: VeroCoder:claude-sonnet-4
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/leds/leds-pca9532.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index cee398d8275b8953b5dbdb741c36161f34bba029..eeb91b7466419325191200f6cd9a4dc81f8ee20c 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);
 				cancel_work_sync(&data->work);
 				data->idev = NULL;
 			}

-- 
2.34.1

Re: [PATCH v2 4/4] leds: pca9532: fix use-after-free on unbind with N2100 beeper

[sashiko-bot]

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 <haibo.chen@nxp.com>
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/20260729-led-fix-v2-0-09ad218457bd@nxp.com?part=4