Re: [PATCH v11 08/18] HID: asus: add support to force feedback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] HID: asus: TOCTOU race on ally_x_hdev in hid_asus_ally_init() causes UAF
- [Medium] HID: asus: FF init race and missing error cleanup in ally_x_setup_input()
- [Medium] HID: asus: race in hid_asus_ally_remove() and ally_x_ff_work_fn() teardown
--

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

HID: asus: add support to force feedback

This commit adds force-feedback (FF_RUMBLE) support for the ASUS ROG Ally X
gamepad interface using input_ff_create_memless() and a workqueue to send
output reports asynchronously.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index e1883ee20413c..a3a21a1493aa8 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -2674,6 +2812,36 @@ static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *all
>  	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY);
>  	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY1);
>  
> +	memcpy(&ally->ff_packet, ALLY_FORCE_FEEDBACK_OFF, sizeof(ally->ff_packet));
> +	/*
> +	 * A play_effect() racing the previous teardown can have left the
> +	 * update latch set: a fresh interface must not inherit it, or the
> +	 * first rumble work would send a packet nobody asked for.
> +	 */
> +	ally->update_ff = false;
> +	ally->ff_work_initialized = true;
> +
> +	input_set_capability(input, EV_FF, FF_RUMBLE);
> +
> +	/*
> +	 * A registered device advertising FF_RUMBLE without the memless core
> +	 * behind it would crash input_ff_upload(): fail the whole setup instead.
> +	 */
> +	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;
> +	}
> +
> +	/*
> +	 * Publish the interface before the input device becomes visible to
> +	 * userspace: an effect uploaded right after registration would
> +	 * otherwise find a NULL hdev and get dropped.
> +	 */
> +	spin_lock_irqsave(&ally_data_lock, flags);
> +	ally->ally_x_hdev = hdev;
> +	spin_unlock_irqrestore(&ally_data_lock, flags);
> +
>  	ret = input_register_device(input);
>  	if (ret) {
>  		hid_err(hdev, "Failed to register Ally X gamepad device: %d\n", ret);
> @@ -2687,20 +2855,61 @@ static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *all
>  
>  	return 0;
>  ally_x_setup_input_err:
> +	/*
> +	 * Unpublish like hid_asus_ally_remove() does, intf_mutex held: the
> +	 * cross-interface senders snapshot ally_x_hdev under ally_data_lock
> +	 * and send under intf_mutex, so the unpublish must take both in the
> +	 * same order to keep the handshake meaningful.
> +	 */
> +	scoped_guard(mutex, &ally->intf_mutex) {
> +		spin_lock_irqsave(&ally_data_lock, flags);
> +		if (ally->ally_x_hdev == hdev)
> +			ally->ally_x_hdev = NULL;
> +		spin_unlock_irqrestore(&ally_data_lock, flags);
> +	}
> +
>  	input_free_device(input);
>  	return ret;
>  }

[Severity: Medium]
In ally_x_setup_input(), ally->ff_packet, ally->update_ff, and
ally->ff_work_initialized are updated without holding ally->ff_lock. In
addition, if input_ff_create_memless() or input_register_device() fails and
jumps to ally_x_setup_input_err, ally->ff_work_initialized remains set to
true and ally->ff_work is not cancelled, while hid_asus_ally_probe() returns
an error before setting drvdata->rog_ally (leaving hid_asus_ally_remove() as
a no-op).

Also, ally_x_setup_input() publishes ally->ally_x_hdev and calls
input_register_device(input) before hid_asus_ally_probe() sends the initial
ally_x_send_ff_off(hdev) packet (and hid_asus_ally_probe() then assigns
ally_drvdata.ally_x_hdev = hdev a second time). If userspace opens the new
input device immediately upon registration and triggers an effect via
ally_x_play_effect() -> ally_x_ff_work_fn(), can the late
ally_x_send_ff_off(hdev) call in hid_asus_ally_probe() overwrite and silence
that active rumble effect?

Should the initial ally_x_send_ff_off(hdev) happen before registering the
input device, with ally->ff_packet, ally->update_ff, and
ally->ff_work_initialized initialized under ally->ff_lock and cleaned up on
the ally_x_setup_input_err path?

