* [PATCH] HID: synchronize input before cleaning up a failed probe
From: Yousef Alhouseen @ 2026-06-28 0:58 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, syzbot+9eebf5f6544c5e873858,
Yousef Alhouseen
hid_device_io_start() allows reports to run concurrently with probe. If
the probe subsequently fails, __hid_device_probe() releases driver
resources and clears hdev->driver without first excluding those report
callbacks.
For example, a report may enter hidraw_report_event() while the failure
path frees the associated hidraw object, leading to a use-after-free when
the report takes the object's list lock.
Stop input before performing failed-probe cleanup. This reacquires
driver_input_lock and waits for any report callback already in progress.
Fixes: c849a6143bec ("HID: Separate struct hid_device's driver_lock into two locks.")
Reported-by: syzbot+9eebf5f6544c5e873858@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9eebf5f6544c5e873858
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
drivers/hid/hid-core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 41a79e43c82b..469b849b7a2a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2835,6 +2835,8 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv)
*/
if (ret) {
+ if (hdev->io_started)
+ hid_device_io_stop(hdev);
devres_release_group(&hdev->dev, hdev->devres_group_id);
hid_close_report(hdev);
hdev->driver = NULL;
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] HID: synchronize input before cleaning up a failed probe
From: sashiko-bot @ 2026-06-28 1:14 UTC (permalink / raw)
To: Yousef Alhouseen; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260628005846.31248-1-alhouseenyousef@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-After-Free in hidraw_report_event() due to drivers calling hid_hw_stop() before hid_device_io_stop() on probe error paths.
--
commit 6c338032433c7e95cf76d26b6790e7b5f487a9b4
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>
HID: synchronize input before cleaning up a failed probe
hid_device_io_start() allows reports to run concurrently with probe. If
the probe subsequently fails, __hid_device_probe() releases driver
resources and clears hdev->driver without first excluding those report
callbacks.
Closes: https://syzkaller.appspot.com/bug?extid=9eebf5f6544c5e873858
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 41a79e43c82b3..469b849b7a2a4 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2835,6 +2835,8 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv)
> */
>
> if (ret) {
> + if (hdev->io_started)
> + hid_device_io_stop(hdev);
[Severity: High]
This is a pre-existing issue, but does this fix cover all the cases where the
use-after-free can occur?
If drivers call hid_hw_stop(hdev) in their own probe error paths before
returning an error, they might bypass the core's new hid_device_io_stop()
cleanup.
hid_hw_stop() unconditionally calls hid_disconnect(), which frees the hidraw
object. If HID_QUIRK_ALWAYS_POLL is set (or for Bluetooth devices), polling
continues, and a concurrent report could enter hidraw_report_event() with a
dangling pointer before driver_input_lock is taken.
For example, this pattern appears in nintendo_hid_probe():
drivers/hid/hid-nintendo.c:nintendo_hid_probe() {
...
err_close:
hid_hw_close(hdev);
err_stop:
hid_hw_stop(hdev);
...
}
And similarly in corsairpsu_probe():
drivers/hwmon/corsair-psu.c:corsairpsu_probe() {
...
fail_and_close:
hid_hw_close(hdev);
fail_and_stop:
hid_hw_stop(hdev);
...
}
Could these drivers also trigger the use-after-free by tearing down the
hardware and freeing hidraw while IO is still started and driver_input_lock
is unheld?
> devres_release_group(&hdev->dev, hdev->devres_group_id);
> hid_close_report(hdev);
> hdev->driver = NULL;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628005846.31248-1-alhouseenyousef@gmail.com?part=1
^ permalink raw reply
* [PATCH 0/2] Add keyboard LED support for Lenovo IdeaPad 83RR/83SR
From: Rodnei Cilto @ 2026-06-29 1:57 UTC (permalink / raw)
To: Dmitry Torokhov, Ike Panhc, Mark Pearson, Derek J. Clark,
Hans de Goede, Ilpo Järvinen
Cc: linux-input, linux-kernel, platform-driver-x86, Rodnei Cilto
This series adds support for the internal keyboard of Lenovo IdeaPad
laptops based on Wildcat Lake SoC (models 83RR worldwide and 83SR
Brazil regional variant).
The EC PS/2 emulation on these models does not fully support the AT
protocol. Sending the SETLEDS command (0xED) after initialization
causes the EC to return corrupted scancodes ('**' in i8042.debug),
rendering the keyboard non-functional.
Patch 1 adds a softleds DMI quirk to atkbd that suppresses 0xED
while keeping EV_LED capabilities visible to userspace.
Patch 2 adds physical CapsLock/NumLock LED control in ideapad-laptop
via direct EC register access (offset 0xA1) and ACPI _QDF method,
discovered via DSDT analysis.
Rodnei Cilto (2):
input: atkbd: add softleds quirk for broken EC PS/2 emulation
platform/x86: ideapad-laptop: add CapsLock/NumLock LED via EC
Signed-off-by: Rodnei Cilto <rodnei.cilto@gmail.com>
---
Rodnei Cilto (2):
input: atkbd: add softleds quirk for broken EC PS/2 emulation
platform/x86: ideapad-laptop: add CapsLock/NumLock LED via EC
drivers/input/keyboard/atkbd.c | 46 ++++++++-
drivers/input/serio/i8042-acpipnpio.h | 3 +
drivers/platform/x86/lenovo/ideapad-laptop.c | 143 +++++++++++++++++++++++++++
3 files changed, 191 insertions(+), 1 deletion(-)
---
base-commit: 03e2778d1f11de9260543f969e9e888a1c2bf830
change-id: 20260628-ideapad-kbd-leds-16f587e9ab97
Best regards,
--
Rodnei Cilto <rodnei.cilto@gmail.com>
^ permalink raw reply
* [PATCH 1/2] input: atkbd: add softleds quirk for broken EC PS/2 emulation
From: Rodnei Cilto @ 2026-06-29 1:57 UTC (permalink / raw)
To: Dmitry Torokhov, Ike Panhc, Mark Pearson, Derek J. Clark,
Hans de Goede, Ilpo Järvinen
Cc: linux-input, linux-kernel, platform-driver-x86, Rodnei Cilto
In-Reply-To: <20260628-ideapad-kbd-leds-v1-0-50b421d94927@gmail.com>
Some Lenovo IdeaPad laptops (e.g. 83RR/83SR, Wildcat Lake) implement
PS/2 keyboard emulation via the Embedded Controller (EC) but do not
fully support the AT protocol. Specifically, sending the SETLEDS
command (0xED) after initialization causes the EC to return corrupted
scancodes (reported as '**' in i8042.debug), rendering the keyboard
non-functional.
The existing SERIO_QUIRK_DUMBKBD resolves scancode corruption by
zeroing serio->write, preventing AT commands. However, LED registration
in atkbd_set_device_attrs() depends on atkbd->write being set, so
dumbkbd mode loses EV_LED capabilities entirely.
Note: serio->id.extra is __u8 (8 bits only) and cannot be used to
pass new quirk flags from i8042 to atkbd. The quirk is detected
directly in atkbd via its DMI quirk table.
Introduce atkbd_softleds: a DMI-detected mode that combines dumbkbd
behaviour (serio->write = NULL, no 0xED sent) with EV_LED registration
so that CapsLock/NumLock/ScrollLock state remains visible to userspace
via the input subsystem.
Add DMI entries for Lenovo IdeaPad 83RR (Wildcat Lake) and its Brazil
regional variant 83SR.
Signed-off-by: Rodnei Cilto <rodnei.cilto@gmail.com>
---
drivers/input/keyboard/atkbd.c | 46 ++++++++++++++++++++++++++++++++++-
drivers/input/serio/i8042-acpipnpio.h | 3 +++
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 8cb4dc6fb165..826a21dc016a 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -212,6 +212,7 @@ struct atkbd {
bool softrepeat;
bool softraw;
bool scroll;
+ bool softleds; /* suppress 0xED, register EV_LED in software */
bool enabled;
/* Accessed only from interrupt */
@@ -245,6 +246,7 @@ static unsigned int (*atkbd_platform_scancode_fixup)(struct atkbd *, unsigned in
* to many commands until full reset (ATKBD_CMD_RESET_BAT) is performed.
*/
static bool atkbd_skip_deactivate;
+static bool atkbd_softleds;
static ssize_t atkbd_attr_show_helper(struct device *dev, char *buf,
ssize_t (*handler)(struct atkbd *, char *));
@@ -600,6 +602,14 @@ static int atkbd_set_leds(struct atkbd *atkbd)
struct input_dev *dev = atkbd->dev;
u8 param[2];
+ /*
+ * softleds: EC PS/2 emulation does not support AT commands
+ * after initialization. Accept LED state from userspace but
+ * never send SETLEDS (0xED) to avoid scancode corruption.
+ */
+ if (atkbd->softleds)
+ return 0;
+
param[0] = (test_bit(LED_SCROLLL, dev->led) ? 1 : 0)
| (test_bit(LED_NUML, dev->led) ? 2 : 0)
| (test_bit(LED_CAPSL, dev->led) ? 4 : 0);
@@ -1193,7 +1203,7 @@ static void atkbd_set_device_attrs(struct atkbd *atkbd)
input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |
BIT_MASK(EV_MSC);
- if (atkbd->write) {
+ if (atkbd->write || atkbd->softleds) {
input_dev->evbit[0] |= BIT_MASK(EV_LED);
input_dev->ledbit[0] = BIT_MASK(LED_NUML) |
BIT_MASK(LED_CAPSL) | BIT_MASK(LED_SCROLLL);
@@ -1291,6 +1301,12 @@ static int atkbd_connect(struct serio *serio, struct serio_driver *drv)
if (atkbd->softrepeat)
atkbd->softraw = true;
+ if (atkbd_softleds) {
+ serio->write = NULL;
+ atkbd->write = false;
+ atkbd->softleds = true;
+ }
+
serio_set_drvdata(serio, atkbd);
err = serio_open(serio, drv);
@@ -1767,6 +1783,12 @@ static int __init atkbd_deactivate_fixup(const struct dmi_system_id *id)
return 1;
}
+static int __init atkbd_setup_softleds(const struct dmi_system_id *id)
+{
+ atkbd_softleds = true;
+ return 1;
+}
+
/*
* NOTE: do not add any more "force release" quirks to this table. The
* task of adjusting list of keys that should be "released" automatically
@@ -1938,6 +1960,28 @@ static const struct dmi_system_id atkbd_dmi_quirk_table[] __initconst = {
},
.callback = atkbd_deactivate_fixup,
},
+ {
+ /*
+ * Lenovo IdeaPad 83RR (Wildcat Lake) - EC PS/2 emulation
+ * returns corrupted scancodes ('**' in i8042.debug) when
+ * receiving AT SETLEDS (0xED) after keyboard initialization.
+ * Enable softleds mode: suppress 0xED to hardware while
+ * keeping CapsLock/NumLock/ScrollLock visible to userspace.
+ */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "83RR"),
+ },
+ .callback = atkbd_setup_softleds,
+ },
+ {
+ /* Lenovo IdeaPad 83SR (83RR Brazil regional variant) */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "83SR"),
+ },
+ .callback = atkbd_setup_softleds,
+ },
{ }
};
diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
index 8ebdf4fb9030..d233544ebac9 100644
--- a/drivers/input/serio/i8042-acpipnpio.h
+++ b/drivers/input/serio/i8042-acpipnpio.h
@@ -79,6 +79,9 @@ static inline void i8042_write_command(int val)
#define SERIO_QUIRK_DIRECT BIT(8)
#define SERIO_QUIRK_DUMBKBD BIT(9)
#define SERIO_QUIRK_NOLOOP BIT(10)
+/* SERIO_QUIRK_DUMBKBD_LEDS handled via atkbd DMI quirk table.
+ * serio->id.extra is __u8 (8 bits only), cannot carry this flag.
+ */
#define SERIO_QUIRK_NOTIMEOUT BIT(11)
#define SERIO_QUIRK_KBDRESET BIT(12)
#define SERIO_QUIRK_DRITEK BIT(13)
--
2.51.0
^ permalink raw reply related
* [PATCH 2/2] platform/x86: ideapad-laptop: add CapsLock/NumLock LED via EC
From: Rodnei Cilto @ 2026-06-29 1:57 UTC (permalink / raw)
To: Dmitry Torokhov, Ike Panhc, Mark Pearson, Derek J. Clark,
Hans de Goede, Ilpo Järvinen
Cc: linux-input, linux-kernel, platform-driver-x86, Rodnei Cilto
In-Reply-To: <20260628-ideapad-kbd-leds-v1-0-50b421d94927@gmail.com>
Some Lenovo IdeaPad laptops (e.g. 83RR/83SR, Wildcat Lake) have
physical CapsLock and NumLock LEDs controlled via the EC.
The EC exposes CAPL (bit 5) and NUML (bit 4) at offset 0xA1.
Writing these bits via ec_write() and evaluating _QDF via
acpi_evaluate_object() causes the firmware to sync EC state to the
GPIO lines that drive the physical LEDs.
Discovery via DSDT analysis on Lenovo IdeaPad 83RR (Wildcat Lake):
- CAPL/NUML at EC offset 0xA1 (bits 5 and 4)
- _QDF (_SB.PC00.LPCB.EC0._QDF) reads CAPL/NUML -> SGOV()
- GPIO 0x001A1087 -> CapsLock LED physical pin
- GPIO 0x001A0485 -> NumLock LED physical pin
- ec_read/ec_write exported via EXPORT_SYMBOL in drivers/acpi/ec.c
and declared in <linux/acpi.h>
Add two led_classdev entries (input::capslock, input::numlock)
guarded by DMI match (features.kbd_leds) for 83RR and its Brazil
regional variant 83SR.
Signed-off-by: Rodnei Cilto <rodnei.cilto@gmail.com>
---
drivers/platform/x86/lenovo/ideapad-laptop.c | 143 +++++++++++++++++++++++++++
1 file changed, 143 insertions(+)
diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
index 4fbc904f1fc3..03e3f234067c 100644
--- a/drivers/platform/x86/lenovo/ideapad-laptop.c
+++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
@@ -42,6 +42,17 @@
#include <dt-bindings/leds/common.h>
+/* EC keyboard LED control (IdeaPad EC PS/2 emulation).
+ * Validated on Lenovo IdeaPad 83RR (Wildcat Lake):
+ * EC offset 0xA1 bit4=NUML, bit5=CAPL
+ * _QDF syncs EC state to GPIO -> physical LED
+ * ec_read/ec_write declared in <linux/acpi.h>
+ */
+#define IDEAPAD_EC_KBD_LED_OFFSET 0xA1
+#define IDEAPAD_EC_KBD_LED_NUML_BIT BIT(4)
+#define IDEAPAD_EC_KBD_LED_CAPL_BIT BIT(5)
+#define IDEAPAD_ACPI_EC0_QDF_PATH "\\_SB.PC00.LPCB.EC0._QDF"
+
#define IDEAPAD_RFKILL_DEV_NUM 3
enum {
@@ -198,6 +209,7 @@ struct ideapad_private {
bool ctrl_ps2_aux_port : 1;
bool usb_charging : 1;
bool ymc_ec_trigger : 1;
+ bool kbd_leds : 1;
} features;
struct {
bool initialized;
@@ -210,6 +222,11 @@ struct ideapad_private {
struct led_classdev led;
unsigned int last_brightness;
} fn_lock;
+ struct {
+ bool initialized;
+ struct led_classdev capslock;
+ struct led_classdev numlock;
+ } kbd_leds;
};
static bool no_bt_rfkill;
@@ -1587,6 +1604,99 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv)
/*
* keyboard backlight
*/
+static int ideapad_kbd_led_ec_set(u8 bit, bool on)
+{
+ u8 val;
+ int err;
+
+ err = ec_read(IDEAPAD_EC_KBD_LED_OFFSET, &val);
+ if (err)
+ return err;
+ if (on)
+ val |= bit;
+ else
+ val &= ~bit;
+ err = ec_write(IDEAPAD_EC_KBD_LED_OFFSET, val);
+ if (err)
+ return err;
+ acpi_evaluate_object(NULL, IDEAPAD_ACPI_EC0_QDF_PATH, NULL, NULL);
+ return 0;
+}
+
+static void ideapad_capslock_led_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ ideapad_kbd_led_ec_set(IDEAPAD_EC_KBD_LED_CAPL_BIT, brightness != LED_OFF);
+}
+
+static enum led_brightness ideapad_capslock_led_get(struct led_classdev *led_cdev)
+{
+ u8 val;
+
+ if (ec_read(IDEAPAD_EC_KBD_LED_OFFSET, &val))
+ return LED_OFF;
+ return (val & IDEAPAD_EC_KBD_LED_CAPL_BIT) ? LED_ON : LED_OFF;
+}
+
+static void ideapad_numlock_led_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ ideapad_kbd_led_ec_set(IDEAPAD_EC_KBD_LED_NUML_BIT, brightness != LED_OFF);
+}
+
+static enum led_brightness ideapad_numlock_led_get(struct led_classdev *led_cdev)
+{
+ u8 val;
+
+ if (ec_read(IDEAPAD_EC_KBD_LED_OFFSET, &val))
+ return LED_OFF;
+ return (val & IDEAPAD_EC_KBD_LED_NUML_BIT) ? LED_ON : LED_OFF;
+}
+
+static int ideapad_kbd_leds_init(struct ideapad_private *priv)
+{
+ int err;
+
+ if (WARN_ON(priv->kbd_leds.initialized))
+ return -EEXIST;
+
+ priv->kbd_leds.capslock.name = "input::capslock";
+ priv->kbd_leds.capslock.max_brightness = 1;
+ priv->kbd_leds.capslock.brightness_set = ideapad_capslock_led_set;
+ priv->kbd_leds.capslock.brightness_get = ideapad_capslock_led_get;
+ priv->kbd_leds.capslock.flags = LED_RETAIN_AT_SHUTDOWN;
+
+ err = led_classdev_register(&priv->platform_device->dev,
+ &priv->kbd_leds.capslock);
+ if (err)
+ return err;
+
+ priv->kbd_leds.numlock.name = "input::numlock";
+ priv->kbd_leds.numlock.max_brightness = 1;
+ priv->kbd_leds.numlock.brightness_set = ideapad_numlock_led_set;
+ priv->kbd_leds.numlock.brightness_get = ideapad_numlock_led_get;
+ priv->kbd_leds.numlock.flags = LED_RETAIN_AT_SHUTDOWN;
+
+ err = led_classdev_register(&priv->platform_device->dev,
+ &priv->kbd_leds.numlock);
+ if (err) {
+ led_classdev_unregister(&priv->kbd_leds.capslock);
+ return err;
+ }
+
+ priv->kbd_leds.initialized = true;
+ return 0;
+}
+
+static void ideapad_kbd_leds_exit(struct ideapad_private *priv)
+{
+ if (!priv->kbd_leds.initialized)
+ return;
+ priv->kbd_leds.initialized = false;
+ led_classdev_unregister(&priv->kbd_leds.numlock);
+ led_classdev_unregister(&priv->kbd_leds.capslock);
+}
+
static int ideapad_kbd_bl_check_tristate(int type)
{
return (type == KBD_BL_TRISTATE) || (type == KBD_BL_TRISTATE_AUTO);
@@ -1832,6 +1942,29 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
priv->r_touchpad_val = value;
}
+static const struct dmi_system_id ideapad_kbd_leds_dmi_table[] = {
+ {
+ /*
+ * Lenovo IdeaPad 83RR (Wildcat Lake) - EC PS/2 emulation
+ * controls CapsLock/NumLock LEDs via EC offset 0xA1 + _QDF.
+ * CAPL=bit5 (0x20), NUML=bit4 (0x10).
+ * _QDF drives GPIO via SGOV() to physical LED pins.
+ */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "83RR"),
+ },
+ },
+ {
+ /* Lenovo IdeaPad 83SR (83RR Brazil regional variant) */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "83SR"),
+ },
+ },
+ { }
+};
+
static const struct dmi_system_id ymc_ec_trigger_quirk_dmi_table[] = {
{
/* Lenovo Yoga 7 14ARB7 */
@@ -2178,6 +2311,8 @@ static int ideapad_check_features(struct ideapad_private *priv)
priv->features.touchpad_ctrl_via_ec = touchpad_ctrl_via_ec;
priv->features.ymc_ec_trigger =
ymc_ec_trigger || dmi_check_system(ymc_ec_trigger_quirk_dmi_table);
+ priv->features.kbd_leds =
+ dmi_check_system(ideapad_kbd_leds_dmi_table);
if (!read_ec_data(handle, VPCCMD_R_FAN, &val))
priv->features.fan_mode = true;
@@ -2418,6 +2553,12 @@ static int ideapad_acpi_add(struct platform_device *pdev)
dev_info(&pdev->dev, "FnLock control not available\n");
}
+ if (priv->features.kbd_leds) {
+ err = ideapad_kbd_leds_init(priv);
+ if (err)
+ dev_warn(&pdev->dev, "Could not set up kbd LEDs: %d\n", err);
+ }
+
/*
* On some models without a hw-switch (the yoga 2 13 at least)
* VPCCMD_W_RF must be explicitly set to 1 for the wifi to work.
@@ -2477,6 +2618,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
ideapad_unregister_rfkill(priv, i);
ideapad_fn_lock_led_exit(priv);
+ ideapad_kbd_leds_exit(priv);
ideapad_kbd_bl_exit(priv);
ideapad_input_exit(priv);
@@ -2506,6 +2648,7 @@ static void ideapad_acpi_remove(struct platform_device *pdev)
ideapad_unregister_rfkill(priv, i);
ideapad_fn_lock_led_exit(priv);
+ ideapad_kbd_leds_exit(priv);
ideapad_kbd_bl_exit(priv);
ideapad_input_exit(priv);
ideapad_debugfs_exit(priv);
--
2.51.0
^ permalink raw reply related
* Re: [PATCH 2/2] platform/x86: ideapad-laptop: add CapsLock/NumLock LED via EC
From: sashiko-bot @ 2026-06-28 2:06 UTC (permalink / raw)
To: Rodnei Cilto; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260628-ideapad-kbd-leds-v1-2-50b421d94927@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sleeping in atomic context due to improper LED brightness callback assignment.
- [Medium] Unsynchronized read-modify-write on the shared EC keyboard LED register.
- [Medium] Missing default triggers for CapsLock and NumLock LEDs.
--
commit 3bd8490b486c6791548a82ecc890f8f432c92886
Author: Rodnei Cilto <rodnei.cilto@gmail.com>
platform/x86: ideapad-laptop: add CapsLock/NumLock LED via EC
This commit adds support for physical CapsLock and NumLock LEDs on specific
Lenovo IdeaPad laptops (83RR/83SR). It maps the EC's offset 0xA1 bits 5 and
4 to CAPL and NUML and exposes them as led_classdev entries.
> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
[ ... ]
> +static int ideapad_kbd_led_ec_set(u8 bit, bool on)
> +{
> + u8 val;
> + int err;
> +
> + err = ec_read(IDEAPAD_EC_KBD_LED_OFFSET, &val);
> + if (err)
> + return err;
> + if (on)
> + val |= bit;
> + else
> + val &= ~bit;
> + err = ec_write(IDEAPAD_EC_KBD_LED_OFFSET, val);
[Severity: Medium]
Is this read-modify-write sequence safe against concurrent updates?
Since CapsLock and NumLock are exposed as separate led_classdev devices, the
LED subsystem protects their state with independent locks. If both LEDs are
toggled simultaneously, could two threads concurrently enter
ideapad_kbd_led_ec_set(), read the same initial value, and write it back,
causing one modification to overwrite and lose the other?
> + if (err)
> + return err;
> + acpi_evaluate_object(NULL, IDEAPAD_ACPI_EC0_QDF_PATH, NULL, NULL);
> + return 0;
> +}
[ ... ]
> +static int ideapad_kbd_leds_init(struct ideapad_private *priv)
> +{
> + int err;
> +
> + if (WARN_ON(priv->kbd_leds.initialized))
> + return -EEXIST;
> +
> + priv->kbd_leds.capslock.name = "input::capslock";
> + priv->kbd_leds.capslock.max_brightness = 1;
> + priv->kbd_leds.capslock.brightness_set = ideapad_capslock_led_set;
[Severity: Medium]
Will this LED automatically synchronize with the system's actual keyboard lock
state? Without setting .default_trigger = "kbd-capslock", the LED subsystem
might not automatically link the keyboard input state to this LED, requiring
manual sysfs configuration.
[Severity: High]
Does this risk a scheduling while atomic panic? The LED subsystem often calls
the non-blocking brightness_set callback from atomic contexts, like softirq
for keyboard input triggers.
Since ideapad_capslock_led_set() calls ideapad_kbd_led_ec_set(), which uses
ec_read() and ec_write() that acquire mutexes and sleep, should this use
brightness_set_blocking instead to safely defer execution to a workqueue?
> + priv->kbd_leds.capslock.brightness_get = ideapad_capslock_led_get;
> + priv->kbd_leds.capslock.flags = LED_RETAIN_AT_SHUTDOWN;
> +
> + err = led_classdev_register(&priv->platform_device->dev,
> + &priv->kbd_leds.capslock);
> + if (err)
> + return err;
> +
> + priv->kbd_leds.numlock.name = "input::numlock";
> + priv->kbd_leds.numlock.max_brightness = 1;
> + priv->kbd_leds.numlock.brightness_set = ideapad_numlock_led_set;
[Severity: Medium]
Similarly, should this have .default_trigger = "kbd-numlock" set to ensure
the NumLock LED automatically tracks the physical keyboard state?
[Severity: High]
As with the CapsLock LED, does this need to use brightness_set_blocking to
avoid sleeping in atomic context when the trigger updates the LED?
> + priv->kbd_leds.numlock.brightness_get = ideapad_numlock_led_get;
> + priv->kbd_leds.numlock.flags = LED_RETAIN_AT_SHUTDOWN;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628-ideapad-kbd-leds-v1-0-50b421d94927@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH 1/2] input: atkbd: add softleds quirk for broken EC PS/2 emulation
From: sashiko-bot @ 2026-06-28 2:09 UTC (permalink / raw)
To: Rodnei Cilto; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260628-ideapad-kbd-leds-v1-1-50b421d94927@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The `atkbd` driver mutates the parent `serio` port's `write` function pointer to `NULL`, which is a driver model violation and permanently corrupts the bus state.
- [Medium] The newly added early return `if (atkbd->softleds) return 0;` inside `atkbd_set_leds()` is completely unreachable dead code.
Pre-existing issues:
- [High] A pre-existing race condition in `atkbd_disconnect()` can lead to a Use-After-Free of the `input_dev` structure in `atkbd_event_work()`.
--
commit 449f71d6f3b7e8daf08e55bc0da16d0f59475df4
Author: Rodnei Cilto <rodnei.cilto@gmail.com>
input: atkbd: add softleds quirk for broken EC PS/2 emulation
This commit introduces the atkbd_softleds mode to handle broken EC PS/2
emulation on some Lenovo IdeaPad laptops. It combines dumbkbd behaviour with
EV_LED registration to keep LED states visible to userspace without sending
the SETLEDS command. DMI entries are added for the Lenovo IdeaPad 83RR and
83SR.
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 8cb4dc6fb1658..826a21dc016ab 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -600,6 +602,14 @@ static int atkbd_set_leds(struct atkbd *atkbd)
> struct input_dev *dev = atkbd->dev;
> u8 param[2];
>
> + /*
> + * softleds: EC PS/2 emulation does not support AT commands
> + * after initialization. Accept LED state from userspace but
> + * never send SETLEDS (0xED) to avoid scancode corruption.
> + */
> + if (atkbd->softleds)
> + return 0;
> +
[Severity: Medium]
Is this early return reachable? When atkbd_softleds is active, atkbd_connect()
sets atkbd->write = false.
When an EV_LED event occurs, atkbd_event() checks atkbd->write and immediately
returns -1, dropping the event before atkbd_schedule_event_work() is called.
Does this mean atkbd_set_leds() is never executed by the workqueue in this
mode, and this check will never be triggered?
[ ... ]
> @@ -1291,6 +1301,12 @@ static int atkbd_connect(struct serio *serio, struct serio_driver *drv)
> if (atkbd->softrepeat)
> atkbd->softraw = true;
>
> + if (atkbd_softleds) {
> + serio->write = NULL;
> + atkbd->write = false;
> + atkbd->softleds = true;
> + }
> +
[Severity: High]
Does this permanently modify the parent bus provider's function pointer
(serio->write)?
If atkbd is unbound and another driver (e.g., serio_raw) binds to the same
port, or if atkbd is rebound, would the port remain crippled and unable to
transmit due to serio->write still being NULL?
[Severity: High]
This is a pre-existing issue, but can this sequence lead to a use-after-free
if a privileged user unbinds the driver while LED or repeat rate events are
being processed?
In atkbd_disconnect(), input_unregister_device() is called before
cancel_delayed_work_sync():
drivers/input/keyboard/atkbd.c:atkbd_disconnect() {
...
input_unregister_device(atkbd->dev);
/*
* Make sure we don't have a command in flight.
...
*/
cancel_delayed_work_sync(&atkbd->event_work);
...
}
If atkbd_event_work() starts executing on another CPU, it acquires
atkbd->mutex and checks atkbd->enabled. If preempted immediately after this
check, atkbd_disconnect() can proceed to set atkbd->enabled = false,
unregister and free the input device, and then block on
cancel_delayed_work_sync().
When the worker thread resumes, it calls atkbd_set_leds():
drivers/input/keyboard/atkbd.c:atkbd_event_work() {
...
if (!atkbd->enabled) {
...
} else {
if (test_and_clear_bit(ATKBD_LED_EVENT_BIT, &atkbd->event_mask))
atkbd_set_leds(atkbd);
...
}
Would atkbd_set_leds() then dereference the now-freed atkbd->dev?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628-ideapad-kbd-leds-v1-0-50b421d94927@gmail.com?part=1
^ permalink raw reply
* [git pull] Input updates for v7.2-rc0 (part 2)
From: Dmitry Torokhov @ 2026-06-28 5:11 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-input
Hi Linus,
Please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tags/input-for-v7.2-rc0-2
to receive updates for the input subsystem. You will get:
- Updates to Synaptics RMI4 driver to fix potential OOB accesses in
F30 and F3A keymap handling
- A workaround in Synaptics RMI4 to tolerate buggy firmware on some
touchpads (e.g. ThinkPad T14 Gen 1) that report incomplete register
descriptor structures, preventing probe failures
- A revert of an incorrect register descriptor address calculation in
Synaptics RMI4 driver
- A fix for a regression in HP GSC PS/2 (gscps2) driver where the
receive buffer write index was not advanced, leaving keyboard and
mouse unusable.
Changelog:
---------
Bryam Vargas (2):
Input: synaptics-rmi4 - bound the F3A keymap to the GPIO count
Input: synaptics-rmi4 - bound the F30 keymap to the GPIO/LED count
Dmitry Torokhov (2):
Revert "Input: rmi4 - fix register descriptor address calculation"
Input: rmi4 - tolerate short register descriptor structure
Xu Rao (1):
Input: gscps2 - advance receive buffer write index
Diffstat:
--------
drivers/input/rmi4/rmi_driver.c | 39 ++++++++++++++++++++++++++-------------
drivers/input/rmi4/rmi_f30.c | 2 +-
drivers/input/rmi4/rmi_f3a.c | 2 +-
drivers/input/serio/gscps2.c | 1 +
4 files changed, 29 insertions(+), 15 deletions(-)
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] input: atkbd: add softleds quirk for broken EC PS/2 emulation
From: Dmitry Torokhov @ 2026-06-28 5:20 UTC (permalink / raw)
To: Rodnei Cilto
Cc: Ike Panhc, Mark Pearson, Derek J. Clark, Hans de Goede,
Ilpo Järvinen, linux-input, linux-kernel,
platform-driver-x86
In-Reply-To: <20260628-ideapad-kbd-leds-v1-1-50b421d94927@gmail.com>
Hi Rodnei,
On Sun, Jun 28, 2026 at 10:57:03PM -0300, Rodnei Cilto wrote:
> Some Lenovo IdeaPad laptops (e.g. 83RR/83SR, Wildcat Lake) implement
> PS/2 keyboard emulation via the Embedded Controller (EC) but do not
> fully support the AT protocol. Specifically, sending the SETLEDS
> command (0xED) after initialization causes the EC to return corrupted
> scancodes (reported as '**' in i8042.debug), rendering the keyboard
> non-functional.
"**" do not represent corrupted scancodes, they are safety measure to
not disclose your password on accident.
Use i8042.debug=1 i8042.unmask_kbd_data=1 to unveil real data.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [regression] synaptics-rmi4: Failed to read the Control Register Descriptor: EIO
From: Dmitry Torokhov @ 2026-06-28 5:43 UTC (permalink / raw)
To: David Heidelberg; +Cc: linux-input, Linux Next Mailing List
In-Reply-To: <916192e4-7d2c-4463-8576-3cfe9e36faa2@ixit.cz>
On Sat, Jun 27, 2026 at 12:06:29PM +0200, David Heidelberg wrote:
> On 26/06/2026 23:18, Dmitry Torokhov wrote:
> > Hi David,
> >
> > On Fri, Jun 26, 2026 at 11:09:54PM +0200, David Heidelberg wrote:
> > > With next-20260626 without any additional patches, the synaptics fails on
> > > OnePlus 6T with:
> > >
> > > ```
> > > [ 16.620292] rmi4_i2c 12-0020: registering I2C-connected sensor
> > > [ 16.857617] rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer:
> > > Synaptics, product: S3706B, fw id: 2852315
> > > [ 16.867324] rmi4_f12 rmi4-00.fn12: Failed to read the Control Register
> > > Descriptor: -5
> > > [ 16.867338] rmi4_f12 rmi4-00.fn12: probe with driver rmi4_f12 failed with
> > > error -5
> > > ```
> >
> > Yes, at least a98518e72439 "Input: rmi4 - fix register descriptor address
> > calculation" is wrong and needs to be reverted.
>
> That did it for me, now the TS works again.
Thank you for letting me know.
--
Dmitry
^ permalink raw reply
* [PATCH] HID: hiddev: keep state alive through disconnect unlock
From: Yousef Alhouseen @ 2026-06-28 9:32 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-usb, linux-kernel, stable,
syzbot+563191a4939ddbfe73d4, Yousef Alhouseen
hiddev_disconnect() drops existancelock before freeing the hiddev state,
but a waiting final file release can run as soon as the mutex becomes
available. On PREEMPT_RT, that waiter may free hiddev while the disconnect
thread is still executing the mutex slow-unlock path, causing a
use-after-free in the mutex implementation.
Give the connection and each open file an explicit reference. Drop each
reference only after its existancelock critical section has completed, so
the state cannot be freed from the other unlock path.
Fixes: 079034073faf ("HID: hiddev cleanup -- handle all error conditions properly")
Reported-by: syzbot+563191a4939ddbfe73d4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=563191a4939ddbfe73d4
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
drivers/hid/usbhid/hiddev.c | 37 +++++++++++++++++++------------------
include/linux/hiddev.h | 2 ++
2 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 6378801b22c6..21396481995b 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -46,6 +46,12 @@ struct hiddev_list {
struct mutex thread_lock;
};
+static void hiddev_put(struct hiddev *hiddev)
+{
+ if (refcount_dec_and_test(&hiddev->refcount))
+ kfree(hiddev);
+}
+
/*
* Find a report, given the report's type and ID. The ID can be specified
* indirectly by REPORT_ID_FIRST (which returns the first report of the given
@@ -216,26 +222,21 @@ static int hiddev_fasync(int fd, struct file *file, int on)
static int hiddev_release(struct inode * inode, struct file * file)
{
struct hiddev_list *list = file->private_data;
+ struct hiddev *hiddev = list->hiddev;
unsigned long flags;
- spin_lock_irqsave(&list->hiddev->list_lock, flags);
+ spin_lock_irqsave(&hiddev->list_lock, flags);
list_del(&list->node);
- spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
+ spin_unlock_irqrestore(&hiddev->list_lock, flags);
- mutex_lock(&list->hiddev->existancelock);
- if (!--list->hiddev->open) {
- if (list->hiddev->exist) {
- hid_hw_close(list->hiddev->hid);
- hid_hw_power(list->hiddev->hid, PM_HINT_NORMAL);
- } else {
- mutex_unlock(&list->hiddev->existancelock);
- kfree(list->hiddev);
- vfree(list);
- return 0;
- }
+ mutex_lock(&hiddev->existancelock);
+ if (!--hiddev->open && hiddev->exist) {
+ hid_hw_close(hiddev->hid);
+ hid_hw_power(hiddev->hid, PM_HINT_NORMAL);
}
- mutex_unlock(&list->hiddev->existancelock);
+ mutex_unlock(&hiddev->existancelock);
+ hiddev_put(hiddev);
vfree(list);
return 0;
@@ -270,6 +271,7 @@ static int __hiddev_open(struct hiddev *hiddev, struct file *file)
spin_unlock_irq(&hiddev->list_lock);
file->private_data = list;
+ refcount_inc(&hiddev->refcount);
return 0;
@@ -897,6 +899,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
INIT_LIST_HEAD(&hiddev->list);
spin_lock_init(&hiddev->list_lock);
mutex_init(&hiddev->existancelock);
+ refcount_set(&hiddev->refcount, 1);
hid->hiddev = hiddev;
hiddev->hid = hid;
hiddev->exist = 1;
@@ -937,9 +940,7 @@ void hiddev_disconnect(struct hid_device *hid)
if (hiddev->open) {
hid_hw_close(hiddev->hid);
wake_up_interruptible(&hiddev->wait);
- mutex_unlock(&hiddev->existancelock);
- } else {
- mutex_unlock(&hiddev->existancelock);
- kfree(hiddev);
}
+ mutex_unlock(&hiddev->existancelock);
+ hiddev_put(hiddev);
}
diff --git a/include/linux/hiddev.h b/include/linux/hiddev.h
index 2164c03d2c72..8e9f8a33e359 100644
--- a/include/linux/hiddev.h
+++ b/include/linux/hiddev.h
@@ -13,6 +13,7 @@
#ifndef _HIDDEV_H
#define _HIDDEV_H
+#include <linux/refcount.h>
#include <uapi/linux/hiddev.h>
@@ -24,6 +25,7 @@ struct hiddev {
int minor;
int exist;
int open;
+ refcount_t refcount;
struct mutex existancelock;
wait_queue_head_t wait;
struct hid_device *hid;
--
2.54.0
^ permalink raw reply related
* [PATCH] HID: mcp2221: reject short input reports
From: Yousef Alhouseen @ 2026-06-28 9:36 UTC (permalink / raw)
To: Rishi Gupta, Jiri Kosina, Benjamin Tissoires
Cc: linux-i2c, linux-input, linux-kernel, stable,
syzbot+1018672fe70298606e5f, Yousef Alhouseen
The MCP2221 raw-event callback reads fixed offsets from the 64-byte input
report, including data[50] and a variable payload beginning at data[4].
Raw-event callbacks run before HID core extends short reports to their
declared size, so a malformed USB device can make these accesses run past
the received buffer.
Reject reports whose size does not match the protocol and complete the
pending command with -EMSGSIZE so its caller does not wait for a timeout.
Fixes: 67a95c21463d ("HID: mcp2221: add usb to i2c-smbus host bridge")
Reported-by: syzbot+1018672fe70298606e5f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1018672fe70298606e5f
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
drivers/hid/hid-mcp2221.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index e4ddd8e9293b..311b51c17b4e 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -861,6 +861,12 @@ static int mcp2221_raw_event(struct hid_device *hdev,
u8 *buf;
struct mcp2221 *mcp = hid_get_drvdata(hdev);
+ if (size != sizeof(mcp->txbuf)) {
+ mcp->status = -EMSGSIZE;
+ complete(&mcp->wait_in_report);
+ return 1;
+ }
+
switch (data[0]) {
case MCP2221_I2C_WR_DATA:
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] HID: mcp2221: reject short input reports
From: sashiko-bot @ 2026-06-28 9:51 UTC (permalink / raw)
To: Yousef Alhouseen; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260628093658.43445-1-alhouseenyousef@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Use-After-Free and kernel stack corruption due to un-cleared mcp->rxbuf on timeout.
- [Critical] Stack buffer overflow due to using uninitialized memory as I2C read length.
- [High] Delayed USB reports indiscriminately complete pending waits, causing state desynchronization.
- [High] Static retries variable underflow causes infinite loop of workqueue rescheduling.
--
commit 421d556dea19ceb8f40a640c782b6801d60f43cd
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>
HID: mcp2221: reject short input reports
The MCP2221 raw-event callback reads fixed offsets from the 64-byte input
report, including data[50] and a variable payload beginning at data[4].
This rejects reports whose size does not match the protocol and completes
the pending command with -EMSGSIZE.
Closes: https://syzkaller.appspot.com/bug?extid=1018672fe70298606e5f
> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index e4ddd8e9293b6..311b51c17b4e4 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -861,6 +861,12 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> u8 *buf;
> struct mcp2221 *mcp = hid_get_drvdata(hdev);
>
> + if (size != sizeof(mcp->txbuf)) {
> + mcp->status = -EMSGSIZE;
> + complete(&mcp->wait_in_report);
> + return 1;
> + }
> +
[Severity: High]
This is a pre-existing issue, but does mcp2221_raw_event() indiscriminately
complete pending waits for any incoming report without matching it to the
pending request?
If a previous request times out, and a delayed USB response arrives later,
it appears it could prematurely wake up a new command's wait queue with
incorrect status and data:
drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
...
case MCP2221_I2C_WR_DATA:
...
complete(&mcp->wait_in_report);
break;
...
}
[Severity: Critical]
This isn't a bug introduced by this patch, but can this lead to stack
corruption if a request times out?
In mcp_i2c_smbus_read(), if mcp_send_data_req_status() fails or times out,
it returns immediately without clearing mcp->rxbuf:
drivers/hid/hid-mcp2221.c:mcp_i2c_smbus_read() {
...
ret = mcp_send_data_req_status(mcp, mcp->txbuf, 4);
if (ret)
return ret;
...
}
Since mcp->rxbuf can point to a caller-provided stack buffer (like
union i2c_smbus_data), the stack frame is popped upon return. If a delayed
USB response arrives later, wouldn't mcp2221_raw_event() blindly write into
this dangling mcp->rxbuf pointer?
drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
...
if (data[2] == MCP2221_I2C_READ_COMPL ||
data[2] == MCP2221_I2C_READ_PARTIAL) {
...
buf = mcp->rxbuf;
memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);
...
}
[Severity: Critical]
This is a pre-existing issue, but does mcp_smbus_xfer() use uninitialized
stack memory as the I2C read length during a block read?
drivers/hid/hid-mcp2221.c:mcp_smbus_xfer() {
...
ret = mcp_i2c_smbus_read(mcp, NULL,
MCP2221_I2C_RD_RPT_START,
addr, data->block[0] + 1,
data->block);
...
}
The union i2c_smbus_data provided by the caller is uninitialized stack memory
on read. This configures mcp->rxbuf_size to an arbitrarily large value. Could
this lead to a massive stack buffer overflow when mcp2221_raw_event() copies
up to this value into the statically sized 34-byte data->block stack buffer?
[Severity: High]
This is a pre-existing issue, but could the statically scoped retries variable
in mcp_init_work() cause an infinite reschedule loop?
drivers/hid/hid-mcp2221.c:mcp_init_work() {
static int retries = 5;
...
reschedule_task:
...
if (!retries--)
return;
/* Device is not ready to read SRAM or FLASH data, try again */
schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
}
Since retries is static, when initialization fails 6 times, it reaches 0,
returns, and decrements to -1. On a subsequent failure, retries starts at -1.
Because !(-1) is false, it bypasses the return, decrements to -2, and
unconditionally schedules the delayed work every 100ms forever.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628093658.43445-1-alhouseenyousef@gmail.com?part=1
^ permalink raw reply
* Re: [git pull] Input updates for v7.2-rc0 (part 2)
From: pr-tracker-bot @ 2026-06-28 11:43 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Linus Torvalds, linux-kernel, linux-input
In-Reply-To: <akCs9sqkWvpJnCpK@google.com>
The pull request you sent on Sat, 27 Jun 2026 22:11:31 -0700:
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tags/input-for-v7.2-rc0-2
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/8b69c047587112f7bcb4b0d83f2729d8dd29ebe2
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* [PATCH] HID: intel-thc-hid: intel-quickspi: validate report size before copy
From: HyeongJun An @ 2026-06-28 13:37 UTC (permalink / raw)
To: Even Xu, Xinpeng Sun, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, HyeongJun An, stable
write_cmd_to_txdma() builds an output report in qsdev->report_buf, a heap
buffer allocated in quickspi_alloc_report_buf() to the device-descriptor
derived max_report_len (a few hundred bytes for a touch controller). It
copies the caller-supplied report into that buffer:
memcpy(write_buf->content, report_buf, report_buf_len);
The HID core caps a report at HID_MAX_BUFFER_SIZE (16384) by default, and
quickspi_hid_ll_driver does not set max_buffer_size, so the length reaches
the driver unbounded. A hidraw SET_REPORT/SET_FEATURE ioctl carrying a
report larger than max_report_len therefore overflows report_buf with
attacker-controlled length and content.
Record the report_buf allocation size and reject reports that do not fit
before copying, matching the equivalent guard in the intel-quicki2c
sibling (quicki2c_init_write_buf()) and the hid-goodix-spi fix.
Fixes: 9d8d51735a3a ("HID: intel-thc-hid: intel-quickspi: Add HIDSPI protocol implementation")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c | 2 ++
drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h | 1 +
drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c | 3 +++
3 files changed, 6 insertions(+)
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
index 4ae2e1718b30..1695efd5961d 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -559,6 +559,8 @@ static int quickspi_alloc_report_buf(struct quickspi_device *qsdev)
if (!qsdev->report_buf)
return -ENOMEM;
+ qsdev->report_buf_size = max_report_len;
+
return 0;
}
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
index bf5e18f5a5f4..0ed964bfe3dd 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
@@ -157,6 +157,7 @@ struct quickspi_device {
u8 *report_descriptor;
u8 *input_buf;
u8 *report_buf;
+ u32 report_buf_size;
u32 report_len;
wait_queue_head_t reset_ack_wq;
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
index cb19057f1191..db6054843e77 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
@@ -30,6 +30,9 @@ static int write_cmd_to_txdma(struct quickspi_device *qsdev,
write_buf = (struct output_report *)qsdev->report_buf;
+ if (HIDSPI_OUTPUT_REPORT_SIZE(report_buf_len) > qsdev->report_buf_size)
+ return -EINVAL;
+
write_buf->output_hdr.report_type = report_type;
write_buf->output_hdr.content_len = cpu_to_le16(report_buf_len);
write_buf->output_hdr.content_id = report_id;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] HID: intel-thc-hid: intel-quickspi: validate report size before copy
From: sashiko-bot @ 2026-06-28 13:51 UTC (permalink / raw)
To: HyeongJun An; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260628133717.941389-1-sammiee5311@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] The newly introduced bounds check rejects valid, maximum-sized output reports.
Pre-existing issues:
- [Critical] Heap buffer overflow in GET_REPORT requests due to ignored user space length parameter.
- [High] Data races and waitqueue corruption due to concurrent lockless HID requests.
- [High] Heap buffer overflow in incoming report processing due to missing buffer bounds check.
- [High] Heap buffer overflow in `read_dma_buffer()` due to 4K max packet size rounding.
--
commit c642b7d6dc654ce88acbb51328c011ab2d5e04f0
Author: HyeongJun An <sammiee5311@gmail.com>
HID: intel-thc-hid: intel-quickspi: validate report size before copy
write_cmd_to_txdma() builds an output report in qsdev->report_buf, a heap
buffer allocated in quickspi_alloc_report_buf() to the device-descriptor
derived max_report_len. This patch records the report_buf allocation size
and rejects reports that do not fit before copying.
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index 4ae2e1718b30c..1695efd5961d7 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -559,6 +559,8 @@ static int quickspi_alloc_report_buf(struct quickspi_device *qsdev)
> if (!qsdev->report_buf)
> return -ENOMEM;
>
> + qsdev->report_buf_size = max_report_len;
> +
[Severity: High]
Since this size is based on the unadjusted device descriptor maximums, will
this reject valid maximum-sized output reports?
When the driver builds an output report in write_cmd_to_txdma(), it prepends
HIDSPI_OUTPUT_REPORT_HEADER_SIZE. If a maximum-sized report is sent, the
combined size will exceed qsdev->report_buf_size and trigger the new bounds
check.
> return 0;
> }
>
[ ... ]
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> index cb19057f1191b..db6054843e777 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> @@ -30,6 +30,9 @@ static int write_cmd_to_txdma(struct quickspi_device *qsdev,
>
> write_buf = (struct output_report *)qsdev->report_buf;
>
> + if (HIDSPI_OUTPUT_REPORT_SIZE(report_buf_len) > qsdev->report_buf_size)
> + return -EINVAL;
> +
> write_buf->output_hdr.report_type = report_type;
> write_buf->output_hdr.content_len = cpu_to_le16(report_buf_len);
> write_buf->output_hdr.content_id = report_id;
>
> if (report_buf && report_buf_len > 0)
> memcpy(write_buf->content, report_buf, report_buf_len);
[Severity: High]
This is a pre-existing issue, but can multiple threads corrupt
qsdev->report_buf by executing this concurrently?
Since HID core does not serialize raw_request, multiple threads performing
concurrent HID ioctls (like HIDIOCGFEATURE and HIDIOCSFEATURE) could enter
write_cmd_to_txdma() simultaneously. Without locking, they would overwrite the
shared qsdev->report_buf with their respective headers and payloads.
This concurrent access could also corrupt the shared waitqueue state when
handling the responses:
drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c:quickspi_handle_input_data() {
...
qsdev->get_report_cmpl = true;
wake_up_interruptible(&qsdev->get_report_cmpl_wq);
...
}
[Severity: Critical]
This is a pre-existing issue, but does quickspi_get_report() overflow the
caller's buffer during GET_REPORT requests?
When a user space process issues a GET_REPORT ioctl with a smaller buffer,
hid_hw_raw_request() allocates a kernel buffer matching the requested length.
However, quickspi_hid_raw_request() discards this length parameter:
drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c:quickspi_hid_raw_request() {
...
ret = quickspi_get_report(qsdev, rtype, reportnum, buf);
...
}
Then quickspi_get_report() copies the full device-supplied response into this
potentially smaller buffer:
drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c:quickspi_get_report() {
...
memcpy(buf, qsdev->report_buf, qsdev->report_len);
...
}
[Severity: High]
This is a pre-existing issue, but does quickspi_handle_input_data() lack
bounds checking before copying incoming reports?
When a device responds to a GET_FEATURE or GET_INPUT_REPORT, the function
trusts the incoming input_len:
drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c:quickspi_handle_input_data() {
...
qsdev->report_len = sizeof(body_hdr->content_id) + input_len;
input_report = input_body->content - sizeof(body_hdr->content_id);
memcpy(qsdev->report_buf, input_report, qsdev->report_len);
...
}
If a compromised or malicious SPI device sends an artificially large input_len
that fits within the DMA transaction's packet bounds (up to 4K) but exceeds
qsdev->report_buf_size, this memcpy will overflow the strictly allocated
qsdev->report_buf.
[Severity: High]
This is a pre-existing issue, but can the 4K packet size rounding in the DMA
configuration lead to a heap overflow?
dma_set_max_packet_size() rounds up the requested DMA max packet sizes (such
as max_input_len) to a 4K boundary:
drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:dma_set_max_packet_size() {
...
dma_config->max_packet_size = ALIGN(size, SZ_4K);
...
}
When read_dma_buffer() receives data, it bounds-checks mes_len against this
4K-aligned max_packet_size before copying into the caller's buffer:
drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:read_dma_buffer() {
...
if (mes_len > read_config->max_packet_size) {
...
}
ret = sg_copy_to_buffer(sg, nent, read_buff, mes_len);
...
}
If a malicious device returns more data than max_input_len but less than the
4K boundary, the check passes. Since qsdev->input_buf was allocated without
this 4K alignment, the copy will overflow the buffer.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628133717.941389-1-sammiee5311@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH] HID: steam: reject short serial number reports
From: Yousef Alhouseen @ 2026-06-28 16:23 UTC (permalink / raw)
To: Vicki Pfau, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, stable, syzbot+75f3f9bff8c510602d36
In-Reply-To: <612b5987-1bc6-4b42-bfba-9c72ee5d51dc@endrift.com>
Hi Vicki,
That broader fix sounds preferable. Please go ahead and split it out
and submit it; I will drop this version to avoid overlapping work.
Thanks,
Yousef
On Sat, 27 Jun 2026 17:47:40 -0700, Vicki Pfau <vi@endrift.com> wrote:
> Hi Yousef,
>
> On 6/27/26 5:41 PM, Yousef Alhouseen wrote:
> > steam_recv_report() may return a short positive response and copies
> > only the bytes actually received. steam_get_serial() nevertheless reads
> > the full three-byte header and trusts its length without checking that
> > the serial payload was returned.
> >
> > A malformed USB device can therefore make the driver read uninitialized
> > stack bytes. With a complete-looking short header, those bytes can also
> > be copied into steam->serial_no and printed.
> >
> > Account for the stripped report ID in the return value and reject replies
> > that do not contain both the header and its declared payload.
> >
> > Fixes: c164d6abf384 ("HID: add driver for Valve Steam Controller")
> > Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> > ---
> > drivers/hid/hid-steam.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> > index 197126d6e081..8c8bfb10e8b8 100644
> > --- a/drivers/hid/hid-steam.c
> > +++ b/drivers/hid/hid-steam.c
> > @@ -454,11 +454,20 @@ static int steam_get_serial(struct steam_device *steam)
> > ret = steam_recv_report(steam, reply, sizeof(reply));
> > if (ret < 0)
> > goto out;
> > + /* hid_hw_raw_request() counts the stripped report ID byte. */
> > + if (ret < 4) {
> > + ret = -EIO;
> > + goto out;
> > + }
> > if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
> > reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
> > ret = -EIO;
> > goto out;
> > }
> > + if (ret - 1 < 3 + reply[1]) {
> > + ret = -EIO;
> > + goto out;
> > + }
> > reply[3 + STEAM_SERIAL_LEN] = 0;
> > strscpy(steam->serial_no, reply + 3, reply[1]);
> > out:
>
> I already have locally a patch that fixes this as part of my pending Steam Controller 2 support. However, it chooses to fix it in a different way that would affect all uses of steam_recv_report instead of per-callsite (with only one callsite fixed). I am hoping to get this patchset submitted soon, once more widescale testing is done, but if you want in the meantime I can pull out that single fix and submit it separately; it's a bit more sprawling and involves adding a new function for combined send/recv.
>
> Vicki
^ permalink raw reply
* [PATCH] HID: pxrc: reject short input reports
From: Yousef Alhouseen @ 2026-06-28 16:28 UTC (permalink / raw)
To: Marcus Folkesson, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, stable, Yousef Alhouseen
pxrc_raw_event() unconditionally reads and writes data[7], although a
malformed USB device can submit a shorter input report. The raw-event
callback runs before the HID core expands short reports to the size from
the report descriptor, so this accesses beyond the received buffer.
Ignore reports that do not contain all eight controller axes.
Fixes: acc3e34613da ("HID: Add driver for PhoenixRC Flight Controller")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
drivers/hid/hid-pxrc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-pxrc.c b/drivers/hid/hid-pxrc.c
index 71fe0c06ddcd..e3755d8b85c2 100644
--- a/drivers/hid/hid-pxrc.c
+++ b/drivers/hid/hid-pxrc.c
@@ -55,6 +55,9 @@ static int pxrc_raw_event(struct hid_device *hdev, struct hid_report *report,
{
struct pxrc_priv *priv = hid_get_drvdata(hdev);
+ if (size < 8)
+ return 0;
+
if (priv->alternate)
priv->slider = data[7];
else
--
2.54.0
^ permalink raw reply related
* [PATCH] HID: mcp2200: reject short read-all responses
From: Yousef Alhouseen @ 2026-06-28 16:30 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, stable, Yousef Alhouseen
mcp2200_raw_event() casts every READ_ALL response to the full 16-byte
response structure and reads fields through byte 10 without checking the
received report size. A malformed USB device can therefore trigger
out-of-bounds reads from the input buffer.
Complete the pending command with -EMSGSIZE when the response is short.
Fixes: 740329d7120f ("HID: mcp2200: added driver for GPIOs of MCP2200")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
drivers/hid/hid-mcp2200.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
index dafdd5b4a079..d49f3aa44448 100644
--- a/drivers/hid/hid-mcp2200.c
+++ b/drivers/hid/hid-mcp2200.c
@@ -302,6 +302,10 @@ static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
switch (data[0]) {
case READ_ALL:
all_resp = (struct mcp_read_all_resp *) data;
+ if (size < sizeof(*all_resp)) {
+ mcp->status = -EMSGSIZE;
+ break;
+ }
mcp->status = 0;
mcp->gpio_inval = all_resp->io_port_val_bmap;
mcp->baud_h = all_resp->baud_h;
--
2.54.0
^ permalink raw reply related
* [PATCH 1/4] HID: elo: ignore short touch reports
From: Yousef Alhouseen @ 2026-06-28 16:35 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Stefan Achatz, linux-input, linux-kernel, stable,
Yousef Alhouseen
elo_process_data() reads coordinates, flags, and pressure through data[7].
The raw-event callback only checks the packet marker, so a malformed USB
device can submit a one-byte marker report and trigger out-of-bounds
reads from the input buffer.
Only process touch packets that contain all eight protocol bytes.
Fixes: d23efc19478a ("HID: add driver for ELO 4000/4500")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
drivers/hid/hid-elo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-elo.c b/drivers/hid/hid-elo.c
index b8f5f3eb53a4..1aeec712c67b 100644
--- a/drivers/hid/hid-elo.c
+++ b/drivers/hid/hid-elo.c
@@ -89,7 +89,7 @@ static int elo_raw_event(struct hid_device *hdev, struct hid_report *report,
switch (report->id) {
case 0:
- if (data[0] == 'T') { /* Mandatory ELO packet marker */
+ if (size >= 8 && data[0] == 'T') { /* Mandatory ELO packet marker */
elo_process_data(hidinput->input, data, size);
return 1;
}
--
2.54.0
^ permalink raw reply related
* [PATCH 2/4] HID: cougar: reject short vendor reports
From: Yousef Alhouseen @ 2026-06-28 16:35 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Stefan Achatz, linux-input, linux-kernel, stable,
Yousef Alhouseen
In-Reply-To: <20260628163527.14279-1-alhouseenyousef@gmail.com>
cougar_raw_event() reads the key code and action from bytes one and two
of every enabled vendor-interface report. A malformed USB device can
send a shorter report and make both accesses run beyond the received
buffer.
Consume short vendor reports without parsing them.
Fixes: b8e759b8f6da ("HID: cougar: Add support for the Cougar 500k Gaming Keyboard")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
drivers/hid/hid-cougar.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
index ad027c45f162..7156658166f5 100644
--- a/drivers/hid/hid-cougar.c
+++ b/drivers/hid/hid-cougar.c
@@ -270,6 +270,9 @@ static int cougar_raw_event(struct hid_device *hdev, struct hid_report *report,
if (!shared->enabled || !shared->input)
return -EPERM;
+ if (size <= COUGAR_FIELD_ACTION)
+ return -EPERM;
+
code = data[COUGAR_FIELD_CODE];
action = data[COUGAR_FIELD_ACTION];
for (i = 0; cougar_mapping[i][0]; i++) {
--
2.54.0
^ permalink raw reply related
* [PATCH 3/4] HID: zydacron: reject short key reports
From: Yousef Alhouseen @ 2026-06-28 16:35 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Stefan Achatz, linux-input, linux-kernel, stable,
Yousef Alhouseen
In-Reply-To: <20260628163527.14279-1-alhouseenyousef@gmail.com>
The raw-event handler reads data[1] for report IDs 2 and 3 without
checking that the report includes a payload byte. A malformed USB device
can submit a report containing only the report ID and trigger an
out-of-bounds read.
Ignore the key value when the payload byte is missing.
Fixes: d0742abaa1c3 ("HID: add omitted hid-zydacron.c file")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
drivers/hid/hid-zydacron.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-zydacron.c b/drivers/hid/hid-zydacron.c
index 1aae80f848f5..b882d2a0ba1a 100644
--- a/drivers/hid/hid-zydacron.c
+++ b/drivers/hid/hid-zydacron.c
@@ -129,6 +129,9 @@ static int zc_raw_event(struct hid_device *hdev, struct hid_report *report,
switch (report->id) {
case 0x02:
case 0x03:
+ if (size < 2)
+ break;
+
switch (data[1]) {
case 0x10:
key = KEY_MODE;
--
2.54.0
^ permalink raw reply related
* [PATCH 4/4] HID: roccat-savu: reject short special reports
From: Yousef Alhouseen @ 2026-06-28 16:35 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Stefan Achatz, linux-input, linux-kernel, stable,
Yousef Alhouseen
In-Reply-To: <20260628163527.14279-1-alhouseenyousef@gmail.com>
savu_report_to_chrdev() casts special reports to a five-byte structure and
reads all of its payload fields without checking the received size. A
malformed USB device can therefore trigger out-of-bounds reads from the
input buffer when the character device is claimed.
Pass the report size into the helper and require the complete structure.
Fixes: 6a2a6390cf09 ("HID: roccat: add support for Roccat Savu")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
drivers/hid/hid-roccat-savu.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-roccat-savu.c b/drivers/hid/hid-roccat-savu.c
index 679136933560..04fa4c50cfa4 100644
--- a/drivers/hid/hid-roccat-savu.c
+++ b/drivers/hid/hid-roccat-savu.c
@@ -152,12 +152,13 @@ static void savu_remove(struct hid_device *hdev)
}
static void savu_report_to_chrdev(struct roccat_common2_device const *savu,
- u8 const *data)
+ u8 const *data, int size)
{
struct savu_roccat_report roccat_report;
struct savu_mouse_report_special const *special_report;
- if (data[0] != SAVU_MOUSE_REPORT_NUMBER_SPECIAL)
+ if (data[0] != SAVU_MOUSE_REPORT_NUMBER_SPECIAL ||
+ size < sizeof(*special_report))
return;
special_report = (struct savu_mouse_report_special const *)data;
@@ -183,7 +184,7 @@ static int savu_raw_event(struct hid_device *hdev,
return 0;
if (savu->roccat_claimed)
- savu_report_to_chrdev(savu, data);
+ savu_report_to_chrdev(savu, data, size);
return 0;
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] HID: pxrc: reject short input reports
From: sashiko-bot @ 2026-06-28 16:41 UTC (permalink / raw)
To: Yousef Alhouseen; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260628162806.10675-1-alhouseenyousef@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete fix / logic error: returning 0 for short reports does not drop the report.
--
commit 601e7ac29ed99f9b83b5ad4f7ade423528b08e79
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>
HID: pxrc: reject short input reports
This commit fixes an out-of-bounds access in pxrc_raw_event() by checking
if the incoming input report is shorter than 8 bytes and returning early.
> diff --git a/drivers/hid/hid-pxrc.c b/drivers/hid/hid-pxrc.c
> index 71fe0c06ddcdf..e3755d8b85c22 100644
> --- a/drivers/hid/hid-pxrc.c
> +++ b/drivers/hid/hid-pxrc.c
> @@ -55,6 +55,9 @@ static int pxrc_raw_event(struct hid_device *hdev, struct hid_report *report,
> {
> struct pxrc_priv *priv = hid_get_drvdata(hdev);
>
> + if (size < 8)
> + return 0;
[Severity: Medium]
Does returning 0 here achieve the goal of ignoring the report?
When the driver receives a short report and pxrc_raw_event() returns 0,
it signals success to the HID core. Looking at __hid_input_report(),
if the raw_event callback returns >= 0, the core falls through to
hid_report_raw_event(). This will zero-pad the missing bytes and dispatch
the report, which can result in erratic zero-values for the missing axes
in userspace applications.
Should this return a negative error code (like -EINVAL) to ensure the
malformed report is completely dropped by the core instead of padded?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628162806.10675-1-alhouseenyousef@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH] HID: mcp2200: reject short read-all responses
From: sashiko-bot @ 2026-06-28 16:42 UTC (permalink / raw)
To: Yousef Alhouseen; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260628163035.12212-1-alhouseenyousef@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing synchronization in `mcp_cmd_read_all` and lockless shared state access in `mcp2200_raw_event` lead to race conditions, timeouts, and data corruption.
- [High] `mcp_get` silently ignores negative error codes returned by `mcp_get_multiple`, converting hardware or communication errors into a valid 'low' state.
- [High] The driver fails to synchronize its internal state cache (`mcp->gpio_dir` and `mcp->gpio_val`) with the actual hardware configuration during probe.
- [High] Device unbind race condition leading to Use-After-Free due to improper devm_ teardown order.
- [High] The `hid_report` buffer is embedded directly within `struct mcp2200` and passed to DMA mapping functions, violating DMA cache-line alignment requirements.
--
commit 3df6b9c6c4d5763dc3316160e66ba44dfef6c606
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>
HID: mcp2200: reject short read-all responses
This commit adds a size check for READ_ALL responses in the MCP2200
driver. If the response size is smaller than the expected struct size,
it sets the status to -EMSGSIZE and aborts processing to prevent
out-of-bounds reads.
> diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
> index dafdd5b4a0797..d49f3aa44448e 100644
> --- a/drivers/hid/hid-mcp2200.c
> +++ b/drivers/hid/hid-mcp2200.c
> @@ -302,6 +302,10 @@ static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
> switch (data[0]) {
> case READ_ALL:
> all_resp = (struct mcp_read_all_resp *) data;
> + if (size < sizeof(*all_resp)) {
> + mcp->status = -EMSGSIZE;
[Severity: High]
This is a pre-existing issue, but won't this new -EMSGSIZE error be silently
swallowed by mcp_get()?
Looking at mcp_get(), it calls mcp_get_multiple() which propagates this
negative error code. However, mcp_get() discards the return value:
drivers/hid/hid-mcp2200.c:mcp_get() {
...
mcp_get_multiple(gc, &mask, &bits);
return bits > 0;
}
Since bits remains uninitialized or zeroed upon error, this converts the
-EMSGSIZE error (or any other communication failure) into a valid logical
low state.
Does this need to be handled so downstream drivers don't receive false
logical states when the device errors out?
> + break;
> + }
> mcp->status = 0;
> mcp->gpio_inval = all_resp->io_port_val_bmap;
[Severity: High]
This is a pre-existing issue, but does mcp2200_raw_event() update shared
device state locklessly?
mcp_cmd_read_all() orchestrates a transaction but drops mcp->lock before
waiting for a completion, and calls reinit_completion() unprotected:
drivers/hid/hid-mcp2200.c:mcp_cmd_read_all() {
...
reinit_completion(&mcp->wait_in_report);
mutex_lock(&mcp->lock);
...
mutex_unlock(&mcp->lock);
...
t = wait_for_completion_timeout(&mcp->wait_in_report, ...);
}
If multiple threads concurrently invoke mcp_cmd_read_all(), or unrelated
hardware reports arrive while waiting, their wait queues and completion
states can collide.
Furthermore, mcp2200_raw_event() updates mcp->status and mcp->gpio_inval
locklessly in interrupt context, and unconditionally completes the wait queue
for any unexpected report.
Should a single lock serialize the entire command-response wait cycle?
[Severity: High]
This is also a pre-existing issue, but why is the io_bmap hardware direction
state provided in READ_ALL responses completely ignored here instead of being
saved to mcp->gpio_dir?
In mcp2200_probe(), mcp->gpio_dir defaults to 0 (all outputs) without
querying the initial state. Because io_bmap is ignored here, when
mcp_set_direction() is later called to modify a specific pin, it masks
against the uninitialized local cache:
drivers/hid/hid-mcp2200.c:mcp_set_direction() {
...
conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
| (io_direction << gpio_nr);
}
Doesn't this clobber the direction of all other pins, potentially causing
electrical short-circuits by driving input pins as outputs?
> mcp->baud_h = all_resp->baud_h;
[ ... ]
Additionally, there are a couple of other pre-existing issues in this driver
that might be worth addressing:
[Severity: High]
This is a pre-existing issue, but is there a device unbind race condition
leading to a use-after-free due to the devm_ teardown order?
mcp2200_probe() uses devm_gpiochip_add_data() to register the gpiochip:
drivers/hid/hid-mcp2200.c:mcp2200_probe() {
...
ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
...
}
However, mcp2200_remove() calls hid_hw_stop(hdev) synchronously, freeing
underlying URBs:
drivers/hid/hid-mcp2200.c:mcp2200_remove() {
...
hid_hw_stop(hdev);
}
Because devm_ cleanup callbacks run after mcp2200_remove() completes, the
gpiochip remains fully active.
If userspace invokes a GPIO operation during this window, mcp_cmd_read_all()
will call hid_hw_output_report(), triggering a use-after-free on the freed
urbout.
Should the driver use manual teardown in remove() instead of devm_?
[Severity: High]
This is a pre-existing issue, but does the hid_report buffer violate DMA
cache-line alignment requirements?
The hid_report buffer is embedded directly within struct mcp2200:
drivers/hid/hid-mcp2200.c:
struct mcp2200 {
...
struct gpio_chip gc;
u8 hid_report[16];
};
This buffer is placed at the end of the struct without a
____cacheline_aligned attribute.
When hid_hw_output_report() is called, the USB HID layer receives this
buffer and maps it for DMA. On non-coherent architectures, mapping a buffer
that shares cache lines with other actively modified struct members (like gc
or status) can cause cache invalidation or flush operations to overwrite
concurrent CPU writes.
Could this buffer be allocated independently or aligned to the cacheline size?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628163035.12212-1-alhouseenyousef@gmail.com?part=1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox