Re: [PATCH v2 2/4] HID: hid-msi: Add M-key mapping attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `cfg_mutex` guard in `claw_hw_output_report()` is scoped to an `if` block, releasing the lock before the critical section executes.
- [High] Lockless concurrent access to `profile_pending` between process and interrupt contexts causes data corruption and sequence mismatches on delayed responses.
- [High] Unsafe assumption of USB transport in `claw_probe()` causes type confusion and invalid pointer dereference for non-USB devices.
- [High] Returning an error from `claw_profile_event()` for a spurious event unconditionally triggers `complete()`, causing premature wakeups of unrelated commands.
- [Medium] The function `claw_buttons_store()` illegally mixes scope-based `guard()` cleanup with `goto`-based error handling.
--

commit 42be71c046dc9b6b31d28598f5e0bb568d78b7a1
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add M-key mapping attributes

Adds attributes that allow for remapping the M-keys with up to 5 values
when in macro mode.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 8915942af15e6..13ba2747fdb66 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -125,6 +294,31 @@ static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
>  	return 0;
>  }
>  
> +static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
> +{
> +	u8 *codes;
> +	int i;
> +
> +	switch (drvdata->profile_pending) {
> +	case CLAW_M1_PENDING:
> +	case CLAW_M2_PENDING:
> +		codes = (drvdata->profile_pending == CLAW_M1_PENDING) ?
> +			drvdata->m1_codes : drvdata->m2_codes;
> +		/* Extract key codes; replace disabled (0xff) with 0x00, which is (null) in _show */
> +		for (i = 0; i < CLAW_KEYS_MAX; i++)
> +			codes[i] = (cmd_rep->data[6 + i] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
> +		break;

Is it safe to read and modify profile_pending locklessly here in the
interrupt handler?

Since claw_buttons_show() modifies this state in process context under
profile_mutex, if a sysfs request times out and a subsequent request starts,
could the delayed ACK from the first request cause this handler to see the
new profile_pending state and corrupt the new request's buffer?

> +	default:
> +		dev_warn(&drvdata->hdev->dev,
> +			 "Got profile event without changes pending from command: %x\n",
> +			 cmd_rep->cmd);
> +		return -EINVAL;
> +	}
> +	drvdata->profile_pending = CLAW_NO_PENDING;
> +
> +	return 0;
> +}
> +
>  static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
>  			  u8 *data, int size)
>  {

[ ... ]

> @@ -146,6 +340,9 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
>  	case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
>  		ret = claw_gamepad_mode_event(drvdata, cmd_rep);
>  		break;
> +	case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> +		ret = claw_profile_event(drvdata, cmd_rep);
> +		break;
>  	case CLAW_COMMAND_TYPE_ACK:
>  		break;
>  	default:

When claw_profile_event() returns -EINVAL for a spurious event,
claw_raw_event() breaks from the switch and unconditionally calls
complete(&drvdata->send_cmd_complete).

Could this prematurely wake up a thread waiting on a completely different
command, such as WRITE_PROFILE_DATA?

[ ... ]

> @@ -366,6 +563,161 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_WO(reset);
>  
> +static int button_mapping_name_to_code(const char *name)
> +{

[ ... ]

> +static int claw_buttons_store(struct device *dev, const char *buf, u8 mkey_idx)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	u8 data[] = { 0x01, (drvdata->bmap_addr[mkey_idx] >> 8) & 0xff,
> +		      drvdata->bmap_addr[mkey_idx] & 0xff, 0x07,
> +		      0x04, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff };
> +	size_t len = ARRAY_SIZE(data);
> +	int ret, key_count, i;
> +	char **raw_keys;
> +
> +	raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
> +	if (!raw_keys)
> +		return -ENOMEM;
> +
> +	guard(mutex)(&drvdata->rom_mutex); /* all err_free paths must be in scope */
> +	if (key_count > CLAW_KEYS_MAX) {
> +		ret = -EINVAL;
> +		goto err_free;
> +	}
> +
> +	if (key_count == 0)
> +		goto set_buttons;
> +
> +	for (i = 0; i < key_count; i++) {
> +		ret = button_mapping_name_to_code(raw_keys[i]);
> +		if (ret < 0)
> +			goto err_free;
> +
> +		data[6 + i] = ret;
> +	}
> +
> +set_buttons:
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA, data, len, 8);

While reviewing the addition of these sysfs attributes, I noticed a
pre-existing issue in claw_hw_output_report() that is exacerbated by
the concurrent sysfs requests added here:

drivers/hid/hid-msi.c:claw_hw_output_report() {
    ...
    if (timeout) {
        guard(mutex)(&drvdata->cfg_mutex);
        reinit_completion(&drvdata->send_cmd_complete);
    }
    ...
}

Since guard() is scoped to the if block, the mutex is immediately released at
the closing brace.

Doesn't this mean hid_hw_output_report() and
wait_for_completion_interruptible_timeout() execute entirely without
the lock, defeating the intended serialization and allowing concurrent
requests to race?

> +	if (ret < 0)
> +		goto err_free;
> +
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
> +
> +err_free:
> +	argv_free(raw_keys);
> +	return ret;
> +}

Is it safe to mix goto-based error handling with scope-based guard() cleanup
in the same function?

According to the cleanup guidelines, functions should either convert
entirely to scope-based cleanup, for example by using a DEFINE_FREE
wrapper for argv_free, or not use guard() at all to avoid confusing
ownership semantics.

[ ... ]

> @@ -432,8 +796,31 @@ static void cfg_resume_fn(struct work_struct *work)
>  		dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
>  }
>  
> +static void claw_features_supported(struct claw_drvdata *drvdata)
> +{

[ ... ]

>  static int claw_probe(struct hid_device *hdev, u8 ep)
>  {
> +	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> +	struct usb_device *udev = interface_to_usbdev(intf);

Is it guaranteed that this device is always connected via USB?

If this driver is bound to a device connected over a non-USB transport like
I2C, Bluetooth, or UHID, hdev->dev.parent would not be a valid USB
interface, which could cause an invalid pointer dereference here.

Could this code check hid_is_usb(hdev) first or read the version from
hdev->version instead?

>  	struct claw_drvdata *drvdata;
>  	int ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=2