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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Cross-device pointer access causes a Use-After-Free race when the keyboard interface is unbound.
- [High] Custom keyboard events silently fail because keyboard_input is extracted before the inputs list is populated.
- [High] The use of a global singleton `ally_drvdata` structure without enforcing a single connected device allows malicious state overwrites and cross-device Use-After-Free.
- [Medium] The custom Ally X gamepad input device lacks open and close callbacks, risking runtime PM suspension.
--

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

HID: asus: add support for ROG Ally handhelds

Implement the core functionality for ASUS handhelds by extending hid-asus
functionality if the driver is enabled. The device is initialized to a ready
state, Ally X input mapping is implemented, and a proper event is emitted
for the AC button long press.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 9ce21fcc7d2e0..88b30726f6c67 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> +/*
> + * Changes to ally_drvdata must lock: the raw_event callbacks, which may
> + * run in atomic (URB completion) context, also take this lock, so it must
> + * be a spinlock.
> + */
> +static DEFINE_SPINLOCK(ally_data_lock);
> +static struct ally_handheld ally_drvdata = {

[Severity: High]
Could using a single global ally_drvdata structure cause state corruption if
multiple ROG Ally devices are connected?

If a user plugs in two devices or a spoofed USB peripheral, the second probe
unconditionally overwrites the pointers for the first device. Unbinding them
later could lead to use-after-free or dangling pointers.

Should this state be allocated dynamically per device instance instead?

> +	.intf_mutex = __MUTEX_INITIALIZER(ally_drvdata.intf_mutex),
> +	/*
> +	 * Initialised statically so it is always safe to cancel, whichever
> +	 * of the interfaces probed or failed to probe.
> +	 */

[ ... ]

> +static bool handle_ally_event(struct hid_device *hdev, struct ally_handheld *ally,
> +			      u8 *data, int size)
> +{

[ ... ]

> +		spin_lock_irqsave(&ally_data_lock, flags);
> +		keyboard_input = ally->keyboard_input;
> +		spin_unlock_irqrestore(&ally_data_lock, flags);
> +
> +		if (!keyboard_input)
> +			return false;
> +
> +		input_report_key(keyboard_input, keycode, 1);

[Severity: High]
Is it possible for an input_dev use-after-free to occur here? The
keyboard_input pointer is fetched under ally_data_lock, but the lock is
dropped before calling input_report_key().

If the device is concurrently unbound, hid_asus_ally_remove() will clear the
pointer from the global state and the input core will free the input_dev,
meaning the concurrent event handler would access freed memory.

Would taking a reference to the input device before dropping the lock
prevent this race?

> +		input_sync(keyboard_input);
> +		input_report_key(keyboard_input, keycode, 0);
> +		input_sync(keyboard_input);
> +		return true;
> +	}

[ ... ]

> +static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *ally)
> +{
> +	struct input_dev *input = ally_x_alloc_input_dev(hdev);

[ ... ]

> +	input_set_capability(input, EV_KEY, KEY_F18);
> +	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY);
> +	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY1);
> +
> +	ret = input_register_device(input);

[Severity: Medium]
Does the custom Ally X gamepad input device need open and close callbacks?

Usually, standard hid-input devices assign these callbacks to invoke
hid_hw_open() and hid_hw_close(). Without them, opening just the gamepad
node might not wake the hardware from suspend or prevent it from sleeping,
which could lead to dropped events or input latency during runtime PM
suspension.

> +	if (ret) {
> +		hid_err(hdev, "Failed to register Ally X gamepad device: %d\n", ret);
> +		goto ally_x_setup_input_err;
> +	}

[ ... ]

> +static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
> +{

[ ... ]

> +	case HID_ALLY_INTF_KEYBOARD_IN:
> +		spin_lock_irqsave(&ally_data_lock, flags);
> +		ally_drvdata.keyboard_hdev = hdev;
> +		if (!list_empty(&hdev->inputs)) {
> +			hidinput = list_first_entry(&hdev->inputs, struct hid_input, list);
> +			ally_drvdata.keyboard_input = hidinput->input;
> +		}

[Severity: High]
Will ally_drvdata.keyboard_input ever be populated here? Looking at
asus_probe(), hid_asus_ally_probe() is called before hid_hw_start().

At this point, the HID core has not yet called hidinput_connect(), so
hdev->inputs is guaranteed to be empty. Because of this, the keyboard_input
pointer remains NULL, and custom keyboard events might be silently discarded
in handle_ally_event().

Could this initialization be deferred until after hid_hw_start() populates
the inputs list?

> +		spin_unlock_irqrestore(&ally_data_lock, flags);
> +		break;
> +	default:
> +		/* This is normally supposed to happen */
> +		break;

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