Re: [PATCH v3 4/8] HID: asus: cleanup keyboard listener on failure: avoid use-after-free

[Denis Benato]

On 6/13/26 17:57, Antheas Kapenekakis wrote:
> On Sat, 13 Jun 2026 at 17:30, Denis Benato <denis.benato@xxxxxxxxx> wrote:
>> asus_kbd_register_leds(), I noticed it registers a listener to a global list
>> and uses devm_kzalloc(). If a subsequent initialization step in asus_probe()
>> fails the driver returns without unregistering the listener, and the devres
>> subsystem will automatically free the memory, leaving a dangling pointer
>> in the global list.
>>
>> Fixes: b34b5945a769 ("HID: asus: listen to the asus-wmi brightness device instead of creating one")
>> Reported-by: sahiko-bot@xxxxxxxxxx
>> Signed-off-by: Denis Benato <denis.benato@xxxxxxxxx>
>> ---
>>  drivers/hid/hid-asus.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index 0a6c97155549..f38b18ad65c6 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
>> @@ -1426,11 +1426,17 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>                 if (drvdata->tp) {
>>                         ret = asus_start_multitouch(hdev);
>>                         if (ret)
>> -                               goto err_stop_hw;
>> +                               goto err_unregister_backlight;
>>                 }
> This block currently does two things, rename and init the touchpad.
> Instead,  you may pull the touchpad init to be below the hid_hw_start
> block and keep the same bail.
I'm not particularly keen on changing the sequence of packets in these
kinds of patches: I was trying to solve the reported issue in a way that
only solves the issue: nothing less and nothing more.
>>         }
>>
>>         return 0;
>> +err_unregister_backlight:
>> +       if (drvdata->kbd_backlight) {
>> +               asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
>> +               devm_kfree(&hdev->dev, drvdata->kbd_backlight);
>> +               drvdata->kbd_backlight = NULL;
>> +       }
>>  err_stop_hw:
> OR add asus_hid_unregister_listener(&drvdata->kbd_backlight->listener) here.
>
> Here, you'd also drop err_unregister_backlight, devm_kfree, and
> nulling. The driver is closing. It is not necessary.
>
> err_stop_hw is only used once, you do not need a secondary tag.
I will do this instead, thanks.
>>         hid_hw_stop(hdev);
>>         return ret;
>> --
>> 2.47.3
>>
>>