Re: [PATCH v12 11/17] HID: asus: add support for turbo buttons

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] HID: asus: ABBA deadlock between ally_prefs.lock and sysfs removal
- [High] HID: asus: UAF of ally_prefs.button_entries on second device removal
- [Low] HID: asus: mix of cleanup helpers and goto in ally_prefs_setup() and helper
- [Low] HID: asus: malformed kernel-doc comment on ally_set_turbo_params()
- [Low] HID: asus: per-call stack table and manual copy in ally_btn_param() and setter
--

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

HID: asus: add support for turbo buttons

This commit adds support for configuring ROG Ally turbo button parameters
by exposing turbo_period and toggle_period sysfs attributes for each
gamepad button and restoring the settings during device initialization.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 3e58c5ca0cbd8..d20e433af5740 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -812,6 +812,80 @@ struct ally_joystick_resp_curve {

[ ... ]

> +/* Payload order. Not the same order as enum ally_button_id. */
> +struct ally_turbo_config {
> +	struct ally_btn_turbo_params btn_du;
> +	struct ally_btn_turbo_params btn_dd;
> +	struct ally_btn_turbo_params btn_dl;
> +	struct ally_btn_turbo_params btn_dr;
> +	struct ally_btn_turbo_params btn_j0b;
> +	struct ally_btn_turbo_params btn_j1b;
> +	struct ally_btn_turbo_params btn_lb;
> +	struct ally_btn_turbo_params btn_rb;
> +	struct ally_btn_turbo_params btn_a;
> +	struct ally_btn_turbo_params btn_b;
> +	struct ally_btn_turbo_params btn_x;
> +	struct ally_btn_turbo_params btn_y;
> +	struct ally_btn_turbo_params btn_view;
> +	struct ally_btn_turbo_params btn_menu;
> +	struct ally_btn_turbo_params btn_m2;
> +	struct ally_btn_turbo_params btn_m1;
> +};

[ ... ]

> @@ -2120,9 +2197,297 @@ static const struct attribute_group ally_attr_groups[] = {
>  	},
>  };
>  
> +static struct ally_btn_turbo_params *ally_btn_param(enum ally_button_id btn)
> +{
> +	struct ally_btn_turbo_params *table[ALLY_BTN_MAX] = {
> +		[ALLY_BTN_A] = &ally_prefs.turbo.btn_a,
> +		[ALLY_BTN_B] = &ally_prefs.turbo.btn_b,
> +		[ALLY_BTN_X] = &ally_prefs.turbo.btn_x,
> +		[ALLY_BTN_Y] = &ally_prefs.turbo.btn_y,
> +		[ALLY_BTN_LB] = &ally_prefs.turbo.btn_lb,
> +		[ALLY_BTN_RB] = &ally_prefs.turbo.btn_rb,
> +		[ALLY_BTN_DU] = &ally_prefs.turbo.btn_du,
> +		[ALLY_BTN_DD] = &ally_prefs.turbo.btn_dd,
> +		[ALLY_BTN_DL] = &ally_prefs.turbo.btn_dl,
> +		[ALLY_BTN_DR] = &ally_prefs.turbo.btn_dr,
> +		[ALLY_BTN_J0B] = &ally_prefs.turbo.btn_j0b,
> +		[ALLY_BTN_J1B] = &ally_prefs.turbo.btn_j1b,
> +		[ALLY_BTN_MENU] = &ally_prefs.turbo.btn_menu,
> +		[ALLY_BTN_VIEW] = &ally_prefs.turbo.btn_view,
> +		[ALLY_BTN_M1] = &ally_prefs.turbo.btn_m1,
> +		[ALLY_BTN_M2] = &ally_prefs.turbo.btn_m2,
> +	};
> +
> +	if (btn >= ALLY_BTN_MAX)
> +		return NULL;
> +
> +	return table[btn];
> +}
> +
> +/** ally_set_turbo_params() - Set the turbo parameters
> + *
> + * Caller holds ally_prefs.lock.
> + *
> + * @hdev: The HID device to send the packet to.
> + * Return: 0 on success, negative error code on failure.
> + */

