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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Typographical error checking `gamepad_registered` instead of `gp_registered` causes newly added sysfs attributes to unconditionally return `-ENODEV`.
- [High] Dangling asynchronous ACK from `SYNC_TO_ROM` can prematurely complete subsequent commands, corrupting the device state machine.
- [Medium] Spurious or late `READ_PROFILE_ACK` packets incorrectly abort subsequent profile reads with `-EINVAL`.
- [Medium] If `claw_hw_output_report()` fails in `claw_mkey_show()`, the function exits without clearing the `profile_pending` state.
--

commit fe7de95a59af02c44e638aba1fcc1763f3b5354f
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. Mapped events will fire from gamepad, keyboard, or mouse evdevs.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 3a176f44f2c43..5f20102eae69d 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -87,24 +249,47 @@ struct claw_command_report {
>  	u8 data[59];
>  } __packed;
>  
[ ... ]
>  struct claw_drvdata {
>  	/* MCU General Variables */
> +	enum claw_profile_ack_pending profile_pending;
>  	struct completion send_cmd_complete;
[ ... ]
>  	/* Gamepad Variables */
>  	enum claw_mkeys_function_index mkeys_function;
>  	enum claw_gamepad_mode_index gamepad_mode;
> +	u8 m1_codes[CLAW_KEYS_MAX];
> +	u8 m2_codes[CLAW_KEYS_MAX];
> +	bool gamepad_registered;
>  	spinlock_t mode_lock; /* Lock for mode data read/write */
> +	const u16 *bmap_addr;
> +	bool bmap_support;
>  	bool gp_registered;
>  };
[ ... ]
> @@ -165,9 +383,20 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
>  			complete(&drvdata->send_cmd_complete);
>  		}
>  
> +		break;
> +	case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> +		ret = claw_profile_event(drvdata, cmd_rep);
> +		if (drvdata->waiting_for_ack &&
> +		    drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_PROFILE) {
> +			drvdata->cmd_status = ret;
> +			drvdata->waiting_for_ack = false;
> +			complete(&drvdata->send_cmd_complete);
> +		}

[Severity: Medium]
If a spurious or late READ_PROFILE_ACK packet arrives, claw_profile_event()
will return -EINVAL. Does this unconditionally pass the error to cmd_status
and prematurely complete the wait for a valid, currently pending READ_PROFILE
command?

> +
> +		break;
>  	case CLAW_COMMAND_TYPE_ACK:
[ ... ]
> @@ -442,6 +671,177 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
[ ... ]
> +static int claw_mkey_store(struct device *dev, const char *buf, u8 mkey)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct claw_mkey_report report = { {0x01, cpu_to_be16(drvdata->bmap_addr[mkey])},
> +				   0x07, 0x04, 0x00, {0xff, 0xff, 0xff, 0xff, 0xff} };
> +	char **raw_keys __free(argv_free) = NULL;
> +	int ret, key_count, i;
> +
> +	scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> +		/* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
> +		if (!smp_load_acquire(&drvdata->gamepad_registered))
> +			return -ENODEV;
> +	}

[Severity: High]
Does this check the newly added gamepad_registered field instead of the
already existing gp_registered field?

Since gamepad_registered is never set to true, it looks like this will
unconditionally return -ENODEV, rendering the sysfs attributes inaccessible.

> +
> +	raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
[ ... ]
> +set_buttons:
> +	scoped_guard(mutex, &drvdata->rom_mutex) {
> +		ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> +					    (u8 *)&report, sizeof(report), 25);
> +		if (ret)
> +			return ret;
> +		/* MCU will not send ACK until the USB transaction completes. ACK is sent
> +		 * immediately after and will hit the stale state machine, before the next
> +		 * command re-arms the state machine. Timeout 0 ensures no deadlock waiting
> +		 * for ACK that ill never come.
> +		 */
> +		ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);

[Severity: High]
Since this command uses a timeout of 0, could the asynchronous ACK from
SYNC_TO_ROM arrive while the driver is waiting for a subsequent command
(like another WRITE_PROFILE_DATA)?

If the generic ACK arrives after the driver starts waiting for the next
command, it seems it could falsely complete the next command prematurely
and break the lock-step protocol between host and MCU.

> +	}
> +
> +	return ret;
> +}
> +
> +static int claw_mkey_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);
> +	struct claw_mkey_report report = { {0x01, cpu_to_be16(drvdata->bmap_addr[m_key])}, 0x07 };
> +	int i, ret, count = 0;
> +	const char *name;
> +	u8 *codes;
> +
> +	scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> +		/* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
> +		if (!smp_load_acquire(&drvdata->gamepad_registered))
> +			return -ENODEV;
> +	}
> +
> +	codes = (m_key == CLAW_KEY_M1) ? drvdata->m1_codes : drvdata->m2_codes;
> +
> +	guard(mutex)(&drvdata->profile_mutex);
> +	scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
> +		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,
> +				    (u8 *)&report, sizeof(report), 25);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If claw_hw_output_report() fails or times out, this returns immediately.
Does this leave drvdata->profile_pending set to a pending state?

If the state is left dangling after the mutex is released, could a subsequent
late or spurious profile packet be incorrectly accepted and parsed into the
local codes arrays?

> +
> +	for (i = 0; i < CLAW_KEYS_MAX; i++) {

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