Re: [PATCH v2 09/12] HID: asus: add support to force feedback

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent unbind of the gamepad interface causes a Use-After-Free of its `hid_device` in `hid_asus_ally_init()`.
- [Medium] Data race and lifecycle violation on `ally_x_hdev` causes dropped force-feedback events and undefined behavior.
--

--- Patch [9]: [PATCH v2 09/12] HID: asus: add support to force feedback ---
commit 009ee410a607fb5f11138c63d43a972421d99051
Author: Denis Benato <[email protected]>
Subject: HID: asus: add support to force feedback

Unlike ROG ally the X version and following ones uses DInput protocol
and the force feedback needs to be implemented as its protocol is
vendor-specific, therefore add support for FF_RUMBLE with magnitude
scaling on a work-queue based approach to avoid using possibly
sleeping calls in atomic context.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6f43b7317168f..ca3050fea843b 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -2519,6 +2555,86 @@ static bool ally_x_raw_event(struct input_dev *input, struct hid_device *hdev,
[ ... ]
> +static void ally_x_ff_work_fn(struct work_struct *work)
> +{
> +	struct ally_handheld *ally =
> +		container_of(work, struct ally_handheld, ff_work);
> +	struct hid_device *hdev = NULL;
> +	struct ff_report report;
> +	bool update = false;
> +	int ret;
> +
> +	scoped_guard(spinlock_irqsave, &ally->ff_lock) {
> +		if (ally->update_ff) {
> +			report = ally->ff_packet;
> +			ally->update_ff = false;
> +			update = true;
> +			hdev = ally->ally_x_hdev;
[Severity: Medium]
Will this read of ally_x_hdev race with the assignment in
hid_asus_ally_probe()?

Here in ally_x_ff_work_fn(), the hdev pointer is read under the ff_lock.
However, during probe, ally_x_hdev is assigned under a different lock
(ally_data_lock):

hid_asus_ally_probe() {
	...
	spin_lock_irqsave(&ally_data_lock, flags);
	ally_drvdata.ally_x_hdev = hdev;
	spin_unlock_irqrestore(&ally_data_lock, flags);
	...
}

> +		}
> +	}
[ ... ]
> @@ -2574,6 +2690,21 @@ static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *all
[ ... ]
> +	ret = input_ff_create_memless(input, NULL, ally_x_play_effect);
> +	if (ret) {
> +		hid_err(hdev, "Failed to create force-feedback: %d\n", ret);
> +		goto ally_x_setup_input_err;
> +	}
> +
>  	ret = input_register_device(input);
[Severity: Medium]
Is it possible for userspace to immediately send force-feedback events as
soon as this registers, before ally_x_hdev is initialized?

If input_register_device() returns successfully, the device is exposed to
userspace. If a client immediately sends an effect, ally_x_play_effect() can
schedule ally_x_ff_work_fn().

Since ally_x_hdev is only assigned later in hid_asus_ally_probe() after
setup completes, could ally_x_ff_work_fn() see a NULL ally_x_hdev and drop
the initial effects?

>  	if (ret) {
>  		hid_err(hdev, "Failed to register Ally X gamepad device: %d\n", ret);
[ ... ]
> @@ -2592,13 +2723,28 @@ static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *all
>  
>  static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *ally)
>  {
> -	int ret;
> +	struct hid_device *x_hdev;
> +	unsigned long flags;
>  
> -	/* Failure at this point is non-critical */
> -	ret = ally_gamepad_send_packet(ally, hdev, ALLY_FORCE_FEEDBACK_OFF,
> -				       sizeof(ALLY_FORCE_FEEDBACK_OFF));
> -	if (ret < 0)
> -		hid_err(hdev, "Ally failed to init force-feedback off: %d\n", ret);
> +	/*
> +	 * The force-feedback "off" packet belongs to the gamepad interface,
> +	 * which consumes it as an output report: the config interface probed
> +	 * here would reject it. The gamepad probe path sends the packet itself,
> +	 * so this only matters when the gamepad interface is already bound,
> +	 * e.g. after a reset resume that re-initialized the MCU.
> +	 */
> +	spin_lock_irqsave(&ally_data_lock, flags);
> +	x_hdev = ally->ally_x_hdev;
> +	spin_unlock_irqrestore(&ally_data_lock, flags);
> +
> +	if (x_hdev) {
> +		/* Failure at this point is non-critical */
> +		int ret = ally_x_send_ff_off(ally, x_hdev);
[Severity: High]
Can this use the hid_device after it has been freed?

The ally_x_hdev pointer is read and cached under ally_data_lock, but it
is used after the lock is dropped without acquiring a reference via
get_device().

If userspace concurrently unbinds the gamepad interface via sysfs, the
gamepad's remove callback (hid_asus_ally_remove) will clear
ally->ally_x_hdev and return, allowing the HID core to destroy and free
the hid_device structure.

Since hid_asus_ally_init() operates outside the lock without a reference,
would this result in ally_x_send_ff_off() dereferencing a freed pointer?

> +
> +		if (ret < 0)
> +			hid_err(hdev, "Ally failed to init force-feedback off: %d\n",
> +				ret);
> +	}

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