Re: [PATCH 02/12] HID: asus: add support for ROG Ally handhelds

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Acquiring a sleeping lock (mutex) inside an atomic context (URB completion handler), causing a 'BUG: scheduling while atomic' kernel panic.
- [High] Missing rollback of global state pointers on probe failure leaves dangling pointers in a static global struct.
- [Medium] Hardware packet structures are defined using native types (`uint16_t`), breaking portability on Big Endian architectures.
- [Medium] Device fails to resume if the ROG Ally extensions failed to probe.
- [Medium] Missing bounds check before accessing `data[1]` in `ally_x_raw_event` allows reading stale buffer data.
--

--- Patch [2]: [PATCH 02/12] HID: asus: add support for ROG Ally handhelds ---
commit 5a6720dc4f3259dca06d1dbb804d6380655462ce
Author: Denis Benato <[email protected]>

HID: asus: add support for ROG Ally handhelds

This commit implements the core functionality for ASUS handhelds, extending
the hid-asus driver to initialize the device, implement Ally X input mapping,
and handle specific AC button events.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 897b04d06b464..f4b2c4a616b75 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -232,12 +271,655 @@ static const struct asus_touchpad_info medion_e1239t_tp = {
[ ... ]
> +struct ally_x_input_report {
> +	uint16_t x, y;
> +	uint16_t rx, ry;
> +	uint16_t z, rz;
> +	uint8_t buttons[3];
> +} __packed;

[Severity: Medium]
Are these values interpreted correctly on big-endian architectures?

Since USB HID payloads natively use little-endian byte ordering, defining
these as native uint16_t and using them without le16_to_cpu() conversion
can corrupt the analog stick values on big-endian systems.

[ ... ]
> +static bool ally_x_raw_event(struct input_dev *input, struct hid_device *hdev,
> +			    struct hid_report *report, u8 *data, int size)
> +{
> +	struct ally_x_input_report *in_report;
> +	u8 byte;
> +
> +	if (!input)
> +		return false;
> +
> +	if (size < 1)
> +		return false;
> +
> +	if (data[0] == 0x5A) {
> +		input_report_key(input, KEY_PROG1, data[1] == 0x38);

[Severity: Medium]
Is it possible for data[1] to read stale memory here?

Since the size check above only ensures the size is at least 1, if the device
sends a 1-byte report starting with 0x5A, accessing data[1] will read
out-of-bounds from the previous transfer buffer contents.

[ ... ]
> +static bool hid_asus_ally_raw_event(struct hid_device *hdev, struct ally_handheld *ally,
> +			struct hid_report *report, u8 *data, int size)
> +{
> +	struct input_dev *x_input;
> +	struct hid_device *x_hdev;
> +
> +	if (!ally)
> +		return false;
> +
> +	switch (ally_get_endpoint_address(hdev)) {
> +	case HID_ALLY_X_INTF_IN:
> +		scoped_guard(mutex, &ally_data_mutex) {
> +			x_input = ally->ally_x_input;
> +			x_hdev = ally->ally_x_hdev;
> +		}

[Severity: Critical]
Can this cause a kernel panic due to scheduling while atomic?

The hid_asus_ally_raw_event() function is invoked from the driver's
raw_event callback (asus_raw_event). For USB HID devices, the core HID
subsystem invokes raw_event directly from the URB completion handler which
executes in atomic interrupt context.

Acquiring a sleeping lock like a mutex in atomic context violates kernel
scheduling rules and will trigger an immediate panic.

[ ... ]
> @@ -1399,21 +2101,28 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
[ ... ]
> +	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> +		ret = hid_asus_ally_reset_resume(hdev, drvdata->rog_ally);
> +		if (ret) {
> +			hid_err(hdev, "Failed to resume ROG Ally HID extensions: %d\n", ret);
> +			return ret;
> +		}
> +	}

[Severity: Medium]
Could this break system resume if the optional extension failed to probe?

If hid_asus_ally_probe() fails during device probe, drvdata->rog_ally is left
as NULL, but the device continues to operate as a generic HID device.

During system resume, hid_asus_ally_reset_resume() will return -EINVAL when
ally is NULL, which gets propagated here and causes the entire device resume
to fail.

[ ... ]
> @@ -1530,6 +2239,15 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
[ ... ]
> +	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> +		ally = hid_asus_ally_probe(hdev);
> +		if (IS_ERR(ally))
> +			hid_err(hdev, "Failed to initialize ROG Ally HID extensions: %ld\n",
> +				PTR_ERR(ally));
> +		else
> +			drvdata->rog_ally = ally;
> +	}

[Severity: High]
Might this lead to a use-after-free if probe fails later in this function?

The hid_asus_ally_probe() function populates the statically-allocated global
ally_drvdata struct with pointers to the current device's hdev and
devres-managed input_dev.

If a subsequent step in asus_probe() fails (such as
asus_initialize_reports()), the function jumps to the error path without
calling hid_asus_ally_remove(). The devres cleanup will free the input_dev,
but the global ally_drvdata will retain the dangling pointer. If another
interface successfully probes and triggers ally_resume_work_fn(), it will
dereference the freed pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2