[Severity: Low]
This isn't a bug, but in drivers/hid/hid-asus.c above ally_set_turbo_params(),
the kernel-doc comment has the function identifier on the opening /** line
and places the locking note before the @hdev parameter description.

Could the opening /** be placed on its own line and the description text
moved after the parameter list so scripts/kernel-doc can parse it cleanly?

> +static int ally_set_turbo_params(struct hid_device *hdev)
> +{
> +	struct ally_turbo_config *turbo = &ally_prefs.turbo;
> +	const u8 payload[] = {
> +		turbo->btn_du.turbo, turbo->btn_du.toggle,
> +		turbo->btn_dd.turbo, turbo->btn_dd.toggle,
> +		turbo->btn_dl.turbo, turbo->btn_dl.toggle,
> +		turbo->btn_dr.turbo, turbo->btn_dr.toggle,
> +		turbo->btn_j0b.turbo, turbo->btn_j0b.toggle,
> +		turbo->btn_j1b.turbo, turbo->btn_j1b.toggle,
> +		turbo->btn_lb.turbo, turbo->btn_lb.toggle,
> +		turbo->btn_rb.turbo, turbo->btn_rb.toggle,
> +		turbo->btn_a.turbo, turbo->btn_a.toggle,
> +		turbo->btn_b.turbo, turbo->btn_b.toggle,
> +		turbo->btn_x.turbo, turbo->btn_x.toggle,
> +		turbo->btn_y.turbo, turbo->btn_y.toggle,
> +		turbo->btn_view.turbo, turbo->btn_view.toggle,
> +		turbo->btn_menu.turbo, turbo->btn_menu.toggle,
> +		turbo->btn_m2.turbo, turbo->btn_m2.toggle,
> +		turbo->btn_m1.turbo, turbo->btn_m1.toggle,
> +	};

[Severity: Low]
This isn't a bug, but in drivers/hid/hid-asus.c, ally_btn_param() rebuilds
table[ALLY_BTN_MAX] on the stack on every call instead of marking it static
const (like ally_trigger_limits, ally_anti_deadzones, and ally_resp_curves
earlier in the file), and ally_set_turbo_params() manually copies all 32
bytes into payload[] even though struct ally_turbo_config is already laid
out in hardware payload order.

Could table[] in ally_btn_param() be marked static const (or replaced with
an indexed array), and could struct ally_turbo_config be marked __packed and
passed directly to ally_alloc_cmd()?

[ ... ]

> +static ssize_t btn_turbo_period_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct ally_btn_sysfs_entry *entry;
> +	struct ally_btn_turbo_params *params;
> +
> +	entry = container_of(attr, struct ally_btn_sysfs_entry, attr_turbo_period);
> +
> +	guard(mutex)(&ally_prefs.lock);
> +
> +	if (!ally_prefs.turbo_support)
> +		return -EOPNOTSUPP;
> +
> +	params = ally_btn_param(entry->btn);
> +	if (!params)
> +		return -ENODEV;
> +
> +	return sysfs_emit(buf, "%hhu\n", params->turbo);
> +}
> +
> +static ssize_t btn_turbo_period_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct ally_btn_sysfs_entry *entry;
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct ally_btn_turbo_params *params;
> +	u8 old_value, value;
> +	int ret;
> +
> +	entry = container_of(attr, struct ally_btn_sysfs_entry, attr_turbo_period);
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > ALLY_TURBO_PERIOD_MAX)
> +		return ret ? ret : -EINVAL;
> +
> +	scoped_guard(mutex, &ally_prefs.lock) {

[ ... ]

> +static struct ally_btn_sysfs_entry *ally_create_button_attributes(struct hid_device *hdev)
> +{
> +	int i, ret;
> +
> +	if (!ally_prefs.turbo_support)
> +		return NULL;
> +
> +	struct ally_btn_sysfs_entry *entries __free(kfree) = kzalloc_objs(*entries, ALLY_BTN_MAX);
> +	if (!entries)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < ALLY_BTN_MAX; i++) {
> +		entries[i].btn = i;
> +
> +		if (!ally_btn_param(i)) {
> +			hid_err(hdev, "Invalid button id %d for turbo attributes\n", i);
> +			ret = -EINVAL;
> +			goto ally_btn_err_cleanup;
> +		}
> +
> +		ally_btn_turbo_init_attrs(&entries[i]);
> +		entries[i].group.name = ally_button_names[i];
> +		entries[i].group.attrs = entries[i].attrs;
> +
> +		ret = sysfs_create_group(&hdev->dev.kobj, &entries[i].group);
> +		if (ret < 0) {
> +			hid_err(hdev, "Failed to create sysfs group for %s: %d\n",
> +				ally_button_names[i], ret);
> +			goto ally_btn_err_cleanup;
> +		}
> +	}
> +
> +	return no_free_ptr(entries);
> +
> +ally_btn_err_cleanup:
> +	while (i--)
> +		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
> +
> +	return ERR_PTR(ret);
> +}
> +
> +static void ally_remove_button_attributes(struct hid_device *hdev)
> +{
> +	struct ally_btn_sysfs_entry *entries;
> +	int i;
> +
> +	entries = ally_prefs.button_entries;
> +	if (!entries)
> +		return;
> +
> +	for (i = 0; i < ALLY_BTN_MAX; i++)
> +		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
> +
> +	kfree(entries);
> +	ally_prefs.button_entries = NULL;
> +}
> +
>  static int ally_prefs_setup(struct hid_device *hdev)
>  {
> -	int ret, i;
> +	struct ally_btn_sysfs_entry *entries = NULL;
> +	int ret, i, cal_n = 0;
>  
>  	ret = ally_detect_capabilities(hdev);
>  	if (ret)
> @@ -2158,27 +2523,38 @@ static int ally_prefs_setup(struct hid_device *hdev)
>  	/* Skip the calibration groups when the capability is missing. */
>  	if (ally_prefs.user_cal_support || ally_prefs.anti_deadzone_support ||
>  	    ally_prefs.resp_curve_support) {
> -		int cal_i;
> -
> -		for (cal_i = 0; cal_i < ARRAY_SIZE(ally_cal_attr_groups); cal_i++) {
> +		for (; cal_n < ARRAY_SIZE(ally_cal_attr_groups); cal_n++) {
>  			ret = sysfs_create_group(&hdev->dev.kobj,
> -						 ally_cal_attr_groups[cal_i]);
> +						ally_cal_attr_groups[cal_n]);
>  			if (ret) {
>  				hid_err(hdev, "Failed to create Ally sysfs group '%s': %d\n",
> -					ally_cal_attr_groups[cal_i]->name, ret);
> -				while (cal_i--)
> -					sysfs_remove_group(&hdev->dev.kobj,
> -							   ally_cal_attr_groups[cal_i]);
> -				for (i = 0; i < ARRAY_SIZE(ally_attr_groups); i++)
> -					sysfs_remove_group(&hdev->dev.kobj,
> -							   &ally_attr_groups[i]);
> -				return ret;
> +					ally_cal_attr_groups[cal_n]->name, ret);
> +				goto err_cal_groups;
>  			}
>  		}
>  	}
>  
> +	if (ally_prefs.turbo_support) {
> +		entries = ally_create_button_attributes(hdev);
> +		if (IS_ERR(entries)) {
> +			ret = PTR_ERR(entries);
> +			hid_err(hdev, "Failed to create button attributes: %d\n", ret);
> +			goto err_cal_groups;
> +		}
> +	}
> +
> +	ally_prefs.button_entries = entries;
>  	ally_prefs.sysfs_live = true;
>  	return 0;
> +
> +err_cal_groups:
> +	while (cal_n--)
> +		sysfs_remove_group(&hdev->dev.kobj, ally_cal_attr_groups[cal_n]);
> +
> +	for (i = 0; i < ARRAY_SIZE(ally_attr_groups); i++)
> +		sysfs_remove_group(&hdev->dev.kobj, &ally_attr_groups[i]);
> +
> +	return ret;
>  }

