Re: [PATCH v5 13/13] HID: asus: add support for btn remapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A NULL pointer dereference occurs in `ally_set_button_mapping` during initialization on devices that lack turbo support, leading to a kernel panic.
- [Medium] New sysfs attributes are exposed to userspace before the driver's private data is fully initialized, causing spurious -ENODEV errors on early reads.
--

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

HID: asus: add support for btn remapping
    
This commit introduces button remapping for the ROG Ally, allowing
custom mapping for gamepad, keyboard, mouse, and media codes. It adds
new sysfs attributes for configuring the physical buttons based on the
current controller mode.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 7c10e6de438b7..a8b1d407863d7 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -1034,6 +1077,295 @@ static int ally_set_gamepad_mode(struct ally_handheld *ally, struct hid_device *
> +static int ally_set_button_mapping(struct hid_device *hdev, struct ally_handheld *ally,
> +				   struct button_pair_map *mapping)
> +{
> +	/* The MCU mapping block is four consecutive 11-byte entries starting at
> +	 * buf[5]: first remap 5-15, first macro 16-26, second remap 27-37,
> +	 * second macro 38-48 (see hid-asus-ally __btn_pair_to_pkt, BTN_CODE_LEN).
> +	 */
> +	u8 macro_bytes[11] = {0};
> +	u8 btn_bytes[11] = {0};
> +
> +	if (!mapping)
> +		return -EINVAL;
> +
> +	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_MAPPING, NULL, 0);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* This packet is slightly different from the other
> +	 * as before the packet length there is an extra byte
> +	 * which is the pair index.
> +	 */
> +	buf[3] = mapping->pair_index;
> +	buf[4] = 0x2C; /* Length */
> +
> +	/* First button mapping */
> +	buf[5] = mapping->first.remap->type;

[Severity: High]
If a device lacks turbo support, the caller skips initializing the button 
attributes, which leaves cfg->button_mappings zero-initialized.

When hid_asus_ally_init() unconditionally calls ally_apply_button_mappings(),
does this result in a NULL pointer dereference here when accessing 
mapping->first.remap->type?

> +	/* Fill in bytes 6-14 with button code */
> +	if (mapping->first.remap->type) {

[ ... ]

> @@ -3714,6 +4681,8 @@ static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
> +static ssize_t button_remap_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *ally = drvdata->rog_ally;

[Severity: Medium]
Can userspace trigger a spurious -ENODEV error here on early reads?

The sysfs files are created during hid_asus_ally_probe(), exposing them
to userspace immediately. However, drvdata->rog_ally is only assigned in
asus_probe() after hid_asus_ally_probe() returns.

If a userspace daemon reads this file in that window, drvdata->rog_ally
will be NULL. The older attributes avoided this race by caching the ally
pointer in struct ally_btn_sysfs_entry.

> +	struct button_remap_attr *btn_attr = to_button_remap_attr(attr);
> +	const struct btn_code_map *code;
> +	struct ally_config *cfg;
> +	enum ally_button_id button_id = btn_attr->button_id;
> +	enum btn_pair_index pair_idx;
> +	struct button_pair_map *pair;
> +	struct button_map *btn_map;
> +	bool is_first;
> +	int ret;
> +
> +	if (!ally)
> +		return -ENODEV;

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