Re: [PATCH v3 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] Unsynchronized state machine and shared completions cause data corruption and races.
- [High] Blind cast of parent device to `usb_interface` causes a kernel panic via uhid.
- [High] 8ms timeout for hardware operations is dangerously short and will cause spurious failures.
- [Medium] Mixing goto-based cleanup with `scoped_guard` violates cleanup subsystem guidelines.
- [Medium] Unbounded kernel log spam on unsolicited device events.
--

commit f6e1e19ecedd5b80861e87d4606aa90bac28941a
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 89bb32f00bfc3..6ccb9666daedf 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,
[ ... ]
> +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) ?

Can reading drvdata->profile_pending locklessly here cause data races?

It is modified under profile_mutex in process context (e.g. in
claw_buttons_show), which could allow a delayed response to silently
overwrite the wrong profile's memory buffer if another thread is executing
a new request.

> +			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;
> +	default:
> +		dev_warn(&drvdata->hdev->dev,
> +			 "Got profile event without changes pending from command: %x\n",
> +			 cmd_rep->cmd);

Could this cause unbounded kernel log spam?

Since HID devices act as untrusted input and can send up to 1000 reports
per second, a malicious or malfunctioning device could flood the system
and cause a local denial of service via excessive logging. Should this
use dev_warn_ratelimited() or silently drop unsolicited packets?

> +		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)
>  {
> @@ -364,6 +561,163 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
[ ... ]
> +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;
> +
> +	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:
> +	scoped_guard(mutex, &drvdata->rom_mutex) {
> +		ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> +					    data, len, 8);

Are these 8ms timeouts dangerously short?

On kernel configurations where HZ=100, msecs_to_jiffies(8) yields 1 jiffy. A
1-jiffy timeout can expire almost immediately depending on the timing of the
next timer tick. Will this falsely abort the wait and return -EBUSY before
the hardware has time to process the USB HID request?

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

Does mixing goto-based cleanup labels with scoped_guard violate the cleanup
subsystem guidelines?

The kernel guidelines explicitly forbid mixing goto-based cleanups with
scope-based cleanups in the same function, as it creates confusing ownership
semantics and can lead to LIFO ordering bugs.

> +	argv_free(raw_keys);
> +	return ret;
> +}
> +
> +static int claw_buttons_show(struct device *dev, char *buf, enum claw_key_index m_key)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	u8 data[] = { 0x01, (drvdata->bmap_addr[m_key] >> 8) & 0xff,
> +		      drvdata->bmap_addr[m_key] & 0xff, 0x07 };
> +	size_t len = ARRAY_SIZE(data);
> +	int i, ret, count = 0;
> +	const char *name;
> +	u8 *codes;
> +
> +	codes = (m_key == CLAW_KEY_M1) ? drvdata->m1_codes : drvdata->m2_codes;
> +
> +	guard(mutex)(&drvdata->profile_mutex);
> +	drvdata->profile_pending = (m_key == CLAW_KEY_M1) ? CLAW_M1_PENDING : CLAW_M2_PENDING;
> +
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, len, 8);

Because the driver uses a single shared completion (send_cmd_complete) for all
hardware responses in claw_raw_event(), could a delayed reply from an
asynchronous command cause the wait_for_completion_interruptible_timeout() in
claw_hw_output_report() to return prematurely here?

This could result in uninitialized or stale data being returned to userspace.

> +	if (ret) {
> +		drvdata->profile_pending = CLAW_NO_PENDING;
> +		return ret;
> +	}
[ ... ]
> @@ -430,8 +796,31 @@ static void cfg_resume_fn(struct work_struct *work)
[ ... ]
>  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);

Can this blind cast cause a kernel panic?

If a virtual HID device is injected from user-space using the uhid subsystem
with a matching BUS_USB identifier, the parent device is the uhid virtual
device, not a usb_interface. Accessing udev->descriptor.bcdDevice in this case
would result in an out-of-bounds memory read.

Should this use hid_is_usb(hdev) to verify the transport before executing
USB-specific initialization?

>  	struct claw_drvdata *drvdata;
>  	int ret;

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