[PATCH v4] leds: tca6507: Fix use-after-free on unbind

[PATCH v4] leds: tca6507: Fix use-after-free on unbind

[Danish Khateeb]

Since the LEDs are registered with devm_led_classdev_register(), they
are unregistered only after tca6507_remove() has cancelled the work.
Unregistering an LED that is on turns it off, which queues the work
again from led_assign(), and the work then runs after devres has freed
the chip:

  BUG: KASAN: slab-use-after-free in assign_work+0x414/0x5b0
  Read of size 8 at addr ffff888010d5c0c0 by task kworker/1:2/65
  ...
  Last potentially related work creation:
   ...
   queue_work_on+0xb6/0xc0
   led_assign+0x223/0x340 [leds_tca6507]
   led_classdev_unregister+0x26b/0x340
   release_nodes+0xb3/0x140
   devres_release_group+0x247/0x470
   i2c_device_remove+0xbf/0x1b0

Initialise the work before the LEDs are registered and flush it from a
devres action, which then runs after the LEDs are gone and before the
chip is freed, and drop tca6507_remove(). Flushing instead of cancelling
also makes sure that the queued writes reach the chip, so the LEDs are
really turned off. By the time the action runs, the LEDs and the GPIO
chip have been unregistered, and the work does not queue itself, so
nothing can queue it again.

Fixes: 1b5c2fa7081c ("leds: tca6507: Use devm_led_classdev_register() to simplify remove path")
Cc: stable@vger.kernel.org
Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
---