[Severity: Low]
This isn't a bug, but in drivers/hid/hid-asus.c,
ally_create_button_attributes() mixes __free(kfree) on entries with goto
ally_btn_err_cleanup, and ally_prefs_setup() mixes
guard(mutex)(&ally_prefs.lock) with goto err_cal_groups.

Since include/linux/cleanup.h advises against mixing scope-based cleanup
helpers and goto error labels within the same function, could these
functions use either pure goto-based cleanup or pure scope-based cleanup?

>  static void ally_sysfs_remove(struct hid_device *hdev)
> @@ -2195,6 +2571,8 @@ static void ally_sysfs_remove(struct hid_device *hdev)
>  
>  	ally_prefs.sysfs_live = false;
>  
> +	ally_remove_button_attributes(hdev);

[Severity: High]
Can this trigger an ABBA deadlock between ally_prefs.lock and kernfs active
node draining?

In drivers/hid/hid-asus.c, when userspace reads or writes any of the turbo
button sysfs attributes (btn_turbo_period_show(), btn_turbo_period_store(),
btn_toggle_period_show(), or btn_toggle_period_store()), kernfs holds an
active reference on the attribute kernfs_node and the callback then acquires
ally_prefs.lock:

  kernfs_fop_read_iter() / kernfs_fop_write_iter()
    kernfs_get_active_of()          /* acquires kn->active */
    btn_turbo_period_show() / btn_turbo_period_store()
      guard(mutex)(&ally_prefs.lock)

