* [PATCH v5 3/4] Input: snvs_pwrkey - use local device pointer to simple code
From: joy.zou @ 2026-07-15 9:33 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260715-b4-pwrkey-v5-0-07e7353c319e@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
Use local struct device pointer to avoid reference the platform_device
pointer every time.
No functional change.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes for v4:
1. Add Reviewed-by tag.
Changes for v2:
1. Use dev instead of &pdev->dev for devm_input_allocate_device(),
which was missed in patch v1 per AI review comments.
2. Modify commit message.
---
drivers/input/keyboard/snvs_pwrkey.c | 41 ++++++++++++++++++------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index d58bbbe9fd58..cbe44a38d2b3 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -112,6 +112,7 @@ static void imx_snvs_pwrkey_act(void *pdata)
static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct pwrkey_drv_data *pdata;
struct input_dev *input;
struct device_node *np;
@@ -122,26 +123,26 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
u32 vid;
/* Get SNVS register Page */
- np = pdev->dev.of_node;
+ np = dev->of_node;
if (!np)
- return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
+ return dev_err_probe(dev, -ENODEV, "Device tree node not found\n");
- pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
if (IS_ERR(pdata->snvs))
- return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
+ return dev_err_probe(dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
pdata->keycode = KEY_POWER;
- dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
+ dev_warn(dev, "KEY_POWER without setting in dts\n");
}
- clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+ clk = devm_clk_get_optional_enabled(dev, NULL);
if (IS_ERR(clk))
- return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+ return dev_err_probe(dev, PTR_ERR(clk),
"Failed to get snvs clock (%pe)\n", clk);
pdata->wakeup = of_property_read_bool(np, "wakeup-source");
@@ -162,7 +163,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
bpt = (val / 5) - 1;
break;
default:
- return dev_err_probe(&pdev->dev, -EINVAL,
+ return dev_err_probe(dev, -EINVAL,
"power-off-time-sec %d out of range\n", val);
}
@@ -180,9 +181,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
- input = devm_input_allocate_device(&pdev->dev);
+ input = devm_input_allocate_device(dev);
if (!input)
- return dev_err_probe(&pdev->dev, -ENOMEM, "failed to allocate the input device\n");
+ return dev_err_probe(dev, -ENOMEM, "failed to allocate the input device\n");
input->name = pdev->name;
input->phys = "snvs-pwrkey/input0";
@@ -191,27 +192,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
input_set_capability(input, EV_KEY, pdata->keycode);
/* input customer action to cancel release timer */
- error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
+ error = devm_add_action(dev, imx_snvs_pwrkey_act, pdata);
if (error)
- return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
+ return dev_err_probe(dev, error, "failed to register remove action\n");
pdata->input = input;
platform_set_drvdata(pdev, pdata);
- error = devm_request_irq(&pdev->dev, pdata->irq,
- imx_snvs_pwrkey_interrupt,
- 0, pdev->name, pdev);
+ error = devm_request_irq(dev, pdata->irq,
+ imx_snvs_pwrkey_interrupt,
+ 0, pdev->name, pdev);
if (error)
- return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
+ return dev_err_probe(dev, error, "interrupt not available.\n");
error = input_register_device(input);
if (error < 0)
- return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
+ return dev_err_probe(dev, error, "failed to register input device\n");
- device_init_wakeup(&pdev->dev, pdata->wakeup);
- error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
+ device_init_wakeup(dev, pdata->wakeup);
+ error = dev_pm_set_wake_irq(dev, pdata->irq);
if (error)
- dev_err(&pdev->dev, "irq wake enable failed.\n");
+ dev_err(dev, "irq wake enable failed.\n");
return 0;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v5 4/4] Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend
From: joy.zou @ 2026-07-15 9:33 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260715-b4-pwrkey-v5-0-07e7353c319e@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
The driver implements debounce protection using a timer-based mechanism:
when a key interrupt occurs, a timer is scheduled to verify the key state
after DEBOUNCE_TIME before reporting the event. This works well during
normal operation.
However, key press events can be lost during system resume on platforms
like i.MX8MQ-EVK because:
1. During the no_irq resume phase, PCIe driver restoration can take up to
200ms with IRQs disabled.
2. The power key interrupt remains pending during the no_irq phase.
3. If the key is released before IRQs are re-enabled, the timer eventually
runs but sees the key as released and skips reporting the event.
To prevent event loss during system suspend, set a pending_press flag in
the interrupt handler and report the press event from the timer callback
when the flag is set. This avoids out-of-order event delivery and keeps
the existing timer-based debounce mechanism for normal operation.
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes for v5:
1. Replace SIMPLE_DEV_PM_OPS with DEFINE_SIMPLE_DEV_PM_OPS and remove
__maybe_unused from suspend/resume callbacks.
2. Use pm_ptr() to wrap pm_ops pointer in platform_driver.
3. Replace suspended flag check in interrupt handler with a pending_press
latch: set pending_press in hardirq context, consume and report the
press event from the timer callback in softirq context.
Changes for v3:
1. Add spinlock for pdata->keystate and pdata->suspended per AI review
comments.
2. Replace hardcode value 1 with local variable keystate in input_report_key()
under suspended.
Changes for v2:
1. Add a boolean variable suspended and PM callback functions to replace
the use of the is_suspended field per AI review comments.
2. Move event report handle to else branch in suspended state, since the
pdata->minor_rev == 0 branch has no debounce detection per AI review
comments.
3. Modify the commit message.
---
drivers/input/keyboard/snvs_pwrkey.c | 72 ++++++++++++++++++++++++++++++++++--
1 file changed, 68 insertions(+), 4 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index cbe44a38d2b3..970c7d65bd57 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -39,6 +39,9 @@ struct pwrkey_drv_data {
int keycode;
int keystate; /* 1:pressed */
int wakeup;
+ bool suspended; /* Track suspend state */
+ bool pending_press; /* Key pressed during suspend, report from timer callback */
+ spinlock_t lock; /* Protects keystate, suspended and pending_press */
struct timer_list check_timer;
struct input_dev *input;
u8 minor_rev;
@@ -49,14 +52,38 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
check_timer);
struct input_dev *input = pdata->input;
+ bool state_changed = false;
+ bool pending_press;
u32 state;
regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
state = state & SNVS_HPSR_BTN ? 1 : 0;
- /* only report new event if status changed */
- if (state ^ pdata->keystate) {
- pdata->keystate = state;
+ scoped_guard(spinlock_irqsave, &pdata->lock) {
+ pending_press = pdata->pending_press;
+ if (pending_press) {
+ pdata->pending_press = false;
+ pdata->keystate = 1;
+ }
+ /* only report new event if status changed */
+ if (state ^ pdata->keystate) {
+ pdata->keystate = state;
+ state_changed = true;
+ }
+ }
+
+ /*
+ * Report a press event latched during suspend. If the key is still
+ * held, state_changed will be 0 (keystate already set to 1 above),
+ * so no duplicate press is reported. If already released,
+ * state_changed will fire next to report the release.
+ */
+ if (pending_press) {
+ input_report_key(input, pdata->keycode, 1);
+ input_sync(input);
+ }
+
+ if (state_changed) {
input_event(input, EV_KEY, pdata->keycode, state);
input_sync(input);
pm_relax(pdata->input->dev.parent);
@@ -92,8 +119,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
input_sync(input);
pm_relax(input->dev.parent);
} else {
+ /*
+ * If the key is pressed during suspend, latch it so
+ * the timer callback can report the press event in
+ * softirq context, avoiding out-of-order events.
+ */
+ scoped_guard(spinlock_irqsave, &pdata->lock) {
+ if (pdata->suspended)
+ pdata->pending_press = true;
+ }
mod_timer(&pdata->check_timer,
- jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
+ jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
}
}
@@ -151,6 +187,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
if (pdata->irq < 0)
return pdata->irq;
+ spin_lock_init(&pdata->lock);
error = of_property_read_u32(np, "power-off-time-sec", &val);
if (!error) {
switch (val) {
@@ -217,6 +254,32 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
return 0;
}
+static int imx_snvs_pwrkey_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+
+ scoped_guard(spinlock_irqsave, &pdata->lock)
+ pdata->suspended = true;
+
+ return 0;
+}
+
+static int imx_snvs_pwrkey_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+
+ scoped_guard(spinlock_irqsave, &pdata->lock)
+ pdata->suspended = false;
+
+ return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops,
+ imx_snvs_pwrkey_suspend,
+ imx_snvs_pwrkey_resume);
+
static const struct of_device_id imx_snvs_pwrkey_ids[] = {
{ .compatible = "fsl,sec-v4.0-pwrkey" },
{ /* sentinel */ }
@@ -227,6 +290,7 @@ static struct platform_driver imx_snvs_pwrkey_driver = {
.driver = {
.name = "snvs_pwrkey",
.of_match_table = imx_snvs_pwrkey_ids,
+ .pm = pm_ptr(&imx_snvs_pwrkey_pm_ops),
},
.probe = imx_snvs_pwrkey_probe,
};
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v5 3/4] Input: snvs_pwrkey - use local device pointer to simple code
From: sashiko-bot @ 2026-07-15 9:38 UTC (permalink / raw)
To: joy.zou; +Cc: imx, dmitry.torokhov, linux-input, Frank.Li
In-Reply-To: <20260715-b4-pwrkey-v5-3-07e7353c319e@oss.nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Resource Leak / Use-After-Free due to unmanaged dev_pm_set_wake_irq()
--
--- Patch [3]: [PATCH v5 3/4] Input: snvs_pwrkey - use local device pointer to simple code ---
commit a69cd5d0da1072f6e3d82c6e22dfe1b937e29159
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - use local device pointer to simple code
This commit introduces a local device pointer in imx_snvs_pwrkey_probe() to
simplify the code. It replaces repeated references to &pdev->dev with a
single local variable.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index d58bbbe9fd58f..cbe44a38d2b3d 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -191,27 +192,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
[ ... ]
> - device_init_wakeup(&pdev->dev, pdata->wakeup);
> - error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
> + device_init_wakeup(dev, pdata->wakeup);
> + error = dev_pm_set_wake_irq(dev, pdata->irq);
[Severity: High]
This is a pre-existing issue, but does this code leak the wakeirq structure
when the driver is unbound?
Because the driver provides no remove callback or devm action to call
dev_pm_clear_wake_irq(), the allocated wake_irq structure appears to be left
attached to dev->power.wakeirq on unbind. If the driver is re-probed, could
this cause dev_pm_set_wake_irq() to fail with -EEXIST?
Also, since the underlying IRQ is managed by devm_request_irq() and freed on
unbind, if the system enters suspend after unbinding, could the PM core
attempt to call enable_irq_wake() using the stale, freed IRQ number and
cause an oops or use-after-free?
> if (error)
> - dev_err(&pdev->dev, "irq wake enable failed.\n");
> + dev_err(dev, "irq wake enable failed.\n");
>
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-b4-pwrkey-v5-0-07e7353c319e@oss.nxp.com?part=3
^ permalink raw reply
* Re: [PATCH v5 2/4] Input: snvs_pwrkey - propagate error code of platform_get_irq()
From: sashiko-bot @ 2026-07-15 9:42 UTC (permalink / raw)
To: joy.zou; +Cc: linux-input, Frank.Li, dmitry.torokhov, imx
In-Reply-To: <20260715-b4-pwrkey-v5-2-07e7353c319e@oss.nxp.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The driver uses `dev_pm_set_wake_irq()` and `device_init_wakeup()` during probe but provides no `.remove` callback to clean them up, causing a resource leak and permanently breaking driver rebind.
- [High] The driver ignores the return value of `regmap_read()`, leading to the use of uninitialized stack variables which can trigger spurious system shutdowns.
--
--- Patch [2]: [PATCH v5 2/4] Input: snvs_pwrkey - propagate error code of platform_get_irq() ---
commit 353345395cd42cd8e38b32a7bbc24dfad430ba99
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - propagate error code of platform_get_irq()
This commit updates the driver to propagate the actual error code from
platform_get_irq() instead of hardcoding -EINVAL. This ensures probe
deferral and other diagnostic information are preserved.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 8cc6863d26ed8..d58bbbe9fd58f 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -148,7 +148,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>
> pdata->irq = platform_get_irq(pdev, 0);
> if (pdata->irq < 0)
> - return -EINVAL;
> + return pdata->irq;
>
> error = of_property_read_u32(np, "power-off-time-sec", &val);
> if (!error) {
[Severity: High]
This is a pre-existing issue, but does this driver leak the wake IRQ upon
unbind?
Looking at imx_snvs_pwrkey_probe() in drivers/input/keyboard/snvs_pwrkey.c,
the driver sets up a wake IRQ:
error = dev_pm_set_wake_irq(dev, pdata->irq);
if (error)
dev_err(dev, "irq wake enable failed.\n");
However, there is no corresponding .remove callback in the platform_driver
struct (imx_snvs_pwrkey_driver) to call dev_pm_clear_wake_irq() or
device_init_wakeup(..., false):
static struct platform_driver imx_snvs_pwrkey_driver = {
.driver = {
.name = "snvs_pwrkey",
.of_match_table = imx_snvs_pwrkey_ids,
},
.probe = imx_snvs_pwrkey_probe,
};
If the driver is unbound via sysfs and rebound, does this leave a dangling
wakeup source and cause the subsequent bind to fail with -EEXIST?
[Severity: High]
This is a pre-existing issue, but can unhandled regmap_read() errors cause
spurious system shutdowns?
In drivers/input/keyboard/snvs_pwrkey.c, imx_snvs_pwrkey_interrupt() reads
the status register without checking the return value:
regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
if (lp_status & SNVS_LPSR_SPO) {
A similar pattern exists in imx_imx_snvs_check_for_events():
regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
state = state & SNVS_HPSR_BTN ? 1 : 0;
And also in imx_snvs_pwrkey_probe():
regmap_read(pdata->snvs, SNVS_HPVIDR1_REG, &vid);
pdata->minor_rev = vid & 0xff;
If regmap_read() fails due to a clock glitch or bus fault, won't these
stack variables remain uninitialized? Could an uninitialized variable
randomly evaluate to true and be interpreted as a button press,
immediately propagating a spurious power key event?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-b4-pwrkey-v5-0-07e7353c319e@oss.nxp.com?part=2
^ permalink raw reply
* Re: [PATCH v5 4/4] Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend
From: sashiko-bot @ 2026-07-15 9:46 UTC (permalink / raw)
To: joy.zou; +Cc: imx, dmitry.torokhov, linux-input, Frank.Li
In-Reply-To: <20260715-b4-pwrkey-v5-4-07e7353c319e@oss.nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing cleanup for wake IRQ leads to memory leak and device re-bind failure.
--
--- Patch [4]: [PATCH v5 4/4] Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend ---
commit 0a87e611a13956d492552759681ef44d587017b8
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend
This commit introduces a pending_press flag and a spinlock to handle power
key events during system resume phases where IRQs might be disabled. It
ensures that if the key is released before IRQs are re-enabled, a press
event is still reported via the timer callback.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -217,6 +254,32 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> return 0;
> }
[Severity: High]
This is a pre-existing issue, but I noticed the wake IRQ setup just prior to
this return in imx_snvs_pwrkey_probe() lacks a cleanup path:
error = dev_pm_set_wake_irq(dev, pdata->irq);
Since there is no .remove callback or manual cleanup, does this leak the
struct wake_irq memory when the device is unbound?
Additionally, since dev->power.wakeirq is never cleared, would re-binding
the driver trigger the dev_WARN_ONCE ("wake irq already initialized") in
dev_pm_attach_wake_irq(), potentially causing a system panic if
panic_on_warn is enabled?
Could this be resolved by switching to the managed devm_pm_set_wake_irq()
helper?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-b4-pwrkey-v5-0-07e7353c319e@oss.nxp.com?part=4
^ permalink raw reply
* [PATCH] HID: amd_sfh: clear hid_sensor_hubs entry on probe failure
From: Chen Changcheng @ 2026-07-15 9:46 UTC (permalink / raw)
To: basavaraj.natikar, jikos, bentiss
Cc: linux-input, linux-kernel, Chen Changcheng
In amdtp_hid_probe(), the newly allocated HID device is stored in
cli_data->hid_sensor_hubs[cur_hid_dev] before calling hid_add_device().
If hid_add_device() fails, the error path frees the HID device and its
driver_data but does not clear the array entry, leaving a dangling
pointer.
When the caller (amd_sfh_hid_client_init or
amd_sfh1_1_hid_client_init) detects the probe failure, it jumps to its
cleanup label, which unconditionally calls amd_sfh_hid_client_deinit()
and subsequently amdtp_hid_remove(). The latter iterates over all
hid_sensor_hubs[] entries and, upon encountering the non-NULL but freed
pointer, performs a use-after-free read followed by double-free of both
the HID device and its driver_data.
Clear the array entry in the error path of amdtp_hid_probe() so that
amdtp_hid_remove() skips the failed entry.
Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
---
drivers/hid/amd-sfh-hid/amd_sfh_hid.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
index b04f675d49b0..9eaa2785a9ac 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
@@ -162,6 +162,7 @@ int amdtp_hid_probe(u32 cur_hid_dev, struct amdtp_cl_data *cli_data)
return 0;
err_hid_device:
+ cli_data->hid_sensor_hubs[cur_hid_dev] = NULL;
kfree(hid_data);
err_hid_data:
hid_destroy_device(hid);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] HID: amd_sfh: clear hid_sensor_hubs entry on probe failure
From: sashiko-bot @ 2026-07-15 9:58 UTC (permalink / raw)
To: Chen Changcheng; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260715094622.115062-1-chenchangcheng@kylinos.cn>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Improper teardown ordering allows delayed work to be scheduled after cancellation, causing a Use-After-Free.
- [Medium] Unprocessed requests in `req_list` are leaked during driver teardown.
--
--- Patch [1]: [PATCH] HID: amd_sfh: clear hid_sensor_hubs entry on probe failure ---
commit 610c0ee48e98cb215b641a8b6eb6b1d6375db3cc
Author: Chen Changcheng <chenchangcheng@kylinos.cn>
HID: amd_sfh: clear hid_sensor_hubs entry on probe failure
This patch clears the hid_sensor_hubs entry in the error path of
amdtp_hid_probe(). Doing this prevents a dangling pointer that can
trigger a use-after-free and double-free during cleanup when
amdtp_hid_remove() iterates over the array.
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> index b04f675d49b0d..9eaa2785a9aca 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> @@ -162,6 +162,7 @@ int amdtp_hid_probe(u32 cur_hid_dev, struct amdtp_cl_data *cli_data)
> return 0;
>
> err_hid_device:
> + cli_data->hid_sensor_hubs[cur_hid_dev] = NULL;
> kfree(hid_data);
> err_hid_data:
> hid_destroy_device(hid);
[Severity: High]
This is a pre-existing issue, but is it possible for delayed work to be
scheduled after cancellation during driver teardown?
Looking at amd_sfh_hid_client_deinit() in
drivers/hid/amd-sfh-hid/amd_sfh_client.c:
cancel_delayed_work_sync(&cl_data->work);
cancel_delayed_work_sync(&cl_data->work_buffer);
amdtp_hid_remove(cl_data);
Because amdtp_hid_remove() is called after the workqueues are canceled, the
HID interfaces remain fully operational during this window.
Could an unprivileged user concurrently issue a hidraw ioctl, which would call
amd_sfh_get_report() and re-arm the delayed work?
If this happens, the delayed work could run after the driver structures are
freed or destroyed, potentially leading to a use-after-free.
[Severity: Medium]
This is also a pre-existing issue, but does driver teardown leak unprocessed
request_list nodes?
In drivers/hid/amd-sfh-hid/amd_sfh_client.c, amd_sfh_get_report() dynamically
allocates new request_list nodes and appends them to cli_data->req_list:
struct request_list *new = kzalloc_obj(*new);
if (!new)
return -ENOMEM;
...
list_add(&new->list, &req_list->list);
These nodes are normally freed by the delayed work function amd_sfh_work().
However, when amd_sfh_hid_client_deinit() cancels the work, it does not appear
to iterate over req_list to free any pending requests.
If there are unprocessed requests remaining in the list at the time of unbind,
will these dynamically allocated list nodes be permanently leaked?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715094622.115062-1-chenchangcheng@kylinos.cn?part=1
^ permalink raw reply
* Re: [PATCH v12 2/4] HID: hid-msi: Add M-key mapping attributes
From: Derek John Clark @ 2026-07-15 11:11 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260714104459.05A741F000E9@smtp.kernel.org>
On Tue, Jul 14, 2026 at 3:44 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Non-atomic update of drvdata->profile_pending can overwrite concurrent state changes, leading to sysfs read failures.
> - [High] Orphan ACKs from SYNC_TO_ROM commands are ignored and leave the `orphan_ack_pending` flag set if they arrive while waiting for a READ command, causing the next WRITE command to time out.
> --
>
> --- Patch [2]: [PATCH v12 2/4] HID: hid-msi: Add M-key mapping attributes ---
> commit 21a823af0d7845d277f06cccaf5818c1b1be1ce7
> 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.
>
> Link: https://patch.msgid.link/20260529072111.7565-3-derekjohn.clark@gmail.com
>
> > diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> > index 8a48ce099c1ca..9d3249fa50291 100644
> > --- a/drivers/hid/hid-msi.c
> > +++ b/drivers/hid/hid-msi.c
>
> [ ... ]
>
> > @@ -143,6 +327,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_irqsave, &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 -EAGAIN;
> > + codes = (profile == CLAW_M1_PENDING) ? drvdata->m1_codes : drvdata->m2_codes;
> > + for (i = 0; i < CLAW_KEYS_MAX; i++)
> > + codes[i] = (mkeys->codes[i]);
> > + break;
> > + default:
> > + dev_dbg(&drvdata->hdev->dev,
> > + "Got profile event without changes pending from command: %x\n",
> > + cmd_rep->cmd);
> > + return -EINVAL;
> > + }
> > + scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
> > + drvdata->profile_pending = CLAW_NO_PENDING;
>
> [Severity: High]
> Can this unconditionally clearing profile_pending overwrite concurrent
> state changes?
>
> If Thread A times out waiting for M1, and Thread B starts waiting for M2
> by setting profile_pending = CLAW_M2_PENDING. If the delayed USB ACK
> for M1 then arrives, claw_profile_event processes it and executes:
>
> scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
> drvdata->profile_pending = CLAW_NO_PENDING;
>
> Doesn't this clobber Thread B's pending M2 state, causing the M2 ACK to
> be rejected and Thread B to permanently time out?
>
This is not a possible scenario in v12. If M1 ACK is received after
profile_pending is set to CLAW_M2_PENDING it will first be checked
against the bmap_addr for the M2 button. When this check fails it will
return -EAGAIN which will soft break from the event, keeping the M2
ACK primed. If M1 was requested twice and the first times out then
this scenario could happen ass the address would match, but in that
case we're already reporting an error to dmesg and the device is
behaving out of specification.
> > +
> > + return 0;
> > +}
> > +
> > static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
> > u8 *data, int size)
> > {
>
> [ ... ]
>
> > @@ -171,9 +388,23 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
> > complete(&drvdata->send_cmd_complete);
> > }
> >
> > + break;
> > + case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> > + ret = claw_profile_event(drvdata, cmd_rep);
> > + /* Stale address received, ignore and keep waiting */
> > + if (ret == -EAGAIN)
> > + return 0;
> > + 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;
> > case CLAW_COMMAND_TYPE_ACK:
> > - if (drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE)
> > + if (drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_PROFILE ||
> > + drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE)
> > break;
> >
> > if (drvdata->orphan_ack_pending) {
>
> [Severity: High]
> Does this early break for READ commands permanently skip processing
> the orphan ACK?
>
> If a WRITE_PROFILE_DATA command is sent followed by a SYNC_TO_ROM
> command, orphan_ack_pending is set to true. If userspace then
> immediately issues a sysfs read, setting waiting_cmd to
> CLAW_COMMAND_TYPE_READ_PROFILE, and the generic orphan ACK arrives:
>
> claw_raw_event() will evaluate the condition above, execute the early
> break, and bypass clearing orphan_ack_pending.
>
> Later, when another write is issued, won't its real generic ACK be
> incorrectly consumed as the leftover orphan ACK, causing the new write
> to time out since its completion is never signaled?
This finding is valid. I'll refactor how this is handled to simplify
it and add an 8 or 25ms fsleep to __claw_hw_output_report() if an
orphan ACK is pending, which should provide time for the ACK to
arrive.
- Derek
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260714102640.18075-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v3 1/3] dt-bindings: input: Add Qualcomm SPMI PMIC haptics
From: Krzysztof Kozlowski @ 2026-07-15 11:17 UTC (permalink / raw)
To: Fenglin Wu
Cc: linux-arm-msm, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Stephen Boyd, Bjorn Andersson,
Konrad Dybcio, David Collins, Subbaraman Narayanamurthy,
Kamal Wadhwa, kernel, linux-input, devicetree, linux-kernel
In-Reply-To: <20260713-qcom-spmi-haptics-v3-1-c931bb7cb94f@oss.qualcomm.com>
On Mon, Jul 13, 2026 at 10:22:54PM -0700, Fenglin Wu wrote:
> Add binding document for the haptics module inside Qualcomm PMIC
> PMIH0108.
>
> Assisted-by: Claude:claude-4-8-opus
> Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
> ---
> .../bindings/input/qcom,spmi-haptics.yaml | 136 +++++++++++++++++++++
> 1 file changed, 136 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/qcom,spmi-haptics.yaml b/Documentation/devicetree/bindings/input/qcom,spmi-haptics.yaml
> new file mode 100644
> index 000000000000..b1a69e9886ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/qcom,spmi-haptics.yaml
> @@ -0,0 +1,136 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 2/3] dt-bindings: mfd: qcom,spmi-pmic: Document haptics device
From: Krzysztof Kozlowski @ 2026-07-15 11:17 UTC (permalink / raw)
To: Fenglin Wu
Cc: linux-arm-msm, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Stephen Boyd, Bjorn Andersson,
Konrad Dybcio, David Collins, Subbaraman Narayanamurthy,
Kamal Wadhwa, kernel, linux-input, devicetree, linux-kernel
In-Reply-To: <20260713-qcom-spmi-haptics-v3-2-c931bb7cb94f@oss.qualcomm.com>
On Mon, Jul 13, 2026 at 10:22:55PM -0700, Fenglin Wu wrote:
> Some of the Qualcomm SPMI PMIC has haptics device in it, add it in the
> device list.
>
> Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
> ---
> Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH] HID: nintendo: fix out-of-bounds read in joycon_ctlr_read_handler()
From: Ibrahim Hashimov @ 2026-07-15 11:52 UTC (permalink / raw)
To: jikos, bentiss; +Cc: linux-input, linux-kernel, stable
joycon_ctlr_read_handler() casts an incoming HID input report to
struct joycon_input_report and parses it, guarding the cast only with a
12-byte length check:
if (size >= 12) /* make sure it contains the input report */
joycon_parse_report(ctlr, (struct joycon_input_report *)data);
struct joycon_input_report is 49 bytes: a 13-byte header followed by a
union whose IMU arm is 36 bytes. For an IMU report joycon_parse_report()
-> joycon_parse_imu_report() walks that union (struct offsets 13..48),
so a report of exactly 12 bytes with data[0] == JC_INPUT_IMU_DATA passes
the guard yet is read up to 37 bytes past its declared length. The
over-read bytes are decoded into accelerometer/gyroscope values and
forwarded to userspace through the "(IMU)" input device, leaking
driver-internal memory. data[0] and size are fully controlled by a
malicious or spoofed Joy-Con/Pro Controller.
Receive buffers are sized to the maximum report length, so this is an
over-read within the allocation rather than a slab OOB, but the decoded
bytes still reach userspace.
The sibling subcmd path in joycon_ctlr_handle_event() already bounds the
same cast correctly:
if (size < sizeof(struct joycon_input_report) ||
data[0] != JC_INPUT_SUBCMD_REPLY)
break;
Use the same sizeof(struct joycon_input_report) bound here.
Fixes: 2af16c1f846b ("HID: nintendo: add nintendo switch controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Ibrahim Hashimov <security@auditcode.ai>
Assisted-by: AuditCode-AI:2026.07
---
drivers/hid/hid-nintendo.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index e7302ec01ff1..11b5fe05acf4 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -2607,7 +2607,12 @@ static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
{
if (data[0] == JC_INPUT_SUBCMD_REPLY || data[0] == JC_INPUT_IMU_DATA ||
data[0] == JC_INPUT_MCU_DATA) {
- if (size >= 12) /* make sure it contains the input report */
+ /*
+ * The whole struct is cast and parsed below, including the
+ * IMU/subcmd union, not just the 12-byte partial header this
+ * used to check for.
+ */
+ if (size >= sizeof(struct joycon_input_report))
joycon_parse_report(ctlr,
(struct joycon_input_report *)data);
}
--
2.50.1 (Apple Git-155)
^ permalink raw reply related
* [PATCH] HID: picolcd: clamp eeprom debugfs read to bytes actually received
From: Ibrahim Hashimov @ 2026-07-15 11:53 UTC (permalink / raw)
To: jikos, bentiss; +Cc: linux-input, linux-kernel, stable
picolcd_debug_eeprom_read() trusts resp->raw_data[2] -- a length byte
supplied by the device in its REPORT_EE_DATA reply -- clamped only to
the caller's read() count:
ret = resp->raw_data[2];
if (ret > s)
ret = s;
if (copy_to_user(u, resp->raw_data+3, ret))
It never checks resp->raw_size, the number of bytes picolcd_raw_event()
actually copied into the 64-byte raw_data[] of the kmalloc'd struct
picolcd_pending. A device (or a spoofed picoLCD) returning a length byte
of 0xff, read with a count >= 255, makes copy_to_user() read past
raw_data[] into adjacent slab memory and return it to userspace through
the debugfs "eeprom" file:
BUG: KASAN: slab-out-of-bounds in _copy_to_user
Read of size 255 ... picolcd_debug_eeprom_read+0x214/0x2f0 [hid_picolcd]
The debug-dump path in the same file already validates the device length
byte against the received size before trusting it; this read does not.
The file is created S_IRUSR (root-only) and a crafted device is needed,
so it is neither unprivileged- nor remotely-triggerable.
Clamp the copy length to resp->raw_size - 3 (the payload actually
received, minus the 3-byte header), floored at 0 for short replies.
Fixes: 9bbf2b98ba11 ("HID: add experimental access to PicoLCD device's EEPROM and FLASH")
Cc: stable@vger.kernel.org
Signed-off-by: Ibrahim Hashimov <security@auditcode.ai>
Assisted-by: AuditCode-AI:2026.07
---
drivers/hid/hid-picolcd_debugfs.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/hid/hid-picolcd_debugfs.c b/drivers/hid/hid-picolcd_debugfs.c
index 085847a92e07..1f7dfe60e9ba 100644
--- a/drivers/hid/hid-picolcd_debugfs.c
+++ b/drivers/hid/hid-picolcd_debugfs.c
@@ -99,6 +99,15 @@ static ssize_t picolcd_debug_eeprom_read(struct file *f, char __user *u,
ret = resp->raw_data[2];
if (ret > s)
ret = s;
+ /*
+ * raw_data[2] is a device-supplied length; also clamp it to
+ * what picolcd_raw_event() actually stored (raw_size), or a
+ * hostile device overruns the raw_data[] buffer.
+ */
+ if (ret > resp->raw_size - 3)
+ ret = resp->raw_size - 3;
+ if (ret < 0)
+ ret = 0;
if (copy_to_user(u, resp->raw_data+3, ret))
ret = -EFAULT;
else
--
2.50.1 (Apple Git-155)
^ permalink raw reply related
* Re: [PATCH] HID: picolcd: clamp eeprom debugfs read to bytes actually received
From: sashiko-bot @ 2026-07-15 12:05 UTC (permalink / raw)
To: Ibrahim Hashimov; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260715115301.91063-1-security@auditcode.ai>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-After-Free on `data->rc_dev` and `data->input_keys` due to race in probe error path.
--
--- Patch [1]: [PATCH] HID: picolcd: clamp eeprom debugfs read to bytes actually received ---
commit 16232239549016339287f47b37b440446a47d534
Author: Ibrahim Hashimov <security@auditcode.ai>
HID: picolcd: clamp eeprom debugfs read to bytes actually received
This commit fixes an out-of-bounds slab read in picolcd_debug_eeprom_read()
by correctly clamping the copy length to the received payload size.
Cc: stable@vger.kernel.org
> diff --git a/drivers/hid/hid-picolcd_debugfs.c b/drivers/hid/hid-picolcd_debugfs.c
> index 085847a92e07d..1f7dfe60e9ba2 100644
> --- a/drivers/hid/hid-picolcd_debugfs.c
> +++ b/drivers/hid/hid-picolcd_debugfs.c
[ ... ]
[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free in the
probe error path.
If a USB device continuously sends interrupt IN packets to trigger
picolcd_raw_event() while intentionally failing device initialization, could
this cause a race condition?
In drivers/hid/hid-picolcd_core.c:picolcd_probe_lcd(), the probe error
handler destroys the cir and keys objects while the HID device is still open:
err:
picolcd_exit_leds(data);
picolcd_exit_framebuffer(data);
picolcd_exit_backlight(data);
picolcd_exit_lcd(data);
picolcd_exit_cir(data);
picolcd_exit_keys(data);
return error;
Because hid_hw_close() isn't called until later, picolcd_raw_event() can
concurrently dereference the freed data->rc_dev pointer outside of the
spinlock in drivers/hid/hid-picolcd_cir.c:picolcd_raw_cir():
spin_lock_irqsave(&data->lock, flags);
if (!data->rc_dev || (data->status & PICOLCD_CIR_SHUN)) {
spin_unlock_irqrestore(&data->lock, flags);
return 1;
}
spin_unlock_irqrestore(&data->lock, flags);
// ... [IR data processing] ...
ir_raw_event_store(data->rc_dev, &rawir);
The same timing issue appears to apply to data->input_keys, which is accessed
locklessly in drivers/hid/hid-picolcd_core.c:picolcd_raw_keypad():
input_event(data->input_keys, EV_MSC, MSC_SCAN, raw_data[i]);
if (raw_data[i] < PICOLCD_KEYS)
key_code = data->keycode[raw_data[i]];
Could this allow concurrent events to use these pointers after they are freed
by picolcd_exit_cir() and picolcd_exit_keys()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715115301.91063-1-security@auditcode.ai?part=1
^ permalink raw reply
* Re: [PATCH] HID: nintendo: fix out-of-bounds read in joycon_ctlr_read_handler()
From: sashiko-bot @ 2026-07-15 12:15 UTC (permalink / raw)
To: Ibrahim Hashimov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715115253.91029-1-security@auditcode.ai>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The input device is registered before its capabilities are fully initialized.
--
--- Patch [1]: [PATCH] HID: nintendo: fix out-of-bounds read in joycon_ctlr_read_handler() ---
commit 4a13815d9787a8e4509f8221af788267db0c1886
Author: Ibrahim Hashimov <security@auditcode.ai>
HID: nintendo: fix out-of-bounds read in joycon_ctlr_read_handler()
This commit changes a bounds check in joycon_ctlr_read_handler() from
12 bytes to the full size of struct joycon_input_report. This prevents
an out-of-bounds read when decoding IMU reports that are smaller than
the full structure size.
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index e7302ec01ff1f..11b5fe05acf44 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2607,7 +2607,12 @@ static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
> {
> if (data[0] == JC_INPUT_SUBCMD_REPLY || data[0] == JC_INPUT_IMU_DATA ||
> data[0] == JC_INPUT_MCU_DATA) {
> - if (size >= 12) /* make sure it contains the input report */
> + /*
> + * The whole struct is cast and parsed below, including the
> + * IMU/subcmd union, not just the 12-byte partial header this
> + * used to check for.
> + */
> + if (size >= sizeof(struct joycon_input_report))
> joycon_parse_report(ctlr,
> (struct joycon_input_report *)data);
> }
While reviewing this file, I noticed a pre-existing issue in
joycon_input_create():
drivers/hid/hid-nintendo.c:joycon_input_create() {
[ ... ]
ctlr->input->phys = hdev->phys;
input_set_drvdata(ctlr->input, ctlr);
ret = input_register_device(ctlr->input);
if (ret)
return ret;
if (joycon_type_is_right_joycon(ctlr)) {
joycon_config_right_stick(ctlr->input);
[ ... ]
}
[Severity: High]
This is a pre-existing issue, but is the input device registered here before
its capabilities are configured?
Calling input_register_device() before setting the keys and absolute axes
results in an empty capability map during registration. This causes the joydev
handler to reject the device since EV_KEY and EV_ABS are not set.
Could this lead to udev exporting empty capabilities, and evdev calculating
an incorrect event buffer size, causing applications to fail to reliably
recognize the controller?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715115253.91029-1-security@auditcode.ai?part=1
^ permalink raw reply
* Re: [PATCH v5] HID: logitech-hidpp: Add support for HID++ Multi-Platform feature (0x4531)
From: dev exalt @ 2026-07-15 14:01 UTC (permalink / raw)
To: jikos, bentiss
Cc: lains, hadess, linux-input, linux-kernel, sari.kreitem, hbarnor
In-Reply-To: <20260701073727.53636-1-exalt.dev.team@gmail.com>
Dear maintainers,
We would like to kindly follow up on the latest revision of this
patch, as we have not yet received any feedback.
This patch is the latest revision of the work previously discussed here:
https://lore.kernel.org/linux-input/20251215125319.33261-1-exalt.dev.team@gmail.com/
This version incorporates the feedback and suggestions from the
previous discussion and updates the implementation accordingly.
We understand that maintainers are busy, so we are just sending a
gentle reminder in case this patch has not yet had a chance to be
reviewed.
Thank you very much for your time and review.
Best regards,
Baraa Atta
On Wed, Jul 1, 2026 at 10:37 AM DevExalt <exalt.dev.team@gmail.com> wrote:
>
> From: "Baraa Atta (Dev Exalt)" <exalt.dev.team@gmail.com>
>
> 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.
>
> This patch:
> * Adds device IDs for MX Keys S (046d) and Casa Keys (046d).
> * Introduces the per-device sysfs attribute "platform" to allow selecting
> a target platform.
> * Detects whether a device implements feature 0x4531.
> * Validates that the requested platform is supported by the device.
> * Applies the selected platform when valid.
> * Leaves the device unchanged when an unsupported platform is requested.
>
> Supported values for the platform sysfs attribute:
>
> windows, winemb, linux, chrome, android,
> macos, ios, webos, tizen
>
> TEST=Pair MX Keys S and Casa Keys over Bluetooth and verify:
> * Feature 0x4531 is detected.
> * Valid platform values written through sysfs are accepted and applied.
> * Invalid platform values result in no update.
> * Devices without 0x4531 retain default behavior.
> * Platform-specific key behavior is observed once applied.
>
> Signed-off-by: Baraa Atta (Dev Exalt) <exalt.dev.team@gmail.com>
> ---
> Changes in v2:
> * Replace the global hidpp_platform module parameter with a per-device
> sysfs attribute
> * Expose all platforms supported by the HID++ Multi-Platform feature
> * Update documentation and testing description
>
> Changes in v3:
> * Address Sashiko review comments.
> * Switch to devm_mutex_init() to handle mutex lifecycle management
> automatically.
> * Move hidpp_multiplatform_init() to the end of hidpp_probe() after
> hid_device_io_start() to guarantee that the hardware I/O loop is fully
> active when the sysfs attribute becomes visible.
>
> Changes in v4:
> * Address Sashiko review comments.
> * Fix a potential use-after-destroy race condition during device unbind
> by replacing devm_device_add_group() with manual sysfs_create_group()
> management, ensuring the sysfs attribute is removed before the
> associated device resources and send_mutex are destroyed.
>
> Changes in v5:
> * Address Sashiko review comments.
> * Fix a sub system design violation by removing the hardcoded 0xF client
> ID from the lower nibble of the FAP command macros.
>
> .../testing/sysfs-driver-hid-logitech-hidpp | 29 ++
> drivers/hid/hid-ids.h | 2 +
> drivers/hid/hid-logitech-hidpp.c | 399 ++++++++++++++++++
> drivers/hid/hid-quirks.c | 2 +
> 4 files changed, 432 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-logitech-hidpp b/Documentation/ABI/testing/sysfs-driver-hid-logitech-hidpp
> index d8f831f2d6b5..41f0fd8c2192 100644
> --- a/Documentation/ABI/testing/sysfs-driver-hid-logitech-hidpp
> +++ b/Documentation/ABI/testing/sysfs-driver-hid-logitech-hidpp
> @@ -17,3 +17,32 @@ Description:
> handling battery properties in the kernel. This way, upower can
> add a udev rule to decide whether or not it should use the
> internal unifying support or the generic kernel one.
> +
> +What: /sys/bus/hid/drivers/logitech-hidpp-device/<dev>/platform
> +Date: Jul, 2026
> +KernelVersion: 7.2
> +Contact: linux-input@vger.kernel.org
> +Description:
> + (WO) This attribute is present only on Logitech HID++ 2.0 devices
> + that implement feature 0x4531 (Multi-Platform). It allows the host
> + to select which operating-system platform the device should emulate,
> + altering its key mapping and behaviour accordingly.
> +
> + Writing one of the following platform names programs the device:
> +
> + =========== ======================================================
> + windows Standard Windows key layout
> + winemb Windows Embedded key layout
> + linux Linux key layout
> + chrome ChromeOS key layout
> + android Android key layout
> + macos macOS key layout
> + ios iOS key layout
> + webos webOS key layout
> + tizen Tizen key layout
> + =========== ======================================================
> +
> + Only platforms advertised by the device's own descriptors are
> + accepted. The input is case-insensitive. Writing an unknown
> + platform name returns -EINVAL; writing a valid name that the
> + device does not expose in its descriptors returns -EOPNOTSUPP.
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 1059922baaac..a6c2dea79397 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -913,6 +913,8 @@
> #define USB_DEVICE_ID_LOGITECH_T651 0xb00c
> #define USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD 0xb309
> #define USB_DEVICE_ID_LOGITECH_CASA_TOUCHPAD 0xbb00
> +#define USB_DEVICE_ID_LOGITECH_CASA_KEYS_KEYBOARD 0xb371
> +#define USB_DEVICE_ID_LOGITECH_MX_KEYS_S_KEYBOARD 0xb378
> #define USB_DEVICE_ID_LOGITECH_C007 0xc007
> #define USB_DEVICE_ID_LOGITECH_C077 0xc077
> #define USB_DEVICE_ID_LOGITECH_RECEIVER 0xc101
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 90b0184df777..39edb22b55e1 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -209,6 +209,9 @@ struct hidpp_device {
> int hires_wheel_multiplier;
> u8 hires_wheel_feature_index;
>
> + u8 multiplatform_feature_index;
> + struct mutex multiplatform_lock;
> +
> bool connected_once;
> };
>
> @@ -4423,6 +4426,398 @@ 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.
> + */
> +
> +#define HIDPP_MULTIPLATFORM_FEAT_ID 0x4531
> +#define HIDPP_MULTIPLATFORM_GET_FEATURE_INFO 0x00
> +#define HIDPP_MULTIPLATFORM_GET_PLATFORM_DESCRIPTOR 0x10
> +#define HIDPP_MULTIPLATFORM_SET_CURRENT_PLATFORM 0x30
> +
> +#define HIDPP_MULTIPLATFORM_PLATFORM_MASK_TIZEN BIT(0)
> +#define HIDPP_MULTIPLATFORM_PLATFORM_MASK_WINDOWS BIT(8)
> +#define HIDPP_MULTIPLATFORM_PLATFORM_MASK_WINEMB BIT(9)
> +#define HIDPP_MULTIPLATFORM_PLATFORM_MASK_LINUX BIT(10)
> +#define HIDPP_MULTIPLATFORM_PLATFORM_MASK_CHROME BIT(11)
> +#define HIDPP_MULTIPLATFORM_PLATFORM_MASK_ANDROID BIT(12)
> +#define HIDPP_MULTIPLATFORM_PLATFORM_MASK_MACOS BIT(13)
> +#define HIDPP_MULTIPLATFORM_PLATFORM_MASK_IOS BIT(14)
> +#define HIDPP_MULTIPLATFORM_PLATFORM_MASK_WEBOS BIT(15)
> +
> +struct hidpp_platform_desc {
> + u8 plat_idx;
> + u8 desc_idx;
> + u16 plat_mask;
> +};
> +
> +/*
> + * Platform names exposed through the "platform" sysfs attribute. The order of
> + * this array must stay in sync with multiplatform_masks[] below, as the index
> + * returned by sysfs_match_string() is used to look up the matching mask.
> + */
> +static const char * const multiplatform_names[] = {
> + "windows", "winemb", "linux", "chrome",
> + "android", "macos", "ios", "webos", "tizen", NULL
> +};
> +
> +static const u16 multiplatform_masks[] = {
> + HIDPP_MULTIPLATFORM_PLATFORM_MASK_WINDOWS,
> + HIDPP_MULTIPLATFORM_PLATFORM_MASK_WINEMB,
> + HIDPP_MULTIPLATFORM_PLATFORM_MASK_LINUX,
> + HIDPP_MULTIPLATFORM_PLATFORM_MASK_CHROME,
> + HIDPP_MULTIPLATFORM_PLATFORM_MASK_ANDROID,
> + HIDPP_MULTIPLATFORM_PLATFORM_MASK_MACOS,
> + HIDPP_MULTIPLATFORM_PLATFORM_MASK_IOS,
> + HIDPP_MULTIPLATFORM_PLATFORM_MASK_WEBOS,
> + HIDPP_MULTIPLATFORM_PLATFORM_MASK_TIZEN,
> +};
> +
> +/**
> + * hidpp_multiplatform_errno() - Convert HID++ protocol error codes to Linux errno
> + * @err: HID++ protocol error code (positive) or Linux errno (negative or zero)
> + *
> + * Converts a HID++ protocol error code to the corresponding Linux errno. If @err is
> + * already a negative or zero Linux errno, it is returned unchanged. Otherwise, if @err
> + * is a positive HID++ error code, it is mapped to the appropriate negative Linux errno
> + * based on the HID++ specification error codes.
> + *
> + * This is used to ensure that functions interacting with the Multi-Platform feature can
> + * return consistent Linux error codes even when they encounter errors defined by the HID++
> + * protocol when the platform is set from the sysfs attribute.
> + *
> + * Return: Negative Linux errno corresponding to the HID++ error code, or @err if it is
> + * already a Linux errno.
> + */
> +static int hidpp_multiplatform_errno(int err)
> +{
> + if (err <= 0)
> + return err;
> +
> + switch (err) {
> + case HIDPP20_ERROR_INVALID_ARGS:
> + case HIDPP20_ERROR_OUT_OF_RANGE:
> + case HIDPP20_ERROR_INVALID_FEATURE_INDEX:
> + case HIDPP20_ERROR_INVALID_FUNCTION_ID:
> + return -EINVAL;
> + case HIDPP20_ERROR_NOT_ALLOWED:
> + return -EPERM;
> + case HIDPP20_ERROR_BUSY:
> + return -EBUSY;
> + case HIDPP20_ERROR_UNSUPPORTED:
> + return -EOPNOTSUPP;
> + case HIDPP20_ERROR_HW_ERROR:
> + case HIDPP20_ERROR_UNKNOWN:
> + default:
> + return -EIO;
> + }
> +}
> +
> +/**
> + * hidpp_multiplatform_get_num_pdesc() - Retrieve number of platform descriptors
> + * @hidpp: Pointer to the hidpp_device instance
> + * @feat_index: Feature index of the Multi-Platform feature
> + * @num_desc: Pointer to store the number of platform descriptors
> + *
> + * Retrieves the number of platform descriptors supported by the device through
> + * the Multi-Platform feature and stores it in @num_desc.
> + *
> + * Return: 0 on success, or a negative Linux errno on failure.
> + */
> +static int hidpp_multiplatform_get_num_pdesc(struct hidpp_device *hidpp,
> + u8 feat_index, u8 *num_desc)
> +{
> + int ret;
> + struct hidpp_report response;
> + struct hid_device *hdev = hidpp->hid_dev;
> +
> + ret = hidpp_send_fap_command_sync(hidpp, feat_index,
> + HIDPP_MULTIPLATFORM_GET_FEATURE_INFO,
> + NULL, 0, &response);
> + if (ret) {
> + hid_warn(hdev, "Multiplatform: GET_FEATURE_INFO failed (err=%d)", ret);
> + return hidpp_multiplatform_errno(ret);
> + }
> +
> + *num_desc = response.fap.params[3];
> + hid_dbg(hdev, "Multiplatform: Device supports %d platform descriptors", *num_desc);
> +
> + return 0;
> +}
> +
> +/**
> + * hidpp_multiplatform_get_platform_desc() - Retrieve a platform descriptor entry
> + * @hidpp: Pointer to the hidpp_device instance
> + * @feat_index: Feature index of the Multi-Platform feature
> + * @platform_idx: Index of the platform descriptor to retrieve
> + * @pdesc: Pointer to store the retrieved platform descriptor
> + *
> + * Retrieves a single platform descriptor identified by @platform_idx from the
> + * device and stores the parsed descriptor fields in @pdesc.
> + *
> + * Return: 0 on success, or a negative Linux errno on failure.
> + */
> +static int hidpp_multiplatform_get_platform_desc(struct hidpp_device *hidpp, u8 feat_index,
> + u8 platform_idx, struct hidpp_platform_desc *pdesc)
> +{
> + int ret;
> + struct hidpp_report response;
> + u8 params[1] = { platform_idx };
> + struct hid_device *hdev = hidpp->hid_dev;
> +
> + ret = hidpp_send_fap_command_sync(hidpp, feat_index,
> + HIDPP_MULTIPLATFORM_GET_PLATFORM_DESCRIPTOR,
> + params, sizeof(params), &response);
> +
> + if (ret) {
> + hid_warn(hdev,
> + "Multiplatform: GET_PLATFORM_DESCRIPTOR failed for index %d (err=%d)",
> + platform_idx, ret);
> + return hidpp_multiplatform_errno(ret);
> + }
> +
> + pdesc->plat_idx = response.fap.params[0];
> + pdesc->desc_idx = response.fap.params[1];
> + pdesc->plat_mask = get_unaligned_be16(&response.fap.params[2]);
> +
> + hid_dbg(hdev,
> + "Multiplatform: descriptor %d: plat_idx=%d, desc_idx=%d, plat_mask=0x%04x",
> + platform_idx, pdesc->plat_idx, pdesc->desc_idx, pdesc->plat_mask);
> +
> + return 0;
> +}
> +
> +/**
> + * hidpp_multiplatform_get_platform_index() - Find platform index for a mask
> + * @hidpp: Pointer to the hidpp_device instance
> + * @feat_index: Feature index of the Multi-Platform feature
> + * @plat_mask: Platform mask to search for
> + * @plat_index: Pointer to store the matched platform index
> + *
> + * Iterates through all platform descriptors exposed by the device via the
> + * Multi-Platform feature, retrieving each descriptor and comparing its
> + * platform mask to @plat_mask. A descriptor matches if its mask overlaps with
> + * the requested @plat_mask (i.e. (pdesc.plat_mask & plat_mask) is non-zero).
> + *
> + * When a matching descriptor is found, its platform index (plat_idx) is
> + * written to @plat_index and the function returns success.
> + *
> + * Return: 0 on success; -EOPNOTSUPP if the device exposes no descriptor
> + * matching @plat_mask; or another negative Linux errno on transport
> + * failure.
> + */
> +static int hidpp_multiplatform_get_platform_index(struct hidpp_device *hidpp,
> + u8 feat_index, u16 plat_mask,
> + u8 *plat_index)
> +{
> + int i;
> + int ret;
> + u8 num_desc;
> + struct hidpp_platform_desc pdesc;
> + struct hid_device *hdev = hidpp->hid_dev;
> +
> + ret = hidpp_multiplatform_get_num_pdesc(hidpp, feat_index, &num_desc);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < num_desc; i++) {
> + ret = hidpp_multiplatform_get_platform_desc(hidpp, feat_index, i, &pdesc);
> + if (ret)
> + return ret;
> +
> + if (pdesc.plat_mask & plat_mask) {
> + *plat_index = pdesc.plat_idx;
> + hid_dbg(hdev,
> + "Multiplatform: Selected platform index %d for mask 0x%04x",
> + *plat_index, plat_mask);
> + return 0;
> + }
> + }
> +
> + hid_dbg(hdev,
> + "Multiplatform: No matching platform descriptor for mask 0x%04x",
> + plat_mask);
> + return -EOPNOTSUPP;
> +}
> +
> +/**
> + * hidpp_multiplatform_update_device_platform() - Update the device platform
> + * @hidpp: Pointer to the hidpp_device instance
> + * @feat_index: Feature index of the Multi-Platform feature
> + * @plat_index: Platform index to set on the device
> + *
> + * Sends the HID++ Multi-Platform 'SET_CURRENT_PLATFORM' command to the device to
> + * update its platform index to @plat_index.
> + *
> + * Return: 0 on success, or a negative Linux errno on failure.
> + */
> +static int hidpp_multiplatform_update_device_platform(struct hidpp_device *hidpp,
> + u8 feat_index, u8 plat_index)
> +{
> + int ret;
> + struct hidpp_report response;
> + /* Byte 0 (hostIndex): 0xFF selects the current host. */
> + u8 params[2] = { 0xFF, plat_index };
> +
> + ret = hidpp_send_fap_command_sync(hidpp, feat_index,
> + HIDPP_MULTIPLATFORM_SET_CURRENT_PLATFORM,
> + params, sizeof(params), &response);
> +
> + if (ret)
> + hid_warn(hidpp->hid_dev,
> + "Multiplatform: SET_CURRENT_PLATFORM failed for index %d (err=%d)",
> + plat_index, ret);
> +
> + return hidpp_multiplatform_errno(ret);
> +}
> +
> +/**
> + * hidpp_multiplatform_set_platform() - Apply a platform to the device
> + * @hidpp: Pointer to the hidpp_device instance
> + * @mask: A single HIDPP_MULTIPLATFORM_PLATFORM_MASK_* bit to apply
> + *
> + * Looks up the device's platform descriptor whose platform mask matches @mask
> + * and instructs the device to switch to it via SET_CURRENT_PLATFORM.
> + *
> + * Return: 0 on success, -EOPNOTSUPP if the device does not implement feature
> + * 0x4531 or exposes no descriptor matching @mask, or another negative
> + * Linux errno from the underlying HID++ command.
> + */
> +static int hidpp_multiplatform_set_platform(struct hidpp_device *hidpp, u16 mask)
> +{
> + u8 plat_index;
> + int ret;
> +
> + if (!hidpp->multiplatform_feature_index)
> + return -EOPNOTSUPP;
> +
> + ret = hidpp_multiplatform_get_platform_index(hidpp,
> + hidpp->multiplatform_feature_index, mask, &plat_index);
> + if (ret)
> + return ret;
> +
> + ret = hidpp_multiplatform_update_device_platform(hidpp,
> + hidpp->multiplatform_feature_index, plat_index);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +/**
> + * platform_store() - Set the device platform based on user input
> + * @dev: Pointer to the device instance
> + * @attr: Pointer to the device attribute
> + * @buf: Buffer containing the platform name string
> + * @count: Size of the input buffer
> + *
> + * Parses the platform name from the input buffer, converts it to a platform mask,
> + * and applies it to the device using the HID++ Multi-Platform feature. The function
> + * handles errors gracefully, returning appropriate Linux errno values if the input
> + * is invalid or if the device does not support the requested platform.
> + *
> + * Return: Number of bytes consumed from the input buffer on success, or a negative
> + * Linux errno on failure.
> + */
> +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);
> + 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;
> +}
> +
> +static DEVICE_ATTR_WO(platform);
> +
> +static struct attribute *multiplatform_attrs[] = {
> + &dev_attr_platform.attr,
> + NULL
> +};
> +
> +static const struct attribute_group multiplatform_attribute_group = {
> + .attrs = multiplatform_attrs,
> +};
> +
> +/**
> + * hidpp_multiplatform_init() - Initialize HID++ Multi-Platform support
> + * @hidpp: Pointer to the hidpp_device instance
> + *
> + * Checks if the device supports the HID++ Multi-Platform feature (0x4531) and, if so,
> + * initializes the hidpp_device structure to track the feature index and creates the
> + * corresponding sysfs attribute group for platform selection.
> + */
> +static void hidpp_multiplatform_init(struct hidpp_device *hidpp)
> +{
> + struct hid_device *hdev = hidpp->hid_dev;
> + u8 feat_index;
> + int ret;
> +
> + ret = hidpp_root_get_feature(hidpp, HIDPP_MULTIPLATFORM_FEAT_ID, &feat_index);
> + if (ret)
> + return;
> +
> + hidpp->multiplatform_feature_index = feat_index;
> +
> + mutex_init(&hidpp->multiplatform_lock);
> +
> + ret = sysfs_create_group(&hdev->dev.kobj, &multiplatform_attribute_group);
> + if (ret) {
> + hid_warn(hdev,
> + "Multiplatform: Failed to create sysfs group (err=%d)\n", ret);
> + mutex_destroy(&hidpp->multiplatform_lock);
> + hidpp->multiplatform_feature_index = 0;
> + }
> +}
> +
> +/**
> + * hidpp_multiplatform_cleanup() - Cleanup HID++ Multi-Platform support
> + * @hidpp: Pointer to the hidpp_device instance
> + *
> + * Removes the sysfs attribute group for platform selection and destroys the mutex
> + * used for synchronizing access to the Multi-Platform feature.
> + * This function should be called during device removal or driver cleanup to ensure
> + * proper resource management.
> + */
> +static void hidpp_multiplatform_cleanup(struct hidpp_device *hidpp)
> +{
> + if (!hidpp->multiplatform_feature_index)
> + return;
> +
> + sysfs_remove_group(&hidpp->hid_dev->dev.kobj, &multiplatform_attribute_group);
> + mutex_destroy(&hidpp->multiplatform_lock);
> +}
> +
> static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> {
> struct hidpp_device *hidpp;
> @@ -4545,6 +4940,8 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
> }
>
> + hidpp_multiplatform_init(hidpp);
> +
> /*
> * This relies on logi_dj_ll_close() being a no-op so that DJ connection
> * events will still be received.
> @@ -4557,6 +4954,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> hid_hw_open_fail:
> hid_hw_stop(hdev);
> hid_hw_start_fail:
> + hidpp_multiplatform_cleanup(hidpp);
> sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
> cancel_work_sync(&hidpp->work);
> mutex_destroy(&hidpp->send_mutex);
> @@ -4570,6 +4968,7 @@ static void hidpp_remove(struct hid_device *hdev)
> if (!hidpp)
> return hid_hw_stop(hdev);
>
> + hidpp_multiplatform_cleanup(hidpp);
> sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
>
> hid_hw_stop(hdev);
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index 57d8efdd9b89..577af0075e27 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -537,6 +537,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
> #endif
> #if IS_ENABLED(CONFIG_HID_LOGITECH_HIDPP)
> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL) },
> + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_CASA_KEYS_KEYBOARD) },
> + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MX_KEYS_S_KEYBOARD) },
> #endif
> #if IS_ENABLED(CONFIG_HID_MAGICMOUSE)
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE) },
> --
> 2.34.1
>
^ permalink raw reply
* [PATCH v2] HID: mcp2221: validate raw report length
From: Pengpeng Hou @ 2026-07-15 13:57 UTC (permalink / raw)
To: Rishi Gupta
Cc: Pengpeng Hou, Jiri Kosina, Benjamin Tissoires, linux-i2c,
linux-input, linux-kernel
The MCP2221A data sheet specifies 64-byte HID command and response
reports. mcp2221_raw_event() reads fixed response fields up to byte 55
and can copy a 60-byte I2C payload from byte 4 through byte 63 without
first checking the received size.
HID invokes raw_event() before generic report validation. A short report
can therefore be interpreted beyond its received length. Reject reports
shorter than the fixed protocol size before reading any response fields.
Complete the pending command with -EINVAL for a truncated report so that
a malformed response does not turn into an unrelated timeout.
Fixes: 67a95c21463d ("HID: mcp2221: add usb to i2c-smbus host bridge")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1: https://lore.kernel.org/all/20260715083754.31210-1-pengpeng@iscas.ac.cn/
- validate the fixed 64-byte protocol size instead of the
descriptor-derived report length
- use a named report-size constant and remove matching magic lengths
---
drivers/hid/hid-mcp2221.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index e4ddd8e..c910f66 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -90,7 +90,8 @@ enum {
MCP2221_DIR_IN = 0x01,
};
-#define MCP_NGPIO 4
+#define MCP2221_REPORT_SIZE 64
+#define MCP_NGPIO 4
/* MCP GPIO set command layout */
struct mcp_set_gpio {
@@ -126,7 +127,7 @@ struct mcp2221 {
struct completion wait_in_report;
struct delayed_work init_work;
u8 *rxbuf;
- u8 txbuf[64];
+ u8 txbuf[MCP2221_REPORT_SIZE];
int rxbuf_idx;
int rxbuf_size;
int status;
@@ -639,11 +640,11 @@ static int mcp_gpio_read_sram(struct mcp2221 *mcp)
{
int ret;
- memset(mcp->txbuf, 0, 64);
+ memset(mcp->txbuf, 0, sizeof(mcp->txbuf));
mcp->txbuf[0] = MCP2221_GET_SRAM_SETTINGS;
mutex_lock(&mcp->lock);
- ret = mcp_send_data_req_status(mcp, mcp->txbuf, 64);
+ ret = mcp_send_data_req_status(mcp, mcp->txbuf, sizeof(mcp->txbuf));
mutex_unlock(&mcp->lock);
return ret;
@@ -682,7 +683,7 @@ static int mcp2221_check_gpio_pinfunc(struct mcp2221 *mcp)
* Set all bytes to 0, so Bit 7 is not set. The chip
* only changes content of a register when bit 7 is set.
*/
- memset(mcp->txbuf, 0, 64);
+ memset(mcp->txbuf, 0, sizeof(mcp->txbuf));
mcp->txbuf[0] = MCP2221_SET_SRAM_SETTINGS;
/*
@@ -703,7 +704,7 @@ static int mcp2221_check_gpio_pinfunc(struct mcp2221 *mcp)
}
mutex_lock(&mcp->lock);
- ret = mcp_send_data_req_status(mcp, mcp->txbuf, 64);
+ ret = mcp_send_data_req_status(mcp, mcp->txbuf, sizeof(mcp->txbuf));
mutex_unlock(&mcp->lock);
return ret;
@@ -861,6 +862,12 @@ static int mcp2221_raw_event(struct hid_device *hdev,
u8 *buf;
struct mcp2221 *mcp = hid_get_drvdata(hdev);
+ if (size < MCP2221_REPORT_SIZE) {
+ mcp->status = -EINVAL;
+ complete(&mcp->wait_in_report);
+ return 1;
+ }
+
switch (data[0]) {
case MCP2221_I2C_WR_DATA:
--
2.50.1 (Apple Git-155)
^ permalink raw reply related
* [PATCH 0/4] virtio: validate device-reported values across drivers
From: Hari Mishal @ 2026-07-15 14:22 UTC (permalink / raw)
To: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Gerd Hoffmann,
Michael S . Tsirkin, Jason Wang, David Hildenbrand,
Henrik Rydberg
Cc: Xuan Zhuo, Eugenio Pérez, virtualization, linux-kernel,
linux-input, Hari Mishal
These four patches harden virtio-mem, virtio_input, and virtio_console
against untrusted values coming from the device/backend side of the virtio
interface. The fourth fixes a use-after-free in virtio_console caused by a
missing refcount.
Hari Mishal (4):
virtio-mem: validate device-reported block size
virtio_input: validate device-reported multitouch slot count
virtio_console: avoid NULL portdev dereference in in_intr()
virtio_console: take a kref in find_port_by_vq() to fix port UAF
drivers/char/virtio_console.c | 18 +++++++++++++++++-
drivers/virtio/virtio_input.c | 4 ++++
drivers/virtio/virtio_mem.c | 7 +++++++
3 files changed, 28 insertions(+), 1 deletion(-)
--
2.43.0
^ permalink raw reply
* [PATCH 1/4] virtio-mem: validate device-reported block size
From: Hari Mishal @ 2026-07-15 14:22 UTC (permalink / raw)
To: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Gerd Hoffmann,
Michael S . Tsirkin, Jason Wang, David Hildenbrand,
Henrik Rydberg
Cc: Xuan Zhuo, Eugenio Pérez, virtualization, linux-kernel,
linux-input, Hari Mishal
In-Reply-To: <20260715142337.22811-1-harimishal1@gmail.com>
The device_block_size read from the virtio-mem config space is used
as a divisor and also in ALIGN_DOWN() further down the code path in
the driver without further validation. A zero value leads to a division
by zero, and a non-power-of-two value corrupts the ALIGN_DOWN() bitmask
arithmetic leading to a misreporting of guest usable guest ram, post
crash. Reject both at init time instead of trusting the device.
Assisted-by: gkh_clanker:t1000
Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
drivers/virtio/virtio_mem.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 11c441501582..43d12ec7c323 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -2847,6 +2847,13 @@ static int virtio_mem_init(struct virtio_mem *vm)
&vm->plugged_size);
virtio_cread_le(vm->vdev, struct virtio_mem_config, block_size,
&vm->device_block_size);
+ if (!vm->device_block_size ||
+ !is_power_of_2(vm->device_block_size)) {
+ dev_err(&vm->vdev->dev,
+ "invalid device block size: 0x%llx\n",
+ (unsigned long long)vm->device_block_size);
+ return -EINVAL;
+ }
virtio_cread_le(vm->vdev, struct virtio_mem_config, node_id,
&node_id);
vm->nid = virtio_mem_translate_node_id(vm, node_id);
--
2.43.0
^ permalink raw reply related
* [PATCH 2/4] virtio_input: validate device-reported multitouch slot count
From: Hari Mishal @ 2026-07-15 14:22 UTC (permalink / raw)
To: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Gerd Hoffmann,
Michael S . Tsirkin, Jason Wang, David Hildenbrand,
Henrik Rydberg
Cc: Xuan Zhuo, Eugenio Pérez, virtualization, linux-kernel,
linux-input, Hari Mishal
In-Reply-To: <20260715142337.22811-1-harimishal1@gmail.com>
nslots is derived from the ABS_MT_SLOT maximum reported by the
virtio device. A device could report a bogus maximum (e.g. -1)
making nslots = 0, which input_mt_init_slots() does not reject;
it returns success without allocating any slot storage, silently
leaving the device registered as multitouch capable with no
backing state. Reject non-positive slot counts before calling
input_mt_init_slots().
Assisted-by: gkh_clanker:t1000
Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
drivers/virtio/virtio_input.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index deec24e8e682..2cc19782cdd3 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c
@@ -312,6 +312,10 @@ static int virtinput_probe(struct virtio_device *vdev)
if (test_bit(ABS_MT_SLOT, vi->idev->absbit)) {
nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1;
+ if (nslots <= 0) {
+ err = -EINVAL;
+ goto err_mt_init_slots;
+ }
err = input_mt_init_slots(vi->idev, nslots, 0);
if (err)
goto err_mt_init_slots;
--
2.43.0
^ permalink raw reply related
* [PATCH 3/4] virtio_console: avoid NULL portdev dereference in in_intr()
From: Hari Mishal @ 2026-07-15 14:22 UTC (permalink / raw)
To: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Gerd Hoffmann,
Michael S . Tsirkin, Jason Wang, David Hildenbrand,
Henrik Rydberg
Cc: Xuan Zhuo, Eugenio Pérez, virtualization, linux-kernel,
linux-input, Hari Mishal
In-Reply-To: <20260715142337.22811-1-harimishal1@gmail.com>
A port's virtqueue is not torn down immediately if the port itself is
hot-unplugged (unplug_port() only nulls port->portdev; the vq
callback stays registered until the whole device is removed).
If in_intr() fires for a port in that window it dereferences
port->portdev->vdev via is_rproc_serial(), crashing on the NULL
portdev.
Bail out early when portdev has already been cleared.
Assisted-by: gkh_clanker:t1000
Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
drivers/char/virtio_console.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 198b97314168..faef362dae85 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1720,6 +1720,11 @@ static void in_intr(struct virtqueue *vq)
}
spin_lock_irqsave(&port->inbuf_lock, flags);
+ if (!port->portdev) {
+ /* Port is being unplugged, ignore further data. */
+ spin_unlock_irqrestore(&port->inbuf_lock, flags);
+ return;
+ }
port->inbuf = get_inbuf(port);
/*
--
2.43.0
^ permalink raw reply related
* [PATCH 4/4] virtio_console: take a kref in find_port_by_vq() to fix port UAF
From: Hari Mishal @ 2026-07-15 14:22 UTC (permalink / raw)
To: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Gerd Hoffmann,
Michael S . Tsirkin, Jason Wang, David Hildenbrand,
Henrik Rydberg
Cc: Xuan Zhuo, Eugenio Pérez, virtualization, linux-kernel,
linux-input, Hari Mishal
In-Reply-To: <20260715142337.22811-1-harimishal1@gmail.com>
find_port_by_vq() returns a raw struct port pointer without taking a
reference on it, unlike find_port_by_devt_in_portdev() which does.
find_port_by_vq()'s only two callers, in_intr() and out_intr(), run
as virtqueue interrupt callbacks, entirely independent of and
possibly concurrently with unplug_port(), which itself runs from a
workqueue when the host sends a VIRTIO_CONSOLE_PORT_REMOVE control
message.
unplug_port() removes the port from portdev->ports under ports_lock,
then later drops its last reference with kref_put(), freeing it via
remove_port(). find_port_by_vq() also walks portdev->ports under
ports_lock, so if it finds the port still on the list, the list
removal, and therefore the eventual kref_put(), has not happened yet,
and taking a reference at that point is always safe. Without doing
so, in_intr()/out_intr() can be left holding a pointer to a port that
unplug_port() frees on another core before they are done using it.
Both triggers are host-controlled as the host decides when to send the
PORT_REMOVE control message and when to kick the port's data vq. So a
malicious backend could race the two on purpose, without any guest side
cooperation. The freed object is a generic kmalloc allocation containing
a wait_queue_head_t, which in_intr()/out_intr() pass to
wake_up_interruptible() after touching the stale pointer.
wake_up_interruptible() invokes a function pointer read out of the
wait queue's entries. If the freed slab slot is reclaimed with
attacker influenced content before that call, then this is an arbitrary
function call primitive rather than just undefined behaviour.
Take a reference in find_port_by_vq() while still holding ports_lock,
matching find_port_by_devt_in_portdev(), and release it in in_intr()
and out_intr() once they are done with the port.
Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
drivers/char/virtio_console.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index faef362dae85..1b7593684ed9 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -304,6 +304,11 @@ static struct port *find_port_by_id(struct ports_device *portdev, u32 id)
return port;
}
+/*
+ * The port object's reference is incremented now and
+ * it is the caller's responsibility to decrement it
+ */
+
static struct port *find_port_by_vq(struct ports_device *portdev,
struct virtqueue *vq)
{
@@ -312,8 +317,10 @@ static struct port *find_port_by_vq(struct ports_device *portdev,
spin_lock_irqsave(&portdev->ports_lock, flags);
list_for_each_entry(port, &portdev->ports, list)
- if (port->in_vq == vq || port->out_vq == vq)
+ if (port->in_vq == vq || port->out_vq == vq) {
+ kref_get(&port->kref);
goto out;
+ }
port = NULL;
out:
spin_unlock_irqrestore(&portdev->ports_lock, flags);
@@ -1706,6 +1713,7 @@ static void out_intr(struct virtqueue *vq)
}
wake_up_interruptible(&port->waitqueue);
+ kref_put(&port->kref, remove_port);
}
static void in_intr(struct virtqueue *vq)
@@ -1723,6 +1731,7 @@ static void in_intr(struct virtqueue *vq)
if (!port->portdev) {
/* Port is being unplugged, ignore further data. */
spin_unlock_irqrestore(&port->inbuf_lock, flags);
+ kref_put(&port->kref, remove_port);
return;
}
port->inbuf = get_inbuf(port);
@@ -1756,6 +1765,8 @@ static void in_intr(struct virtqueue *vq)
if (is_console_port(port) && hvc_poll(port->cons.hvc))
hvc_kick();
+
+ kref_put(&port->kref, remove_port);
}
static void control_intr(struct virtqueue *vq)
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 2/5] HID: multitouch: Fix stale MT slots when contact count drops to zero
From: Dave Carey @ 2026-07-15 14:33 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Dave Carey, Benjamin Tissoires, linux-input
In-Reply-To: <20260522111527.69428-1-carvsdriver@gmail.com>
Hi Jiri,
Gentle second ping on this patch, now about eight weeks since submission
with no feedback.
For context, the other hid-multitouch patches for the same device are
already in tree: the INPUT_PROP_BUTTONPAD misclassification fix merged
to mainline 2026-05-28, and the ghost contacts fix (MT_QUIRK_CONTACT_CNT_ACCURATE)
was applied to hid/for-next by Benjamin on 2026-06-10. This stale-slots
fix is the last remaining hid-multitouch patch for the Yoga Book 9 14IAH10.
Happy to rebase, respin, or address any concerns.
Thanks,
Dave
^ permalink raw reply
* Re: [PATCH v2] HID: mcp2221: validate raw report length
From: sashiko-bot @ 2026-07-15 14:34 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715135916.24417-5-pengpeng@iscas.ac.cn>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Use-After-Free and Out-Of-Bounds Write due to a dangling `mcp->rxbuf` pointer after an I2C transaction times out.
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) Buffer Overflow in `mcp2221_raw_event` due to non-atomic buffer state updates.
- [High] Concurrent GPIO and I2C operations modify the shared hardware buffer (`mcp->txbuf`) outside of the `mcp->lock` mutex, leading to data corruption.
- [High] Teardown asymmetry and Use-After-Free caused by runtime modification of the `gpio_mode_enforce` module parameter.
--
--- Patch [1]: [PATCH v2] HID: mcp2221: validate raw report length ---
commit 7349c59966a5598d3165aa4ca24a32b27b7b131a
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Subject: HID: mcp2221: validate raw report length
The MCP2221A data sheet specifies 64-byte HID command and response
reports. mcp2221_raw_event() reads fixed response fields up to byte 55
and can copy a 60-byte I2C payload from byte 4 through byte 63 without
first checking the received size. Reject reports shorter than the fixed
protocol size before reading any response fields.
Link: https://lore.kernel.org/all/20260715083754.31210-1-pengpeng@iscas.ac.cn/
> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index e4ddd8e9293b6..c910f6636ac95 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
[ ... ]
> @@ -639,11 +640,11 @@ static int mcp_gpio_read_sram(struct mcp2221 *mcp)
> {
> int ret;
>
> - memset(mcp->txbuf, 0, 64);
> + memset(mcp->txbuf, 0, sizeof(mcp->txbuf));
[Severity: High]
This is a pre-existing issue, but I noticed mcp->txbuf is being zeroed and
populated here in mcp_gpio_read_sram() (and similarly in mcp_gpio_set() and
mcp_gpio_get()) before acquiring mcp->lock.
Because mcp->txbuf is shared with I2C operations, could this lead to data
corruption if mcp_i2c_xfer() is concurrently preparing an I2C transaction
under the lock? It looks like sysfs GPIO nodes and I2C device interfaces
can race here.
> mcp->txbuf[0] = MCP2221_GET_SRAM_SETTINGS;
>
> mutex_lock(&mcp->lock);
> - ret = mcp_send_data_req_status(mcp, mcp->txbuf, 64);
> + ret = mcp_send_data_req_status(mcp, mcp->txbuf, sizeof(mcp->txbuf));
> mutex_unlock(&mcp->lock);
>
> return ret;
[ ... ]
> @@ -861,6 +862,12 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> u8 *buf;
> struct mcp2221 *mcp = hid_get_drvdata(hdev);
>
> + if (size < MCP2221_REPORT_SIZE) {
> + mcp->status = -EINVAL;
> + complete(&mcp->wait_in_report);
> + return 1;
> + }
> +
[Severity: Critical]
This is a pre-existing issue, but does mcp2221_raw_event() have a
use-after-free or out-of-bounds write risk when handling delayed responses?
If mcp_send_data_req_status() times out inside mcp_i2c_smbus_read(), it
returns early without setting mcp->rxbuf to NULL. When a delayed I2C
response arrives later, wouldn't mcp2221_raw_event() copy up to 60 bytes
into this dangling mcp->rxbuf pointer?
[Severity: Critical]
This is also a pre-existing issue, but is there a TOCTOU race here with how
mcp->rxbuf and mcp->rxbuf_size are updated in mcp_i2c_smbus_read()?
If mcp_i2c_smbus_read() is updating these non-atomically:
mcp_i2c_smbus_read() {
...
mcp->rxbuf = msg->buf;
mcp->rxbuf_size = msg->len;
...
}
and an interrupt fires exactly between assigning a new 1-byte buffer to
mcp->rxbuf and updating mcp->rxbuf_size, could mcp2221_raw_event() bypass
the bounds check by reading the old, larger size?
> switch (data[0]) {
>
> case MCP2221_I2C_WR_DATA:
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but could runtime modifications
to the module parameter gpio_mode_enforce lead to a use-after-free during
device removal?
In mcp2221_remove(), the delayed work is conditionally canceled:
mcp2221_remove() {
...
if (!gpio_mode_enforce)
cancel_delayed_work_sync(&mcp->init_work);
...
}
Since gpio_mode_enforce has 0644 permissions, if it is changed to true
after the device is plugged in, won't mcp2221_remove() skip canceling
mcp->init_work? This would leave the work running after the mcp structure
is freed.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715135916.24417-5-pengpeng@iscas.ac.cn?part=1
^ permalink raw reply
* Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count
From: sashiko-bot @ 2026-07-15 14:35 UTC (permalink / raw)
To: Hari Mishal; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715142337.22811-3-harimishal1@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Concurrent execution of virtqueue interrupts during device teardown causes a use-after-free of the input device.
- [High] If `input_register_device()` fails, the driver deletes virtqueues without first resetting the virtio device, violating the virtio specification.
- [Medium] The driver fails to detach unused buffers from the `vi->evt` queue before deleting the virtqueues, leading to leaked DMA mappings.
- [Medium] Missing null-termination for `vi->name` and `vi->serial` strings copied from the host.
--
--- Patch [2]: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count ---
commit 2551acf6fb14e59620dd29579313834ccc291ba0
Author: Hari Mishal <harimishal1@gmail.com>
virtio_input: validate device-reported multitouch slot count
The commit adds validation for the nslots value derived from the
device-reported ABS_MT_SLOT maximum. It prevents registering a
multitouch device with zero backing slots by rejecting non-positive
slot counts before calling input_mt_init_slots().
> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> index deec24e8e6828..2cc19782cdd33 100644
> --- a/drivers/virtio/virtio_input.c
> +++ b/drivers/virtio/virtio_input.c
> @@ -312,6 +312,10 @@ static int virtinput_probe(struct virtio_device *vdev)
>
> if (test_bit(ABS_MT_SLOT, vi->idev->absbit)) {
> nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1;
> + if (nslots <= 0) {
> + err = -EINVAL;
> + goto err_mt_init_slots;
> + }
> err = input_mt_init_slots(vi->idev, nslots, 0);
> if (err)
> goto err_mt_init_slots;
[Severity: High]
This is a pre-existing issue, but can concurrent execution of virtqueue
interrupts during device teardown cause a use-after-free of the input device?
Looking at virtinput_remove(), the teardown unregisters the input device
before stopping the hardware and synchronizing interrupts:
drivers/virtio/virtio_input.c:virtinput_remove() {
...
input_unregister_device(vi->idev);
virtio_reset_device(vdev);
...
}
If virtinput_recv_events() runs concurrently with virtinput_remove(), the
interrupt handler temporarily drops vi->lock and calls input_event(vi->idev).
Meanwhile, virtinput_remove() can free vi->idev if there are no active users,
since it doesn't call virtio_reset_device() or del_vqs() until after
unregistering the input device.
[Severity: High]
This is a pre-existing issue, but does the error path in virtinput_probe()
violate the virtio specification by deleting virtqueues without resetting
an active device?
During virtinput_probe(), the device is marked ready and activated:
drivers/virtio/virtio_input.c:virtinput_probe() {
...
virtio_device_ready(vdev);
vi->ready = true;
err = input_register_device(vi->idev);
if (err)
goto err_input_register;
...
}
If input_register_device() fails, the code jumps to the error path and deletes
the virtqueues without first calling virtio_reset_device():
drivers/virtio/virtio_input.c:virtinput_probe() {
...
err_input_register:
spin_lock_irqsave(&vi->lock, flags);
vi->ready = false;
spin_unlock_irqrestore(&vi->lock, flags);
err_mt_init_slots:
input_free_device(vi->idev);
err_input_alloc:
vdev->config->del_vqs(vdev);
...
}
Could this lead to memory corruption or host crashes due to DMA into
freed queues?
[Severity: Medium]
This is a pre-existing issue, but does the driver leak DMA mappings for the
vi->evt queue during teardown?
In virtinput_remove() and virtinput_freeze(), the driver detaches and frees
buffers from the vi->sts queue, but ignores the vi->evt queue:
drivers/virtio/virtio_input.c:virtinput_remove() {
...
while ((buf = virtqueue_detach_unused_buf(vi->sts)) != NULL)
kfree(buf);
vdev->config->del_vqs(vdev);
...
}
The buffers in vi->evt were mapped for DMA when queued in virtinput_fill_evt().
Could deleting the virtqueues without detaching these unused buffers leave
their DMA mappings active indefinitely?
[Severity: Medium]
This is a pre-existing issue, but does the device name copy lack
null-termination if the host provides a full-length string?
In virtinput_probe(), the host-provided string is copied into the fixed-size
vi->name array:
drivers/virtio/virtio_input.c:virtinput_probe() {
...
size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config,
u.string),
vi->name, min(size, sizeof(vi->name)));
...
}
If the host provides exactly 64 bytes of non-null characters, vi->name will
not be null-terminated.
Since this string is passed to vi->idev->name, could a subsequent sysfs read
of the device name read past the array boundary?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715142337.22811-1-harimishal1@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH 3/4] virtio_console: avoid NULL portdev dereference in in_intr()
From: sashiko-bot @ 2026-07-15 14:37 UTC (permalink / raw)
To: Hari Mishal; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715142337.22811-4-harimishal1@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The newly introduced `!port->portdev` check in `in_intr()` suffers from a Time-Of-Check to Time-Of-Use (TOCTOU) race condition and fails to robustly prevent the NULL pointer dereference.
- [Medium] Bailing out early from `in_intr()` without consuming the used virtqueue buffers leaves stale data in the queue, contaminating future port instances that reuse the same ID.
Pre-existing issues:
- [High] The patch fails to address a fundamental Use-After-Free (UAF) vulnerability in `in_intr()` and `out_intr()`. The helper `find_port_by_vq()` returns a `port` without taking a reference count, allowing concurrent hot-unplug to free the `port` while interrupt handlers are executing.
--
--- Patch [3]: [PATCH 3/4] virtio_console: avoid NULL portdev dereference in in_intr() ---
commit 53028db078d1834cd8a61fa35f5ddb4495c447b8
Author: Hari Mishal <harimishal1@gmail.com>
virtio_console: avoid NULL portdev dereference in in_intr()
This commit attempts to fix a null pointer dereference in in_intr() caused
when a port is hot-unplugged. It adds an early exit check for portdev being
null under the inbuf_lock to ignore further data during the unplug window.
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 198b973141680..faef362dae853 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
[ ... ]
> @@ -1720,6 +1720,11 @@ static void in_intr(struct virtqueue *vq)
> }
>
> spin_lock_irqsave(&port->inbuf_lock, flags);
[Severity: High]
This is a pre-existing issue, but does find_port_by_vq() return the port
pointer without acquiring a reference via kref_get()?
If unplug_port() runs concurrently, can it drop the final reference and free
the port structure right before we try to take the lock here?
CPU0 CPU1
in_intr() unplug_port()
port = find_port_by_vq(...)
kref_put(&port->kref, remove_port);
spin_lock_irqsave(...)
Would this result in a use-after-free on the port structure?
> + if (!port->portdev) {
[Severity: High]
Does this check reliably prevent a null pointer dereference?
Since unplug_port() sets port->portdev = NULL locklessly outside of
inbuf_lock, could a concurrent unplug set it to null immediately after this
check passes?
Because get_inbuf() is called next, might the compiler reload port->portdev
from memory for the later is_rproc_serial(port->portdev->vdev) check,
crashing on the newly null pointer?
> + /* Port is being unplugged, ignore further data. */
> + spin_unlock_irqrestore(&port->inbuf_lock, flags);
> + return;
[Severity: Medium]
Does returning here without consuming the virtqueue buffers leave stale data
in the queue?
Since virtqueues persist across hot-unplugs for the same port ID, could a
newly hot-plugged port reuse this virtqueue and read leaked data from the
previous session?
Should this path consume the buffers, perhaps by calling
flush_bufs(vq, false), before returning?
> + }
> port->inbuf = get_inbuf(port);
>
> /*
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715142337.22811-1-harimishal1@gmail.com?part=3
^ 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