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

[Andrew Davis <afd@ti.com>]

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