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

[Andrew Davis <afd@ti.com>]

On 9/26/26 12:21 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:
> 

Seems odd to not be able to cancel the work before unregistering,
this driver is a little odd that it uses a work queue for updating
the registers in the first place though.

Anyway, we can sort that all out later, this patch looks correct
for now,

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

>    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
> 
> Set up the work with devm_work_autocancel() before the LEDs are
> registered, so that devres cancels it after they are gone, and drop
> tca6507_remove().
> 
> Fixes: 1b5c2fa7081c ("leds: tca6507: Use devm_led_classdev_register() to simplify remove path")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
> ---
> 
> Notes:
>      Tested in QEMU on a next-20260925 KASAN kernel, with the driver built as
>      a module and a test-only module that adds a fake I2C adapter and a
>      software node for a TCA6507 with two LEDs. With the LEDs on, unbinding
>      hits the report above (38 reports in 20 unbind/bind cycles), and so does
>      unloading the driver in 2 of 3 runs. With this patch there are no
>      reports in 20 cycles or 3 unloads, and the LEDs come back after each
>      rebind. A W=1 build with GPIOLIB is clean.
> 
>   drivers/leds/leds-tca6507.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
> index 9afe2722986c..503a465a07f4 100644
> --- a/drivers/leds/leds-tca6507.c
> +++ b/drivers/leds/leds-tca6507.c
> @@ -79,6 +79,7 @@
>   #include <linux/gpio/driver.h>
>   #include <linux/property.h>
>   #include <linux/workqueue.h>
> +#include <linux/devm-helpers.h>
>   
>   /* LED select registers determine the source that drives LED outputs */
>   #define TCA6507_LS_LED_OFF	0x0	/* Output HI-Z (off) */
> @@ -735,9 +736,15 @@ 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);
> +
> +	/*
> +	 * Unregistering the LEDs turns them off, which queues the work again,
> +	 * so it must be cancelled after them: set this up before registering.
> +	 */
> +	err = devm_work_autocancel(dev, &tca->work, tca6507_work);
> +	if (err)
> +		return err;
>   
>   	for (i = 0; i < NUM_LEDS; i++) {
>   		struct tca6507_led *l = tca->leds + i;
> @@ -766,20 +773,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: 05b4738b0078f7d6f154f68068a11c8a0635e9df