Re: [PATCH v3 1/2] leds: tca6507: Fix use-after-free on unbind

[Andrew Davis <afd@ti.com>]

On 11/4/26 12:22 PM, Lee Jones wrote:
> On Wed, 28 Oct 2026, 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
>>
>> 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
>> Acked-by: Andrew Davis <afd@ti.com>
>> Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
>> ---
>>   drivers/leds/leds-tca6507.c | 15 +++++----------
>>   1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
>> index 9afe2722986c..0f987e551f03 100644
>> --- a/drivers/leds/leds-tca6507.c
>> +++ b/drivers/leds/leds-tca6507.c
>> @@ -71,6 +71,7 @@
>>    * not recognised when matching.
>>    */
>>
>> +#include <linux/devm-helpers.h>
>>   #include <linux/module.h>
>>   #include <linux/slab.h>
>>   #include <linux/leds.h>
>> @@ -735,9 +736,11 @@ 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);
>> +
>> +	err = devm_work_autocancel(dev, &tca->work, tca6507_work);
>> +	if (err)
>> +		return err;
> 
> This is replaced in patch 2.  I'm not sure I see the point.
> 
> Why not do everything at once?
> 

I see a v4 combining these just got posted before I could make
a reply here, but going to reply here anyway.

The two patches do different logical things, the first restores
the order of unregistering the LED before cancelling the work,
which fixes 1b5c2fa7081c. The second is a change in behavior, as
now we flush the work, not just cancel it as had previously always
been the case.

Both changes are correct, so combining is probably fine, but it
does slightly obscure the cancel -> flush behavior change part.

Andrew

>>   	for (i = 0; i < NUM_LEDS; i++) {
>>   		struct tca6507_led *l = tca->leds + i;
>> @@ -766,20 +769,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,
>>   };
>>
>> -- 
>> 2.56.0
>>
>