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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] HID: asus: use-after-free of struct ally_intf in asus_resume()
- [Medium] HID: asus: ally_vendor_keys() omits 0x93 (ROG Ally X AC button)
- [Medium] HID: asus: duplicate gamepad input_dev registered due to mapping report 0x0B
- [Medium] HID: asus: ally_dev_get_report() sizes 0x0D read from 0x5A report length
--

commit 964cb88b58989515f1ea0adc56e4d20719f664a7
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for ROG Ally handhelds

This commit adds core HID support for ASUS ROG Ally handhelds, including
device ready-state initialization, Ally X gamepad input mapping, vendor
key handling, and Ctrl+Alt+Del sequence filtering.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 46643022b68d0..2a3e8c399f0aa 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -239,12 +285,816 @@ static const struct asus_touchpad_info medion_e1239t_tp = {

[ ... ]

> +static bool ally_vendor_keys(struct input_dev *input, const u8 *data, int size)
> +{
> +	bool is_vendor;
> +
> +	if (size < 2 || data[0] != FEATURE_KBD_REPORT_ID)
> +		return false;
> +
> +	is_vendor = data[1] == 0x38 || data[1] == 0xA6 ||
> +		    data[1] == 0xA7 || data[1] == 0xA8;
> +
> +	/* Neither a vendor press nor a release: leave it to the other handlers. */
> +	if (!is_vendor && data[1] != 0)
> +		return false;
> +
> +	if (input) {
> +		/* AC short press */
> +		input_report_key(input, KEY_PROG1, data[1] == 0x38);
> +
> +		/* CC short press & release */
> +		input_report_key(input, KEY_F16,   data[1] == 0xA6);
> +
> +		/* AC long press */
> +		input_report_key(input, KEY_F17,   data[1] == 0xA7);
> +
> +		/* AC long release */
> +		input_report_key(input, KEY_F18,   data[1] == 0xA8);
> +
> +		input_sync(input);
> +	}
> +
> +	/*
> +	 * Only the vendor codes are consumed. They are reported on this
> +	 * interface's evdev above, so hid-core never sees them pressed. Note
> +	 * that 0xA8 ("AC long release") is itself a usage the firmware sends,
> +	 * not the report that clears one. Everything else, including the 0x00
> +	 * report that clears a held usage, must still reach hid-core: it can
> +	 * only release the usages it saw pressed if it sees the report that
> +	 * clears them.
> +	 */
> +	return is_vendor;
> +}

[Severity: Medium]
Should ally_vendor_keys() also check for 0x93 when handling KEY_PROG1?

Later in asus_input_mapping(), this patch maps usage 0x93 to KEY_PROG1 as
the ROG Ally X AC button:

    case 0x38: asus_map_key_clear(KEY_PROG1); break; /* ROG key */
    case 0x93: asus_map_key_clear(KEY_PROG1); break; /* ROG Ally X AC button */

Because ally_vendor_keys() only checks data[1] against 0x38, 0xA6, 0xA7,
and 0xA8, a report with data[1] == 0x93 on the ROG Ally X returns false
and falls through to hid-core, which records 0x93 in field->value[0] and
reports KEY_PROG1 = 1.

If the release report (data[1] == 0x00) is missed across suspend/resume
(where ally_resume_work_fn() -> ally_release_vendor_keys() clears
KEY_PROG1 directly on intf->input without clearing hid-core's
field->value[]), or if the firmware transitions from 0x93 to 0xA7 (AC
long press, which ally_vendor_keys() consumes while reporting
KEY_PROG1 = 0), field->value[0] in hid-core will retain 0x93 while
intf->input has KEY_PROG1 released. Wouldn't hid-core then ignore the
next 0x93 press because 0x93 is still present in field->value[]?

[ ... ]

> +static int ally_dev_set_report(struct hid_device *hdev, const u8 *buf, size_t len)
> +{
> +	return asus_kbd_set_report(hdev, buf, len);
> +}
> +
> +static int ally_dev_get_report(struct hid_device *hdev, u8 *out, size_t len)
> +{
> +	return hid_hw_raw_request(hdev, HID_ALLY_GET_REPORT_ID, out, len,
> +				  HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +}
> +
> +/**
> + * ally_gamepad_send_packet() - Send a raw packet to the gamepad device
> + * @hdev: HID device
> + * @buf: buffer containing the packet data
> + * @len: length of data to send
> + *
> + * Return: count of data transferred, negative if error.
> + */
> +static int ally_gamepad_send_packet(struct hid_device *hdev, const u8 *buf, size_t len)
> +{
> +	return ally_dev_set_report(hdev, buf, len);
> +}
> +
> +/**
> + * ally_gamepad_send_receive_packet() - Send a packet and receive the response
> + * @hdev: HID device
> + * @buf: buffer containing the packet data to send and receive response in
> + * @len: length of buffer
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +static int ally_gamepad_send_receive_packet(struct hid_device *hdev,
> +					    u8 *buf, size_t len)
> +{
> +	int ret;
> +
> +	ret = ally_dev_set_report(hdev, buf, len);
> +	if (ret >= 0) {
> +		memset(buf, 0, len);
> +		ret = ally_dev_get_report(hdev, buf, len);
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
As reported by Sashiko on v11 [1], ally_alloc_cmd() sizes the buffer from
FEATURE_KBD_REPORT_ID (0x5A) via asus_report_len(), and
ally_gamepad_send_receive_packet() passes that length to
ally_dev_get_report() to read HID_ALLY_GET_REPORT_ID (0x0D).

Since asus_kbd_get_report() was moved just above ally_dev_set_report(),
should ally_dev_get_report() call asus_kbd_get_report(hdev,
HID_ALLY_GET_REPORT_ID, out, len) instead of calling hid_hw_raw_request()
directly? That would both size the GET_REPORT transfer from report 0x0D's
descriptor entry and return 0 on success as documented in the kernel-doc
for ally_gamepad_send_receive_packet().

[ ... ]

> @@ -512,6 +1363,19 @@ static int asus_raw_event(struct hid_device *hdev,
>  	if (drvdata->quirks & QUIRK_MEDION_E1239T)
>  		return asus_e1239t_event(drvdata, data, size);
>  
> +	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> +		/*
> +		 * Return -1 to suppress further processing by the generic HID
> +		 * input parser for reports we fully handle. The interface is
> +		 * published with smp_store_release() only after its fields
> +		 * are set. Letting report 0x0B reach the default parser
> +		 * creates a generic gamepad and Steam Input overlaps.
> +		 */
> +		intf = smp_load_acquire(&drvdata->ally); /* fields are published */
> +		if (hid_asus_ally_raw_event(hdev, intf, data, size))
> +			return -1;
> +	}

[Severity: Medium]
As reported by Sashiko on v12 [2], does returning -1 in asus_raw_event()
actually prevent hid-core from creating a duplicate generic gamepad input
device for report 0x0B?

During asus_probe(), hid_hw_start(hdev, HID_CONNECT_DEFAULT) runs
hidinput_connect() -> hidinput_configure_usages() -> asus_input_mapping()
before any raw events arrive. Because asus_input_mapping() does not return
-1 for usages in report 0x0B (HID_ALLY_X_INPUT_REPORT), hid-core maps all
of the standard gamepad usages in report 0x0B and registers a generic
gamepad input_dev on that interface in addition to the custom
intf->gamepad device registered by ally_x_setup_input().

Should asus_input_mapping() return -1 when QUIRK_ROG_ALLY_XPAD is set and
field->report->id == HID_ALLY_X_INPUT_REPORT so that the duplicate input
device is not created?

[ ... ]

> @@ -1569,6 +2336,8 @@ static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
>  static int __maybe_unused asus_resume(struct hid_device *hdev)
>  {
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	/* Published with smp_store_release() after the fields are set. */
> +	struct ally_intf *intf = smp_load_acquire(&drvdata->ally);
>  
>  	/*
>  	 * If we have a backlight listener registered, restore the previous state,
> @@ -1578,12 +2347,16 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
>  	if (drvdata->listener.brightness_set)
>  		asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>  
> +	if (intf)
> +		schedule_delayed_work(&intf->resume_work, msecs_to_jiffies(500));
> +
>  	return 0;
>  }

[ ... ]

> @@ -1763,6 +2564,7 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  static void asus_remove(struct hid_device *hdev)
>  {
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_intf *ally;
>  
>  	if (drvdata->listener.brightness_set)
>  		asus_hid_unregister_listener(&drvdata->listener);
> @@ -1770,6 +2572,14 @@ static void asus_remove(struct hid_device *hdev)
>  	if (drvdata->xgm_led)
>  		devm_led_classdev_unregister(&hdev->dev, &drvdata->xgm_led->cdev);
>  
> +	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> +		ally = hid_asus_ally_terminate(drvdata);
> +		asus_worker_stop(drvdata->worker);
> +		hid_hw_stop(hdev);
> +		ally_free(ally);
> +		return;
> +	}

[Severity: High]
As reported by Sashiko on v12 [3], can asus_resume() access freed memory
in intf->resume_work if it races with asus_remove() or the err_stop_hw
path in asus_probe()?

In hid_asus_ally_terminate() -> ally_quiesce(), disable_delayed_work_sync()
marks intf->resume_work disabled in intf->resume_work.work.data so a
concurrent asus_resume() cannot re-queue it, and then ally_free(ally)
frees the struct ally_intf with kfree().

However, if asus_resume() has already loaded the non-NULL drvdata->ally
pointer via smp_load_acquire(&drvdata->ally) before
hid_asus_ally_terminate() clears drvdata->ally, and reaches
schedule_delayed_work(&intf->resume_work, ...) after ally_free(ally) runs,
schedule_delayed_work() will access and modify intf->resume_work.work.data
after intf has been freed. Could intf be allocated with devm_kzalloc() on
&hdev->dev (or freed via kfree_rcu() with RCU read protection in
asus_resume()) so that intf->resume_work remains valid until the driver
is unbound?

[1] https://lore.kernel.org/r/20261025235232.129945-16-denis.benato@linux.dev
[2] https://lore.kernel.org/r/20261031174130.208240-3-denis.benato@linux.dev
[3] https://lore.kernel.org/r/20261031174130.208240-3-denis.benato@linux.dev

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