Re: [PATCH v5 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] ABBA deadlock between driver removal/probe failure and the deferred WMI_FAN action in the workqueue due to inverted lock acquisition order of `hdev->driver_input_lock`.
- [High] Unbounded dynamic allocation of workqueue actions allows an untrusted USB device or unprivileged user to exhaust kernel atomic memory pools.
- [Medium] Passing the raw packet size as `bufsize` to `hid_report_raw_event()` bypasses the HID core's automatic padding, leading to valid short packets being incorrectly rejected.
--

commit 656f5a6cc923dccccd418cd0aefa2239f221bf47
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: refactor the two workqueues and init sequence
    
Multiple issues have been found within the hid-asus driver. This patch
joins the two workqueues into one and reuses the stopping mechanism
of the brightness workqueue. It also moves the asus_wmi_send_event()
sleeping call away from atomic context.

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
> index d34d74df3dc0b..40191eb9e2e86 100644
> --- 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_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(sizeof(struct asus_work_action), GFP_NOWAIT);

[Severity: High]
Does this code allow an unbounded number of allocations?

If a device sends a flood of fan events, or if a local user rapidly changes
the brightness, it seems this could queue an unlimited number of actions
using GFP_NOWAIT or GFP_ATOMIC.

Could this exhaust kernel atomic memory pools and lead to a local denial of
service?

> +	if (!action)
> +		return -ENOMEM;
> +
> +	action->type = WMI_FAN;
> +	action->data.fan_hid_data.report_size = report_size;
> +	memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
> +	INIT_LIST_HEAD(&action->node);
> +
> +	spin_lock_irqsave(&drvdata->worker->lock, flags);
> +	asus_worker_schedule(drvdata->worker, action);
> +	spin_unlock_irqrestore(&drvdata->worker->lock, flags);
>  
>  	return 0;
>  }

[ ... ]

> @@ -569,59 +668,157 @@ 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)
>  {

[ ... ]

> +	/*
> +	 * Fallback: pass the raw event to the HID core; to avoid
> +	 * racing against the hid_report_raw_event() that generated
> +	 * this event use the same locking mechanism and wait for
> +	 * that function to terminate and signal the deferred execution
> +	 * before raising the stored event.
> +	 */
> +	down(&hdev->driver_input_lock);

[Severity: High]
Can this code cause an ABBA deadlock during device probe failure or removal?

The HID core holds hdev->driver_input_lock during hid_device_probe() and
hid_device_remove().

If asus_remove() or the error path in asus_probe() calls asus_worker_stop()
and blocks on cancel_work_sync(&worker->work) while driver_input_lock is
held:

HID core thread:
hid_device_remove() (acquires driver_input_lock)
  -> asus_remove()
    -> asus_worker_stop()
      -> cancel_work_sync() (waits for worker)

Worker thread:
asus_work()
  -> asus_kbd_wmi_fan()
    -> down(&hdev->driver_input_lock) (waits for HID core thread)

Would this lead to an unrecoverable deadlock?

> +	hid_report_raw_event(hdev, HID_INPUT_REPORT,
> +			     data->report_data, data->report_size,
> +			     data->report_size, 1);

[Severity: Medium]
Does passing data->report_size as the bufsize argument drop valid short
packets?

The HID core natively handles short packets by padding the buffer up to the
expected report descriptor size (rsize), provided the allocated buffer size
(bufsize) is large enough.

If bufsize is artificially restricted to data->report_size, the safety check
"if (bsize < rsize)" inside hid_report_raw_event() might falsely trigger and
reject the valid short event with -EINVAL.

Should FEATURE_KBD_REPORT_SIZE be used as the bufsize parameter instead?

> +	up(&hdev->driver_input_lock);
> +}

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