Notes:
    Changes in v4:
    - Squash the two v3 patches into one, flushing the work from the start (Lee)
    - Drop Andrew's Acked-by, since the code is now different from the
      version he acked
    - Rebase on for-leds-next
    - Link to v3: https://lore.kernel.org/all/20261028174348.64014-1-danishkhateeb03@gmail.com/
    
    Tested in QEMU on a next-20260930 KASAN kernel, with the driver built as
    a module and a test-only module that adds a fake I2C adapter, logs the
    register writes and adds a software node for a TCA6507 with two LEDs.
    Without this patch, unbinding with the LEDs on hits the report above
    (39 reports in 20 unbind/bind cycles). With it there are no reports in
    20 cycles or on unload, and every unbind ends with a write that turns
    both LEDs off (with v2's cancel, 7 of 20 unbinds left an LED on). A W=1
    build with GPIOLIB is clean. The code is the same as v3 with both
    patches applied.
    
    An LLM was used to help with this patch.

 drivers/leds/leds-tca6507.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
index 9afe2722986c..ffad476af625 100644
--- a/drivers/leds/leds-tca6507.c
+++ b/drivers/leds/leds-tca6507.c
@@ -711,6 +711,11 @@ static const struct of_device_id __maybe_unused of_tca6507_leds_match[] = {
 };
 MODULE_DEVICE_TABLE(of, of_tca6507_leds_match);
 
+static void tca6507_flush_work(void *work)
+{
+	flush_work(work);
+}
+
 static int tca6507_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -735,9 +740,12 @@ static int tca6507_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	tca->client = client;
-	INIT_WORK(&tca->work, tca6507_work);
 	spin_lock_init(&tca->lock);
-	i2c_set_clientdata(client, tca);
+
+	INIT_WORK(&tca->work, tca6507_work);
+	err = devm_add_action_or_reset(dev, tca6507_flush_work, &tca->work);
+	if (err)
+		return err;
 
 	for (i = 0; i < NUM_LEDS; i++) {
 		struct tca6507_led *l = tca->leds + i;
@@ -766,20 +774,12 @@ static int tca6507_probe(struct i2c_client *client)
 	return 0;
 }
 
-static void tca6507_remove(struct i2c_client *client)
-{
-	struct tca6507_chip *tca = i2c_get_clientdata(client);
-
-	cancel_work_sync(&tca->work);
-}
-
 static struct i2c_driver tca6507_driver = {
 	.driver   = {
 		.name    = "leds-tca6507",
 		.of_match_table = of_match_ptr(of_tca6507_leds_match),
 	},
 	.probe    = tca6507_probe,
-	.remove   = tca6507_remove,
 	.id_table = tca6507_id,
 };
 

base-commit: fb511964723311e9a7223e424d725c58dde8307a
-- 
2.56.0

Re: [PATCH v4] leds: tca6507: Fix use-after-free on unbind

[Andrew Davis]

On 11/4/26 1:30 PM, Danish Khateeb wrote:
> Since the LEDs are registered with devm_led_classdev_register(), they
> are unregistered only after tca6507_remove() has cancelled the work.
> Unregistering an LED that is on turns it off, which queues the work
> again from led_assign(), and the work then runs after devres has freed
> the chip:
> 
>    BUG: KASAN: slab-use-after-free in assign_work+0x414/0x5b0
>    Read of size 8 at addr ffff888010d5c0c0 by task kworker/1:2/65
>    ...
>    Last potentially related work creation:
>     ...
>     queue_work_on+0xb6/0xc0
>     led_assign+0x223/0x340 [leds_tca6507]
>     led_classdev_unregister+0x26b/0x340
>     release_nodes+0xb3/0x140
>     devres_release_group+0x247/0x470
>     i2c_device_remove+0xbf/0x1b0
> 
> Initialise the work before the LEDs are registered and flush it from a
> devres action, which then runs after the LEDs are gone and before the
> chip is freed, and drop tca6507_remove(). Flushing instead of cancelling
> also makes sure that the queued writes reach the chip, so the LEDs are
> really turned off. By the time the action runs, the LEDs and the GPIO
> chip have been unregistered, and the work does not queue itself, so
> nothing can queue it again.
> 
> Fixes: 1b5c2fa7081c ("leds: tca6507: Use devm_led_classdev_register() to simplify remove path")
> Cc: stable@vger.kernel.org
> Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
> ---
> 
> Notes:
>      Changes in v4:
>      - Squash the two v3 patches into one, flushing the work from the start (Lee)
>      - Drop Andrew's Acked-by, since the code is now different from the
>        version he acked

Acked-by: Andrew Davis <afd@ti.com>

>      - Rebase on for-leds-next
>      - Link to v3: https://lore.kernel.org/all/20261028174348.64014-1-danishkhateeb03@gmail.com/
>      
>      Tested in QEMU on a next-20260930 KASAN kernel, with the driver built as
>      a module and a test-only module that adds a fake I2C adapter, logs the
>      register writes and adds a software node for a TCA6507 with two LEDs.
>      Without this patch, unbinding with the LEDs on hits the report above
>      (39 reports in 20 unbind/bind cycles). With it there are no reports in
>      20 cycles or on unload, and every unbind ends with a write that turns
>      both LEDs off (with v2's cancel, 7 of 20 unbinds left an LED on). A W=1
>      build with GPIOLIB is clean. The code is the same as v3 with both
>      patches applied.
>      
>      An LLM was used to help with this patch.
> 
>   drivers/leds/leds-tca6507.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
> index 9afe2722986c..ffad476af625 100644
> --- a/drivers/leds/leds-tca6507.c
> +++ b/drivers/leds/leds-tca6507.c
> @@ -711,6 +711,11 @@ static const struct of_device_id __maybe_unused of_tca6507_leds_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, of_tca6507_leds_match);
>   
> +static void tca6507_flush_work(void *work)
> +{
> +	flush_work(work);
> +}
> +
>   static int tca6507_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
> @@ -735,9 +740,12 @@ static int tca6507_probe(struct i2c_client *client)
>   		return -ENOMEM;
>   
>   	tca->client = client;
> -	INIT_WORK(&tca->work, tca6507_work);
>   	spin_lock_init(&tca->lock);
> -	i2c_set_clientdata(client, tca);
> +
> +	INIT_WORK(&tca->work, tca6507_work);
> +	err = devm_add_action_or_reset(dev, tca6507_flush_work, &tca->work);
> +	if (err)
> +		return err;
>   
>   	for (i = 0; i < NUM_LEDS; i++) {
>   		struct tca6507_led *l = tca->leds + i;
> @@ -766,20 +774,12 @@ static int tca6507_probe(struct i2c_client *client)
>   	return 0;
>   }
>   
> -static void tca6507_remove(struct i2c_client *client)
> -{
> -	struct tca6507_chip *tca = i2c_get_clientdata(client);
> -
> -	cancel_work_sync(&tca->work);
> -}
> -
>   static struct i2c_driver tca6507_driver = {
>   	.driver   = {
>   		.name    = "leds-tca6507",
>   		.of_match_table = of_match_ptr(of_tca6507_leds_match),
>   	},
>   	.probe    = tca6507_probe,
> -	.remove   = tca6507_remove,
>   	.id_table = tca6507_id,
>   };
>   
> 
> base-commit: fb511964723311e9a7223e424d725c58dde8307a