Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sleeping in atomic context due to implicit GFP_KERNEL allocations in HID event handlers.
- [High] Data race on shared `hid_report` object when calling `hid_report_raw_event()` asynchronously from the workqueue.
- [High] Use-After-Free due to un-canceled workqueue items if `hid_hw_start()` fails during probe.
--

commit 29efc59c7a1d251ec0726f58d50488da03f9ba12
Author: Denis Benato <denis.benato@linux.dev>

    HID: asus: refactor the two workqueues and init sequence
    
    Join the two workqueues into one reusing the stopping mechanism
    of the brightness workqueue, use the joined workqueue to also
    move the asus_wmi_send_event() sleeping call away from atomic
    context and add a size check in asus_raw_event().

    Link: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
[ ... ]
> +static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
> +{
> +	struct asus_work_action *action;
> +	unsigned long flags;
> +
> +	action = kzalloc_obj(struct asus_work_action);

[Severity: High]
Does this allocation sleep in an atomic context?

Because kzalloc_obj() implicitly uses default_gfp() which resolves to
GFP_KERNEL, this introduces a blocking allocation. Since
asus_kbd_fn_lock_set() is called directly from the raw HID event handler
asus_event() which runs in an atomic context, this might trigger kernel
panics or deadlocks under load.

> +	if (!action)
> +		return -ENOMEM;
[ ... ]
> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
> +				 size_t report_size)
> +{
> +	struct asus_work_action *action;
> +	unsigned long flags;
> +
> +	if (report_size > FEATURE_KBD_REPORT_SIZE) {
> +		hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
> +		return -EINVAL;
>  	}
>  
> +	action = kzalloc_obj(struct asus_work_action);

[Severity: High]
Similarly, does this kzalloc_obj() introduce a blocking allocation in an
atomic context?

asus_kbd_wmi_fan_send() is invoked from the asus_raw_event() handler, which
also executes in an atomic context.

> +	if (!action)
> +		return -ENOMEM;
[ ... ]
> @@ -569,59 +668,151 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
[ ... ]
> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int ret;
> +
> +	ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
[ ... ]
> +	/* Fallback: pass the raw event to the HID core */
> +	hid_report_raw_event(hdev, HID_INPUT_REPORT,
> +			     data->report_data, data->report_size,
> +			     data->report_size, 1);

[Severity: High]
Can this data race with native HID IRQ delivery?

If notifying the WMI system of a fan key event fails, the deferred work
handler asus_kbd_wmi_fan() falls back to injecting the raw event into the
HID core using hid_report_raw_event().

However, hid_report_raw_event() parses the report and mutates the shared
report->field state arrays without taking any locks. Because the workqueue
bypasses the hid->driver_input_lock semaphore used by the native IRQ path,
a concurrent hardware interrupt targeting the same report ID could race with
this workqueue. This might lead to unsynchronized reads and writes to
field->value and field->new_value, potentially corrupting the core HID
report state.

> +}
[ ... ]
> @@ -1294,6 +1465,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
[ ... ]
> +	ret = asus_worker_create(hdev, drvdata);
> +	if (ret) {
> +		hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  	if (ret) {
>  		hid_err(hdev, "Asus hw start failed: %d\n", ret);
>  		return ret;
>  	}

[Severity: High]
Could this leak a pending work item and cause a use-after-free if
hid_hw_start() fails?

During driver probe, asus_worker_create() creates a devres-managed worker
struct. Next, hid_hw_start() connects the device, invoking
asus_input_configured(), which synchronously schedules a FN_LOCK_SYNC
work item on the global workqueue.

If hid_hw_start() subsequently fails (e.g., due to an input registration
error later in the sequence), asus_probe() captures the error and returns
it directly, bypassing the err_stop_hw label. Consequently, it fails to
call asus_worker_stop() or cancel_work_sync().

The devres framework will automatically free the worker memory, but the
scheduled work item remains active. When the system workqueue eventually
executes the pending work, it will dereference the freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615165058.3845-1-denis.benato@linux.dev?part=1