Re: [PATCH v7 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:

New issues:
- [High] A stale or late `READ_PROFILE_ACK` packet will abort an active, legitimate `READ_PROFILE` command.
- [High] A Time-Of-Check to Time-Of-Use (TOCTOU) race in `claw_profile_event()` can clobber a newly set `profile_pending` state.
- [High] The driver sends the `SYNC_TO_ROM` command without waiting for its ACK, intentionally leaving an unconsumed ACK in the pipeline.
- [Low] Early error returns leave `drvdata->profile_pending` in a dangling state.

Pre-existing issues:
- [High] Generic ACKs (`CLAW_COMMAND_TYPE_ACK`) unconditionally complete any active wait, undermining synchronization guarantees.
--

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

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

Adds sysfs attributes to allow remapping the M-keys with up to 5 values when in
macro mode. The driver maps gamepad buttons, keyboard keys, and mouse events,
using device endpoints or event devices depending on the mapping type.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index d95483907a5e5..193435ec9386c 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -136,6 +320,39 @@ 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)
> +{
> +	enum claw_profile_ack_pending profile;
> +	struct claw_mkey_report *mkeys;
> +	u8 *codes, key;
> +	int i;
> +
> +	scoped_guard(spinlock, &drvdata->profile_lock)
> +		profile = drvdata->profile_pending;
> +
> +	switch (profile) {
> +	case CLAW_M1_PENDING:
> +	case CLAW_M2_PENDING:
> +		key = (profile == CLAW_M1_PENDING) ? CLAW_KEY_M1 : CLAW_KEY_M2;
> +		mkeys = (struct claw_mkey_report *)cmd_rep->data;
> +		if (be16_to_cpu(mkeys->read_addr) != drvdata->bmap_addr[key])
> +			return -EINVAL;

[Severity: Low]
If this returns early because of an address mismatch, will profile_pending be
left in a dangling state instead of being cleared to CLAW_NO_PENDING?

[ ... ]

> +	default:
> +		dev_dbg(&drvdata->hdev->dev,
> +			"Got profile event without changes pending from command: %x\n",
> +			cmd_rep->cmd);
> +		return -EINVAL;
> +	}
> +	scoped_guard(spinlock, &drvdata->profile_lock)
> +		drvdata->profile_pending = CLAW_NO_PENDING;

[Severity: High]
Can a concurrent sysfs read overwrite the profile_pending state during the
window between releasing and re-acquiring the lock?

If another CPU acquires the lock and sets a new state, such as CLAW_M2_PENDING,
won't this unconditionally overwrite that new state with CLAW_NO_PENDING and
break the subsequent command's wait?

> +
> +	return 0;
> +}

[ ... ]

> @@ -166,6 +383,19 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
>  			}
>  		}
>  
> +		break;
> +	case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> +		ret = claw_profile_event(drvdata, cmd_rep);
> +
> +		scoped_guard(spinlock, &drvdata->cmd_lock) {
> +			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);
> +			}
> +		}
> +
> +		break;

[Severity: High]
If a late or stale READ_PROFILE_ACK arrives during an active READ_PROFILE
command wait, it will cause claw_profile_event() to return -EINVAL.

Will this unconditionally assign the -EINVAL to cmd_status and complete
the wait prematurely, aborting the active valid command instead of ignoring
the mismatched ACK?

>  	case CLAW_COMMAND_TYPE_ACK:
>  		scoped_guard(spinlock, &drvdata->cmd_lock) {
>  			if (drvdata->waiting_for_ack) {
>  				drvdata->cmd_status = 0;
>  				drvdata->waiting_for_ack = false;
>  				complete(&drvdata->send_cmd_complete);
>  			}

[Severity: High]
This is a pre-existing issue, but does this generic ACK handler blindly
complete any active wait without checking drvdata->waiting_cmd?

If a generic ACK arrives while a command like READ_PROFILE is actively
waiting for a specific data payload, the generic ACK will fulfill the wait
early with a success status, causing the caller to falsely assume the read
succeeded and consume stale data.

[ ... ]

> @@ -433,6 +663,173 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_WO(reset);
>  
> +static int mkey_mapping_name_to_code(const char *name)
...
> +static int claw_mkey_store(struct device *dev, const char *buf, u8 mkey)
> +{
...
> +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 sends SYNC_TO_ROM with a timeout of 0, doesn't it intentionally
leave an unconsumed ACK in the pipeline?

Because the generic ACK handler doesn't check waiting_cmd, won't this
delayed ACK unconditionally complete the next command issued to the device
that waits for a response, such as claw_mkey_show(), breaking the host-device
synchronization?

> +	}
> +
> +	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;
> +
> +	/* 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: Low]
If claw_hw_output_report() fails, such as due to a timeout, does this early
return leave profile_pending in a dangling state instead of clearing it
back to CLAW_NO_PENDING?

[ ... ]

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