linux-kernel - Re: [PATCH] HID: asus: Add check for cancelling fn_lock_sync

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aYHlEwK19HzADcOv@chandna.localdomain>
Date: Tue, 3 Feb 2026 17:37:47 +0530
From: Sahil Chandna <chandna.sahil@...il.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Jiri Kosina <jikos@...nel.org>, bentiss@...nel.org,
	connorbelli2003@...il.com, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: asus: Add check for cancelling fn_lock_sync_work

On Mon, Feb 02, 2026 at 10:27:33PM -0500, Alan Stern wrote:
>I just noticed this because of a related message in a different thread.
>
>On Fri, Jan 26, 2026 at 09:22:04PM +0530, Sahil Chandna wrote:
>
>> syzbot reported a workqueue warning where fn_lock_sync_work is cancelled
>> during device removal before the work has been initialized. This can
>> happen when the device is disconnected while initialization is still
>> in progress.
>> Fix this by initializing fn_lock_sync_work before marking fn_lock as
>> enabled, and by using fn_lock as a flag in the remove path. This
>> ensures cancel_work_sync() is only called for initialized work.
>>
>> Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
>> Reported-by: syzbot+13f8286fa2de04a7cd48@...kaller.appspotmail.com
>> Signed-off-by: Sahil Chandna <chandna.sahil@...il.com>
>> ---
>>  drivers/hid/hid-asus.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index 1b9793f7c07e..bb85a36de14f 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
>> @@ -960,8 +960,8 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>  	}
>>
>>  	if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>> -		drvdata->fn_lock = true;
>>  		INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
>> +		drvdata->fn_lock = true;
>>  		asus_kbd_set_fn_lock(hdev, true);
>>  	}
>>
>> @@ -1343,7 +1343,7 @@ static void asus_remove(struct hid_device *hdev)
>>  		cancel_work_sync(&drvdata->kbd_backlight->work);
>>  	}
>>
>> -	if (drvdata->quirks & QUIRK_HID_FN_LOCK)
>> +	if ((drvdata->quirks & QUIRK_HID_FN_LOCK) && drvdata->fn_lock)
>>  		cancel_work_sync(&drvdata->fn_lock_sync_work);
>>
>>  	hid_hw_stop(hdev);
>
>With no synchronization between the two routines, this patch cannot
>possibly be correct.  There's nothing to prevent the CPU running
>asus_input_configured() from executing the assignment to
>drvdata->fn_lock before doing the INIT_WORK() (unless the INIT_WORK()
>call itself contains some synchronization -- but obviously the code
>shouldn't depend on that).
>
>At the very least there should be a pair of memory barriers.  A more
>palatable substitute would be to protect both regions of code with a
>mutex.
>
>Alan Stern
Thanks, I will test with mutex between INIT_WORK and cancel_work
and share v2.
Regards,
Sahil