Conversely, during device removal in ally_sysfs_remove() (and on the error
unwind paths in ally_prefs_setup() and ally_create_button_attributes()),
ally_prefs.lock is held while calling ally_remove_button_attributes() and
sysfs_remove_group(), which calls kernfs_drain() to wait synchronously for
kn->active to drain:

  ally_sysfs_remove()
    guard(mutex)(&ally_prefs.lock)
    ally_remove_button_attributes()
      sysfs_remove_group()
        kernfs_remove_by_name_ns()
          __kernfs_remove()
            kernfs_drain()          /* waits on kn->active */

If a sysfs read or write is waiting to acquire ally_prefs.lock while
ally_sysfs_remove() or ally_prefs_setup() calls sysfs_remove_group() under
ally_prefs.lock, won't both tasks deadlock waiting on each other?

[Severity: High]
Can removing a second HID_ALLY_INTF_CFG_IN device cause a use-after-free of
ally_prefs.button_entries?

In drivers/hid/hid-asus.c, when the first device (hdev1) probes,
ally_prefs_setup(hdev1) allocates ally_prefs.button_entries via
ally_create_button_attributes(hdev1), registers the attribute groups (which
embed struct device_attribute attr_turbo_period and attr_toggle_period) on
hdev1->dev.kobj, and sets ally_prefs.sysfs_live = true.

If a second matching HID_ALLY_INTF_CFG_IN interface (hdev2) is probed,
ally_prefs_setup(hdev2) first calls ally_detect_capabilities(hdev2)
(overwriting ally_prefs.turbo_support and other global capability flags),
and then sees ally_prefs.sysfs_live == true and returns 0 without
registering sysfs groups on hdev2->dev.kobj:

  ally_prefs_setup(hdev2)
    ally_detect_capabilities(hdev2)
    ...
    guard(mutex)(&ally_prefs.lock);
    ...
    if (ally_prefs.sysfs_live)
        return 0;

When hdev2 is subsequently unbound, asus_remove(hdev2) calls
ally_sysfs_remove(hdev2) -> ally_remove_button_attributes(hdev2):

  ally_remove_button_attributes(hdev2)
    entries = ally_prefs.button_entries;
    for (i = 0; i < ALLY_BTN_MAX; i++)
        sysfs_remove_group(&hdev2->dev.kobj, &entries[i].group);
    kfree(entries);
    ally_prefs.button_entries = NULL;

Because ally_prefs does not track which hid_device owns the live sysfs
groups, sysfs_remove_group() looks for the groups on hdev2->dev.kobj (leaving
them registered on hdev1->dev.kobj) and kfree(entries) frees the backing
struct ally_btn_sysfs_entry memory while hdev1's sysfs attributes are still
active. Wouldn't any subsequent sysfs access on hdev1 (or unbinding hdev1)
dereference freed memory?

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