Re: [PATCH v4 10/13] HID: asus: add support to force feedback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent execution of the config interface probe/resume and gamepad interface unbind causes a use-after-free on HID hardware resources.
--

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

    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 bd488acdb5d49..61330ae082b16 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2756,19 +2912,43 @@ static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *all
>  
>  	return 0;
>  ally_x_setup_input_err:
> +	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);
>  	return ret;
>  }
>  
>  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);
> +	/*
> +	 * 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;
> +	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 */
> +		ret = ally_x_send_ff_off(ally, x_hdev);

[Severity: High]
Can this cross-device call cause a use-after-free on transport data structures?

While taking a reference to x_hdev->dev with get_device() prevents the
struct hid_device memory from being freed, does it stop the peer's
transport resources from being torn down?

If the configuration interface probes or resumes concurrently with the
gamepad interface being unbound (e.g., via sysfs or a reset/disconnect cycle),
it appears the transport driver could destroy the transport driver data once
hid_destroy_device() returns, while we are still holding the device reference.
Would the subsequent call to hid_hw_output_report() via ally_x_send_ff_off()
then dereference that freed memory?

> +
> +		if (ret < 0)
> +			hid_err(hdev, "Ally failed to init force-feedback off: %d\n",
> +				ret);
> +
> +		put_device(&x_hdev->dev);
> +	}
>  
>  	cfg = ally_get_config(ally);
>  	if (!cfg)

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