Re: [PATCH v2 4/7] HID: asus: Cycle the platform profile from the Zenbook Duo profile key

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: asus: ABBA deadlock between driver_input_lock and worker cancellation in remove()
- [High] hid: asus: unbounded GFP_ATOMIC allocation in profile cycle event
--

commit f048215156fec2e6351754bc8e9e549f5a266615
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>
Subject: HID: asus: Cycle the platform profile from the Zenbook Duo profile key

This commit adds support for the Zenbook Duo performance mode key by routing
the 0x9d vendor usage event to cycle the platform profile. The processing is
deferred to the driver's workqueue to avoid taking mutexes in the HID input
path.

> 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
> @@ -423,6 +425,28 @@ static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
>  	return 0;
>  }
>  
> +static int asus_platform_profile_cycle(struct asus_drvdata *drvdata)
> +{
> +	struct asus_work_action *action;
> +	unsigned long flags;
> +
> +	if (!IS_REACHABLE(CONFIG_ACPI_PLATFORM_PROFILE))
> +		return 0;
> +
> +	action = kzalloc_obj(struct asus_work_action, GFP_ATOMIC);

[Severity: High]
Can a flood of 0x9d HID events cause memory exhaustion?

For each incoming event, asus_event() calls asus_platform_profile_cycle(),
which performs a GFP_ATOMIC allocation and appends it to the worker queue
without any length limit.

If a device sends these events continuously at a high rate, and the workqueue
thread stalls on the profile_lock mutex in platform_profile_cycle(), could
this interrupt-driven URB completion handler exhaust atomic memory reserves
before the workqueue can process them?

> +	if (!action)
> +		return -ENOMEM;
> +
> +	action->type = PLATFORM_PROFILE_CYCLE;
> +	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;
> +}

[ ... ]

> @@ -788,6 +830,13 @@ static void asus_work(struct work_struct *work)
>  	case WMI_FAN:
>  		asus_kbd_wmi_fan(worker->hdev, &action->data.fan_hid_data);
>  		break;
> +	case PLATFORM_PROFILE_CYCLE:
> +		if (!IS_REACHABLE(CONFIG_ACPI_PLATFORM_PROFILE))
> +			break;
> +		ret = platform_profile_cycle();

[Severity: High]
This problem wasn't introduced by this patch, but does relying on this worker
architecture expose an ABBA deadlock during device removal?

If a user presses the fan control key, the WMI_FAN action is queued. If the
WMI driver is unavailable, asus_kbd_wmi_fan() acquires driver_input_lock:

asus_kbd_wmi_fan() {
        ...
        down(&hdev->driver_input_lock);
        hid_report_raw_event(...);
        up(&hdev->driver_input_lock);
}

However, if the device is unplugged or unbound simultaneously, the HID core
acquires driver_input_lock in hid_device_remove() before calling asus_remove().
asus_remove() then waits for the worker to finish:

asus_remove() {
        ...
        asus_worker_stop(drvdata->worker);
            -> cancel_work_sync(&worker->work);
        hid_hw_stop(hdev);
}

This creates a situation where the worker thread blocks indefinitely on
driver_input_lock, while the remove thread blocks indefinitely waiting for the
worker via cancel_work_sync().

Since this patch extends the same worker thread design, could this lead to a
system hang if device removal races with the workqueue?

> +		if (ret)
> +			hid_dbg(worker->hdev, "Failed to cycle the platform profile: %d\n", ret);
> +		break;
>  	default:
>  		hid_err(worker->hdev, "Invalid action type: %d\n", action->type);
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009153459.124752-1-p.pisati@gmail.com?part=4