Re: [PATCH v2] HID: logitech-hidpp: Add support for HID++ Multi-Platform feature (0x4531)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Destroy of `multiplatform_lock` and race with hardware teardown due to deferred sysfs cleanup.
- [Medium] The `platform` sysfs attribute is exposed before IO is re-enabled, causing writes to time out and drop packets.
- [Low] The `platform` sysfs attribute documentation incorrectly claims it supports read operations.
--

commit b016243fdd25010c24333aa504f62dd4e1905e09
Author: Baraa Atta (Dev Exalt) <exalt.dev.team@gmail.com>

HID: logitech-hidpp: Add support for HID++ Multi-Platform feature (0x4531)

Add support in the Logitech HID++ driver for the HID++ Multi-Platform
feature (0x4531), which enables HID++ devices to adjust their behavior
based on the host operating system.

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 90b0184df777f..a41fac1b4d82d 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
[ ... ]
> @@ -4423,6 +4427,379 @@ static bool hidpp_application_equals(struct hid_device *hdev,
>  	return report && report->application == application;
>  }
>  
> +/* -------------------------------------------------------------------------- */
> +/* 0x4531: Multi-Platform Support                                             */
> +/* -------------------------------------------------------------------------- */
> +
> +/*
> + * Some Logitech devices expose the HID++ feature 0x4531 (Multi-Platform) allowing
> + * the host to specify which operating system platform to use on the device. Changing device's
> + * platform may alter the behavior of the device to match the specified platform.
> + *
> + * Devices that implement this feature expose a per-device sysfs attribute
> + * "platform". Writing one of (windows|winemb|linux|chrome|android|
> + * macos|ios|webos|tizen) selects the matching platform descriptor on the device;
> + * reading reports the platform last applied through this attribute, or an empty
> + * line if none has been set.
> + */

[Severity: Low]
This isn't a bug, but the comment mentions that reading reports the platform
last applied. However, the sysfs attribute is declared as Write-Only using
DEVICE_ATTR_WO(platform) further below.

Should this block comment be updated, or was a read function intended to be
included?

[ ... ]
> +static ssize_t platform_store(struct device *dev,
> +			      struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> +	char platform[16];
> +	int idx;
> +	int ret;
> +
> +	strscpy(platform, buf, sizeof(platform));
> +	string_lower(platform, platform);
> +
> +	idx = sysfs_match_string(multiplatform_names, platform);
> +	if (idx < 0)
> +		return idx;
> +
> +	mutex_lock(&hidpp->multiplatform_lock);

[Severity: High]
Does this create a use-after-destroy regression?

Because devm_device_add_group() is used in hidpp_multiplatform_init(), the
sysfs attribute group is removed only after hidpp_remove() returns.

If hidpp_remove() destroys the multiplatform_lock via mutex_destroy(), could a
concurrent userspace write to the platform sysfs attribute attempt to lock
the destroyed mutex here?

> +	ret = hidpp_multiplatform_set_platform(hidpp, multiplatform_masks[idx]);
> +	mutex_unlock(&hidpp->multiplatform_lock);
> +	if (ret)
> +		return ret;
> +
> +	hid_dbg(hdev, "Multiplatform: Device platform set to '%s'\n",
> +		multiplatform_names[idx]);
> +
> +	return count;
> +}
[ ... ]
> @@ -4517,6 +4895,8 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
>  		connect_mask &= ~HID_CONNECT_HIDINPUT;
>  
> +	hidpp_multiplatform_init(hidpp);
> +
>  	/* Now export the actual inputs and hidraw nodes to the world */
>  	hid_device_io_stop(hdev);

[Severity: Medium]
Will writes to the sysfs attribute time out here?

By initializing the sysfs attribute group in hidpp_multiplatform_init() and
immediately stopping IO with hid_device_io_stop(), the driver creates a
window where writes from userspace are possible but IO responses are blocked.

If a write happens before hid_device_io_start() is called later in probe,
could hidpp_send_fap_command_sync() wait for a response that never arrives,
causing a -ETIMEDOUT error and dropping packets?

>  	ret = hid_connect(hdev, connect_mask);
[ ... ]
> @@ -4576,6 +4957,7 @@ static void hidpp_remove(struct hid_device *hdev)
>  	cancel_work_sync(&hidpp->work);
>  	cancel_work_sync(&hidpp->reset_hi_res_work);
>  	mutex_destroy(&hidpp->send_mutex);
> +	mutex_destroy(&hidpp->multiplatform_lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625080807.74157-1-exalt.dev.team@gmail.com?part=1