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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ABBA deadlock between cfg_mutex and sysfs active references during device removal.
- [High] Race conditions and lockless access to profile_pending cause TOCTOU bugs and mismatched command responses.
- [Low] Redundant local redefinition of core cleanup wrapper for argv_free.
--

commit 1f541fdc7955ce5da7d37b9d253518245001f68a
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. There are 2 mappable buttons on the rear of the
device, M1 on the right and M2 on the left.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 3f809dc70a4cc..991d5a25d3de0 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -128,6 +298,30 @@ 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;

Could this cause a race condition since drvdata->profile_pending is read twice
locklessly?

If the state changes between the switch statement and the ternary operator,
could M1's profile response be incorrectly written to M2's buffer?

Is it possible to use an IRQ-safe spinlock here to protect profile_pending
and the profile buffers, given they are shared between process context and
the HID raw event context?

> +		for (i = 0; i < CLAW_KEYS_MAX; i++)
> +			codes[i] = (cmd_rep->data[6 + i]);
> +		break;
> +	default:
> +		dev_dbg(&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;
> +}

[ ... ]

> @@ -149,6 +343,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:

Since claw_raw_event() unconditionally calls
complete(&drvdata->send_cmd_complete) for any recognized ACK type after this
switch statement, could it wake up claw_buttons_show() prematurely if a
delayed or generic ACK arrives while it is waiting for READ_PROFILE_ACK?

If claw_buttons_show() wakes up prematurely, could it read uninitialized or
stale data from the codes array and leave profile_pending in a dirty state
that corrupts subsequent reads when the real ACK finally arrives?

[ ... ]

> @@ -373,6 +570,164 @@ 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)
> +{

[ ... ]

> +DEFINE_FREE(argv, char **, if (_T) argv_free(_T))

Is it necessary to locally redefine this cleanup wrapper?

The core kernel header include/linux/string.h already provides the exact same
functionality via
DEFINE_FREE(argv_free, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)).

Could this simply be declared as char **raw_keys __free(argv_free) = NULL;
to avoid duplicate code?

> +
> +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 };
> +	char **raw_keys __free(argv) = NULL;
> +	size_t len = ARRAY_SIZE(data);
> +	int ret, key_count, i;

[ ... ]

> +set_buttons:
> +	scoped_guard(mutex, &drvdata->rom_mutex) {
> +		ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> +					    data, len, 8);

Could this cause an ABBA deadlock during device removal?

During removal, claw_remove() acquires drvdata->cfg_mutex and then calls
device_remove_group() to remove the sysfs attributes. This blocks waiting
for kernfs to drain any active references to the sysfs files.

Meanwhile, this sysfs store method (and the show methods) acquire the exact
same drvdata->cfg_mutex internally via claw_hw_output_report().

If a sysfs attribute is being accessed concurrently when device removal occurs,
will the sysfs method block waiting for cfg_mutex while claw_remove() blocks
waiting for the sysfs method to drop its active reference?

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