Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] `speed_show()` incorrectly casts the LED class device pointer to a `hid_device`, leading to an out-of-bounds memory read and kernel crash.
- [High] Deadlock and missing return in `cfg_setup_fn` when handling initialization errors.
- [High] Sysfs attributes re-arm the cancelled delayed work after device teardown, leading to a Use-After-Free.
- [High] Racy nullification of `brightness_set` causes a TOCTOU NULL pointer dereference.
- [Medium] Communication failure in `claw_read_rgb_config` leaves the profile parser in a corrupted state.
--
commit 9663adbabcba6be0b443e7073133d650a790c871
Author: Derek J. Clark [off-list ref]
HID: hid-msi-claw: Add RGB control interface
Adds RGB control interface for MSI Claw devices with 9 total zones and
supports up to 8 sequential frames of RGB zone data.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-msi-claw.c b/drivers/hid/hid-msi-claw.c
index 60694d075d56f..f4fe74a784c29 100644
--- a/drivers/hid/hid-msi-claw.c
+++ b/drivers/hid/hid-msi-claw.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -743,6 +836,389 @@ static const struct attribute_group claw_gamepad_attr_group = {
.is_visible = claw_gamepad_attr_is_visible,
};
+/* Read RGB config from device */
+static int claw_read_rgb_config(struct hid_device *hdev)
+{
+ u8 data[4] = { 0x01, 0x00, 0x00, CLAW_RGB_FRAME_OFFSET };
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u16 read_addr = drvdata->rgb_addr;
+ size_t len = ARRAY_SIZE(data);
+ int ret, i;
+
+ if (!drvdata->rgb_addr)
+ return -ENODEV;
+
+ /* Loop through all 8 pages of RGB data */
+ for (i = 0; i < 8; i++) {
+ drvdata->profile_pending = CLAW_RGB_PENDING;
+ data[1] = (read_addr >> 8) & 0xff;
+ data[2] = read_addr & 0x00ff;
+ ret = mcu_property_out(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, len, 8);
+ if (ret)
+ return ret;
If mcu_property_out() fails here, does drvdata->profile_pending remain set
to CLAW_RGB_PENDING? If a subsequent unrelated profile event or stray ACK
arrives, could claw_profile_event() incorrectly parse its payload as an RGB
frame?
[ ... ]
quoted hunk ↗ jump to hunk
+static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
Since the rgb_attr_group attributes are attached to the LED class device
rather than the HID device, does to_hid_device(dev) result in an invalid
pointer dereference here? Other sysfs functions in this patch seem to
correctly use dev_get_drvdata(dev) to retrieve the led_classdev.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -758,6 +1234,13 @@ static void cfg_setup_fn(struct work_struct *work)
claw_remove(drvdata->hdev);
}
+ ret = claw_read_rgb_config(drvdata->hdev);
+ if (ret) {
+ dev_err(drvdata->led_mc.led_cdev.dev,
+ "Failed to setup device, can't read RGB config: %d\n", ret);
+ claw_remove(drvdata->hdev);
+ }
If claw_remove() is called from within the cfg_setup worker thread, and
claw_remove() calls cancel_delayed_work_sync(&drvdata->cfg_setup), won't
this deadlock waiting for itself to complete?
Also, since there is no return statement after claw_remove() is called in
these error blocks, will this proceed to execute the rest of the function on
a stopped device?
[ ... ]
quoted hunk ↗ jump to hunk
@@ -881,6 +1405,9 @@ static void claw_remove(struct hid_device *hdev)
if (drvdata->endpoint == CLAW_XINPUT_CFG_INTF_IN ||
drvdata->endpoint == CLAW_DINPUT_CFG_INTF_IN) {
+ /* Block writes to brightness/multi_intensity during teardown */
+ drvdata->led_mc.led_cdev.brightness_set = NULL;
Is it safe to dynamically nullify the brightness_set callback here? If a
concurrent brightness update in the LED core checks the pointer, sees it as
valid, and then this nullification happens before the actual function call,
could it result in a NULL pointer dereference?
+ cancel_delayed_work_sync(&drvdata->rgb_queue);
Since the LED class device is registered with devres
(devm_led_classdev_multicolor_register), the sysfs attributes like effect,
speed, and enabled remain accessible until after claw_remove() completes.
If userspace writes to any of those attributes in this window, could
mod_delayed_work() re-arm rgb_queue, leading to a use-after-free when
drvdata is subsequently freed?
sysfs_remove_group(&hdev->dev.kobj, &claw_gamepad_attr_group);
cancel_delayed_work_sync(&drvdata->cfg_setup);
cancel_delayed_work_sync(&drvdata->cfg_resume);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260510043510.442807-1-derekjohn.clark@gmail.com?part=3