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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent unbind of the gamepad interface during the config interface probe causes a Use-After-Free in hid_hw_output_report().
- [Low] Missing return statement after `scoped_guard` loop causes `-Wreturn-type` warning.
--

--- Patch [9]: [PATCH v3 09/12] HID: asus: add support to force feedback ---
commit c9da8775d34d1910105c3e14e76fe75184d40841
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 dc7a504e84cb6..04bf2c6ca449b 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> +static int ally_x_send_ff_report(struct ally_handheld *ally,
> +				 struct hid_device *hdev,
> +				 const u8 *buf, size_t len)
> +{
> +	u8 *dmabuf __free(kfree) = kmemdup(buf, len, GFP_KERNEL);
> +
> +	if (!dmabuf)
> +		return -ENOMEM;
> +
> +	scoped_guard(mutex, &ally->intf_mutex)
> +		return hid_hw_output_report(hdev, dmabuf, len);
> +}

[Severity: Low]
Does returning directly from within a scoped_guard leave the function
without a top-level return statement?

Because scoped_guard expands into a for loop, the compiler's static analysis
assumes the loop condition could evaluate to false, causing execution to fall
through to the end of the non-void function.

This emits a -Wreturn-type warning, which causes build failures on
configurations using -Werror.

Could this use guard() instead?

	guard(mutex)(&ally->intf_mutex);
	return hid_hw_output_report(hdev, dmabuf, len);

[ ... ]

>  static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
>  {
>  	int ret;
>  	struct ally_config *cfg;
> +	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;
> +	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 lead to a use-after-free if the gamepad interface is unbound
concurrently (e.g., via sysfs or USB hot-unplug) during the config probe?

While get_device() prevents the hid_device struct from being freed, it does
not stop the underlying transport driver (like usbhid) from unbinding.

When the transport driver unbinds, it removes the device and frees its
private data. If that happens before ally_x_send_ff_off() calls
hid_hw_output_report(), the transport's output_report callback will
dereference the already freed hdev->driver_data.

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