>  
>  static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *ally)
>  {
> +	struct hid_device *x_hdev;
>  	struct ally_config *cfg;
> +	unsigned long flags;
>  	int ret;
>  
> -	/* 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);
> +	/*
> +	 * Snapshot the gamepad interface and send the force-feedback "off"
> +	 * packet through it. usbhid_stop() frees urbout before NULLing it
> +	 * while usbhid_output_report() dereferences it unlocked, so a send
> +	 * overlapping the transport teardown would be a use-after-free: the
> +	 * device lock is taken around the send because the HID core holds
> +	 * it across remove() and the hid_hw_stop() that follows, making the
> +	 * two mutually exclusive. It is taken before intf_mutex, the same
> +	 * order the core's remove path uses, so no lock inversion exists.
> +	 */
> +	spin_lock_irqsave(&ally_data_lock, flags);
> +	x_hdev = ally->ally_x_hdev;
> +	if (x_hdev)
> +		get_device(&x_hdev->dev);
> +	spin_unlock_irqrestore(&ally_data_lock, flags);
> +
> +	if (x_hdev) {
> +		/* Failure at this point is non-critical */
> +		scoped_guard(mutex, &x_hdev->dev.mutex) {
> +			scoped_guard(mutex, &ally->intf_mutex) {
> +				ret = ally_x_send_ff_off(x_hdev);
> +
> +				if (ret < 0)
> +					hid_err(hdev,
> +						"Ally failed to init force-feedback off: %d\n",
> +						ret);
> +			}
> +		}
> +
> +		put_device(&x_hdev->dev);
> +	}

[Severity: Critical]
Can this sequence in hid_asus_ally_init() race with gamepad interface removal
and cause a use-after-free of usbhid (x_hdev->driver_data)?

In hid_asus_ally_init(), x_hdev is snapshotted from ally->ally_x_hdev under
ally_data_lock before acquiring x_hdev->dev.mutex and ally->intf_mutex, and
ally->ally_x_hdev == x_hdev is not re-checked after the mutexes are acquired.

If the gamepad interface is concurrently being disconnected or unbound:

CPU 0 (cfg probe or reset_resume)       CPU 1 (gamepad disconnect / unbind)
---------------------------------       -----------------------------------
hid_asus_ally_init()
  spin_lock_irqsave(&ally_data_lock)
  x_hdev = ally->ally_x_hdev;
  get_device(&x_hdev->dev);
  spin_unlock_irqrestore(&ally_data_lock)
                                        usbhid_disconnect()
                                          hid_destroy_device(x_hdev)
                                            device_release_driver_internal()
                                              mutex_lock(&x_hdev->dev.mutex)
                                              asus_remove()
                                                hid_asus_ally_remove()
                                                  ally->ally_x_hdev = NULL;
                                                hid_hw_stop(x_hdev)
                                                  usbhid_stop(x_hdev)
                                              mutex_unlock(&x_hdev->dev.mutex)
                                          kfree(usbhid);
  mutex_lock(&x_hdev->dev.mutex)
  mutex_lock(&ally->intf_mutex)
  ally_x_send_ff_off(x_hdev)
    hid_hw_output_report(x_hdev, ...)
      usbhid_output_report(x_hdev, ...)
        usbhid = x_hdev->driver_data;
        if (!usbhid->urbout)  <-- UAF!

Because get_device(&x_hdev->dev) only pins struct hid_device and not
struct usbhid_device, usbhid_disconnect() frees usbhid as soon as
hid_destroy_device() releases x_hdev->dev.mutex:

drivers/hid/usbhid/hid-core.c:usbhid_disconnect() {
    ...
    hid_destroy_device(hid);
    kfree(usbhid);
}

When hid_asus_ally_init() then acquires x_hdev->dev.mutex and calls
ally_x_send_ff_off(x_hdev), usbhid_output_report() dereferences the freed
x_hdev->driver_data (or, on sysfs driver unbind without USB disconnect,
accesses a stopped transport after hid_hw_stop()).

Should hid_asus_ally_init() re-verify that ally->ally_x_hdev == x_hdev under
ally_data_lock after acquiring x_hdev->dev.mutex and ally->intf_mutex before
calling ally_x_send_ff_off(x_hdev)?

[ ... ]
> @@ -3038,31 +3259,81 @@ static void hid_asus_ally_remove(struct hid_device *hdev, struct ally_handheld *
>  	cancel_delayed_work_sync(&ally->resume_work);
>  
>  	spin_lock_irqsave(&ally_data_lock, flags);
> -	if (ally->ally_x_hdev == hdev) {
> -		x_input = ally->ally_x_input;
> -		ally->ally_x_input = NULL;
> -		ally->ally_x_hdev = NULL;
> +	owns_xpad = ally->ally_x_hdev == hdev;
> +	spin_unlock_irqrestore(&ally_data_lock, flags);
> +
> +	if (owns_xpad) {
> +		/*
> +		 * Stop queueing force-feedback work before the gamepad
> +		 * pointers are cleared: play_effect() tests the flag under the
> +		 * same lock, so no work can be queued past the cancel below.
> +		 */
> +		scoped_guard(spinlock_irqsave, &ally->ff_lock)
> +			ally->ff_work_initialized = false;
> +
> +		/*
> +		 * cancel_work_sync() may sleep: keep it out of ally_data_lock,
> +		 * but run it before the pointers are cleared so in-flight work
> +		 * cannot outlive the interface it sends through.
> +		 */
> +		cancel_work_sync(&ally->ff_work);
> +
> +		/*
> +		 * The input core can no longer stop effects through the
> +		 * disabled work: quiesce any rumble still playing ourselves,
> +		 * or the device would keep vibrating after being unbound.
> +		 *
> +		 * Serialize the packet with the other force-feedback senders:
> +		 * intf_mutex is taken again below for the pointer
> +		 * unpublishing, but the two critical sections never nest.
> +		 */
> +		scoped_guard(mutex, &ally->intf_mutex) {
> +			if (ally_x_send_ff_off(hdev) < 0)
> +				hid_warn(hdev,
> +					 "Failed to stop force-feedback\n");
> +		}
>  	}
>  
>  	/*
> -	 * The keyboard interface is torn down before the config one, and
> -	 * its input_dev is freed with it. handle_ally_event() and
> -	 * ally_resume_work_fn() both report keys through it from the
> -	 * config endpoint, so drop the references here or they dangle.
> +	 * Serialize with hid_asus_ally_init(): it sends the force-feedback
> +	 * "off" packet through the gamepad interface recorded here, whose
> +	 * transport is stopped by hid_hw_stop() as soon as this function
> +	 * returns. Holding intf_mutex while the gamepad pointers are
> +	 * unpublished makes init's snapshot-and-send atomic with the
> +	 * removal: the packet either reaches a transport that is still
> +	 * running, or is not sent at all. The lock must be released before
> +	 * the sysfs teardown below, or an in-flight sysfs store blocked on
> +	 * it would deadlock against kernfs waiting for the callback.
>  	 */
> -	if (ally->keyboard_hdev == hdev) {
> -		ally->keyboard_input = NULL;
> -		ally->keyboard_hdev = NULL;
> -	}
> +	scoped_guard(mutex, &ally->intf_mutex) {
> +		spin_lock_irqsave(&ally_data_lock, flags);
> +		if (owns_xpad) {
> +			x_input = ally->ally_x_input;
> +			ally->ally_x_input = NULL;
> +			ally->ally_x_hdev = NULL;
> +		}

[Severity: Medium]
In hid_asus_ally_remove(), ally->ff_work_initialized is cleared,
ally->ff_work is cancelled, and ally->ally_x_hdev is set to NULL before
input_unregister_device(x_input) is called further down in the function.

Until input_unregister_device(x_input) disconnects evdev (flushing active
effects via input_flush_device() -> input_ff_flush() -> erase_effect() ->
ml_ff_playback()) and stops the ff-memless timer via ml_ff_stop(),
ally_x_play_effect() can still run on x_input. Because clearing
ally->ally_x_hdev releases the HID_ALLY_X_INTF_IN role reservation in
hid_asus_ally_probe(), a concurrent re-probe can enter ally_x_setup_input()
and re-enable ally->ff_work_initialized while x_input's teardown in
input_unregister_device(x_input) is still invoking ally_x_play_effect().

Similarly, in ally_x_ff_work_fn(), hdev = ally->ally_x_hdev is snapshotted
under ally_data_lock before acquiring ally->intf_mutex, without re-checking
that ally->ally_x_hdev == hdev once ally->intf_mutex is held (which can race
with the ally_x_setup_input_err unpublish path).

Would it be safer to unregister x_input before disabling and cancelling
ally->ff_work and clearing ally->ally_x_hdev, and to re-check
ally->ally_x_hdev == hdev under ally->intf_mutex in ally_x_ff_work_fn()?

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