* [PATCH v8 1/9] dt-bindings: mfd: mt6397: Add MT6392 PMIC
From: Luca Leonardo Scorcia @ 2026-06-20 19:56 UTC (permalink / raw)
To: linux-mediatek
Cc: Luca Leonardo Scorcia, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sen Chu, Sean Wang,
Macpaul Lin, Lee Jones, Matthias Brugger,
AngeloGioacchino Del Regno, Liam Girdwood, Mark Brown,
Linus Walleij, Val Packett, Julien Massot, Louis-Alexis Eyraud,
Fabien Parent, Akari Tsuyukusa, Chen Zhong, linux-input,
devicetree, linux-kernel, linux-pm, linux-arm-kernel, linux-gpio
In-Reply-To: <20260620200032.334192-1-l.scorcia@gmail.com>
Describe the MT6392 PMIC and its RTC and regulator devices. This device
is mostly based on MT6323 with some similarities to MT6397 and is usually
found on boards using the MT8516/MT8167 SoC.
Signed-off-by: Luca Leonardo Scorcia <l.scorcia@gmail.com>
---
.../bindings/mfd/mediatek,mt6397.yaml | 74 +++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6397.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6397.yaml
index 3cbc0dc12c31..927df823d640 100644
--- a/Documentation/devicetree/bindings/mfd/mediatek,mt6397.yaml
+++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6397.yaml
@@ -40,6 +40,10 @@ properties:
- mediatek,mt6358
- mediatek,mt6359
- mediatek,mt6397
+ - items:
+ - enum:
+ - mediatek,mt6392
+ - const: mediatek,mt6323
- items:
- enum:
- mediatek,mt6366
@@ -72,6 +76,10 @@ properties:
- mediatek,mt6331-rtc
- mediatek,mt6358-rtc
- mediatek,mt6397-rtc
+ - items:
+ - enum:
+ - mediatek,mt6392-rtc
+ - const: mediatek,mt6323-rtc
- items:
- enum:
- mediatek,mt6359-rtc
@@ -99,6 +107,7 @@ properties:
- mediatek,mt6331-regulator
- mediatek,mt6358-regulator
- mediatek,mt6359-regulator
+ - mediatek,mt6392-regulator
- mediatek,mt6397-regulator
- items:
- enum:
@@ -663,3 +672,68 @@ examples:
compatible = "mediatek,mt6397-rtc";
};
};
+
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ pmic {
+ compatible = "mediatek,mt6392", "mediatek,mt6323";
+
+ interrupts-extended = <&pio 28 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
+ keys {
+ compatible = "mediatek,mt6392-keys";
+
+ key-power {
+ linux,keycodes = <KEY_POWER>;
+ wakeup-source;
+ };
+
+ key-home {
+ linux,keycodes = <KEY_HOME>;
+ wakeup-source;
+ };
+ };
+
+ pinctrl {
+ compatible = "mediatek,mt6392-pinctrl";
+
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
+ regulators {
+ compatible = "mediatek,mt6392-regulator";
+
+ vproc {
+ regulator-allowed-modes = <0 1>;
+ regulator-initial-mode = <0>;
+ regulator-min-microvolt = < 700000>;
+ regulator-max-microvolt = <1350000>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ // ...
+
+ vadc18 {
+ regulator-allowed-modes = <0 2>;
+ regulator-initial-mode = <0>;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
+ vefuse {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <2000000>;
+ };
+ };
+
+ rtc {
+ compatible = "mediatek,mt6392-rtc", "mediatek,mt6323-rtc";
+ };
+ };
--
2.43.0
^ permalink raw reply related
* [PATCH v8 0/9] Add support for MT6392 PMIC
From: Luca Leonardo Scorcia @ 2026-06-20 19:56 UTC (permalink / raw)
To: linux-mediatek
Cc: Luca Leonardo Scorcia, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sen Chu, Sean Wang,
Macpaul Lin, Lee Jones, Matthias Brugger,
AngeloGioacchino Del Regno, Liam Girdwood, Mark Brown,
Linus Walleij, Louis-Alexis Eyraud, Val Packett, Julien Massot,
Fabien Parent, Akari Tsuyukusa, Chen Zhong, linux-input,
devicetree, linux-kernel, linux-pm, linux-arm-kernel, linux-gpio
The MediaTek MT6392 PMIC is usually found on devices powered by
the MT8516/MT8167 SoC and is yet another MT6323/MT6397 variant.
This series is mostly based around patches submitted a couple
years ago by Fabien Parent and not merged and from Val Packett's
submission from Jan 2025 that included extra cleanups, fixes, and a
new dtsi file similar to ones that exist for other PMICs. Some
comments weren't addressed and the series was ultimately not merged.
These patches enable four functions: keys, regulator, pinctrl and RTC.
Mono speaker amp will follow later as I need to work further on the
audio codec.
I added a handful of device tree improvements to fix some dtbs_check
errors, added support for the pinctrl device and addressed the comments
from last year's reviews.
Please note that patch 0006 and 0008 depend on patch 0005 as they need the
registers.h file, but belong to different driver areas. I'm not sure if
I'm supposed to squash them even if they belong to different driver
areas of if it's fine like this. Any advice is welcome.
Patch 0009 also depends on patch 0003 because of mt6392-regulator.h.
The series has been tested on Xiaomi Mi Smart Clock X04G and on the
Lenovo Smart Clock 2 CD-24502F.
Changes in v8:
From reviewers:
- Added example code to the MFD device binding, removed it from the
regulators docs.
- Added minItems/maxItems constraints on the regulator mode definitions,
improved the mode constants.
- Fixed formatting issues in the regulator binding.
- Import the mt6392.dtsi file in pumpkin-common.dtsi, as it was originally
meant in [8].
From sashiko:
- Added more explicit constraints on the regulator modes definitions.
- Use the appropriate modeget register for LDO regulators, Buck registers
don't have the corresponding register according to the data sheet.
- Added the missing of_map_mode function.
- Removed some debugging code that had no use and masked error codes.
Changes in v7 [7]:
- Removed patch 0008 dependency on patch 0003.
- Reintroduced the regulator driver. In earlier revisions of this series,
it was proposed to remove the dedicated compatible for the regulator
device [3]. The driver does not use actually it, but it is not possible
at this time to remove it from the bindings since it's a required
property.
Making the regulator-required property conditional was NACKed in [5],
with the suggestion to create a separate binding altogether for devices
that do not require the compatible property. I tried implementing this,
but since the parent device needs to be declared as compatible with
mt6323, it leads to a warning in dt_binding_check since mt6323 would
be declared as a compatible in both mt6392 and mt6397.
In the end the only regulator driver from the mt6397 documentation that
still declares an of_match is mt6397-regulator and it does not seem
to be necessary, so it should be possible to remove it and make the
regulator compatible optional for all regulators, but that change would
probably deserve its own separate patch series.
Changes in v6 [6]:
- Dropped the regulators driver for the moment
- Explained the FCHR key name origin in the commit message
- Introduced the MFD_CELL_* macro in the sub-devices definitions.
A separate, independent commit introduced MFD_CELL_* to all the
subdevices in the mt6397-core.c file for consistency
- Replaced of_device_get_match_data with device_get_match_data
- Removed the mfd_match_data enum in favor of the preexisting
chip_id enum
- Adjusted the error message if the device is unsupported
Changes in v5 [5]:
- Double checked regulator driver with data sheet and Android sources.
The data sheet I have misses a lot of register descriptions, but
Android sources have been helpful to fill the gaps
- Reintroduced the required attribute for the regulator compatible
in the bindings
- Fixed the missing reference to the MT6392 schema
- Fixed casts/unused vars reported by kernel test robot
- Removed Reviewed-by tags from the regulator patches as they have been
modified in this version
Changes in v4 [4]:
- Dropped usage of the regulator compatible
- Fixed commit messages text to properly reference the target subsystem
- Added supply rails to the regulator
- Reworked the regulator schema and PMIC dtsi. Now all supplies are
documented and the schema no longer includes voltage information
- Removed redundant ldo- / buck- prefixes
- Renamed the pinfunc header to mediatek,mt6392-pinfunc.h
- Modified the MFD driver to use a simple identifier in the of_match
data properties
Changes in v3 [3]:
- Added pinctrl device
- Changed mt6397-rtc fallback to mt6323-rtc
- Added schema for regulators
- Fixed checkpatch issues
Changes in v2 [2]:
- Replaced explicit compatibles with fallbacks
Initial version: [1]
[1] https://lore.kernel.org/linux-mediatek/cover.1771865014.git.l.scorcia@gmail.com/
[2] https://lore.kernel.org/linux-mediatek/20260306120521.163654-1-l.scorcia@gmail.com/
[3] https://lore.kernel.org/linux-mediatek/20260317184507.523060-1-l.scorcia@gmail.com/
[4] https://lore.kernel.org/linux-mediatek/20260330083429.359819-1-l.scorcia@gmail.com/
[5] https://lore.kernel.org/linux-mediatek/20260420213529.1645560-1-l.scorcia@gmail.com/
[6] https://lore.kernel.org/linux-mediatek/20260612200717.361018-1-l.scorcia@gmail.com/
[7] https://lore.kernel.org/linux-mediatek/20260615071836.362883-1-l.scorcia@gmail.com/
[8] https://lore.kernel.org/linux-mediatek/20190323211612.860-25-fparent@baylibre.com/
Fabien Parent (3):
dt-bindings: input: mtk-pmic-keys: Add MT6392 PMIC keys
mfd: mt6397: Add support for MT6392 PMIC
regulator: Add MediaTek MT6392 regulator
Luca Leonardo Scorcia (4):
dt-bindings: mfd: mt6397: Add MT6392 PMIC
regulator: dt-bindings: Add MediaTek MT6392 PMIC
mfd: mt6397: Use MFD_CELL_* to describe sub-devices
pinctrl: mediatek: mt6397: Add MediaTek MT6392
Val Packett (2):
input: keyboard: mtk-pmic-keys: Add MT6392 support
arm64: dts: mediatek: Add MediaTek MT6392 PMIC dtsi
.../bindings/input/mediatek,pmic-keys.yaml | 1 +
.../bindings/mfd/mediatek,mt6397.yaml | 74 ++
.../regulator/mediatek,mt6392-regulator.yaml | 118 +++
arch/arm64/boot/dts/mediatek/mt6392.dtsi | 148 ++++
.../boot/dts/mediatek/pumpkin-common.dtsi | 2 +
drivers/input/keyboard/mtk-pmic-keys.c | 17 +
drivers/mfd/mt6397-core.c | 295 ++++---
drivers/mfd/mt6397-irq.c | 8 +
drivers/pinctrl/mediatek/pinctrl-mt6397.c | 37 +-
drivers/pinctrl/mediatek/pinctrl-mtk-mt6392.h | 64 ++
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/mt6392-regulator.c | 764 ++++++++++++++++++
.../regulator/mediatek,mt6392-regulator.h | 23 +
include/linux/mfd/mt6392/core.h | 43 +
include/linux/mfd/mt6392/registers.h | 488 +++++++++++
include/linux/mfd/mt6397/core.h | 1 +
include/linux/regulator/mt6392-regulator.h | 42 +
18 files changed, 1973 insertions(+), 162 deletions(-)
create mode 100644 Documentation/devicetree/bindings/regulator/mediatek,mt6392-regulator.yaml
create mode 100644 arch/arm64/boot/dts/mediatek/mt6392.dtsi
create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt6392.h
create mode 100644 drivers/regulator/mt6392-regulator.c
create mode 100644 include/dt-bindings/regulator/mediatek,mt6392-regulator.h
create mode 100644 include/linux/mfd/mt6392/core.h
create mode 100644 include/linux/mfd/mt6392/registers.h
create mode 100644 include/linux/regulator/mt6392-regulator.h
--
2.43.0
^ permalink raw reply
* psmouse: missing pnpID?
From: John Ogness @ 2026-06-20 16:39 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input
Hi,
I have an HP laptop, running a fresh install of Debian/trixie and I see
the boot message:
psmouse serio1: synaptics: Your touchpad (PNP: SYN3286 PNP0f13) says
it can support a different bus. If i2c-hid and hid-rmi are not used,
you might want to try setting psmouse.synaptics_intertouch to 1 and
report this to linux-input@vger.kernel.org.
Indeed, touchpad and keyboard do not work unless I add
psmouse.synaptics_intertouch=1 as a boot argument. I am happy using this
boot argument, but it sounds like you are trying to collect some data on
this. Please let me know if I should provide any specific data.
John Ogness
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/2] selftests/hid: Cover hid_bpf_get_data() size overflow
From: bot+bpf-ci @ 2026-06-20 15:11 UTC (permalink / raw)
To: chenyy23, jikos, bentiss, bpf, linux-input
Cc: chenyy23, shuah, ast, daniel, andrii, martin.lau, eddyz87, memxor,
song, yonghong.song, jolsa, linux-kselftest, linux-kernel, ast,
andrii, daniel, martin.lau, eddyz87, yonghong.song, clm,
ihor.solodrai
In-Reply-To: <b9c4b268b403e2b41362dd678dca8feb35d34a9c.1781964949.git.chenyy23@mails.tsinghua.edu.cn>
[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]
> selftests/hid: Cover hid_bpf_get_data() size overflow
>
> Add a HID-BPF regression check for hid_bpf_get_data() requests whose
> size would overflow when added to the offset.
>
> The new rdesc fixup callback asks for offset 2 and size ~0ULL, then
> records whether the helper returns NULL. A vulnerable kernel returns a
> non-NULL pointer because the runtime check wraps the addition. A fixed
> kernel rejects the request. The test only checks the helper result and
> does not dereference the returned pointer.
>
> Also add KHDR_INCLUDES to the HID selftest build so hid_bpf.c sees the
> current kernel UAPI HID definitions on systems whose installed headers do
> not provide enum hid_report_type.
>
> Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
This adds a regression test for the hid_bpf_get_data() size-overflow
bug, where the range check wrapped on a large size and returned an
out-of-bounds pointer.
That wrapping check was introduced in 658ee5a64fcf, and the fix commit
carries a Fixes: tag for it. Should this test commit carry the same tag?
Fixes: 658ee5a64fcf ("HID: bpf: allocate data memory for device_event BPF programs")
> diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> index 5ecc845ef792..b21fbb13c926 100644
> --- a/tools/testing/selftests/hid/progs/hid.c
> +++ b/tools/testing/selftests/hid/progs/hid.c
> @@ -240,6 +241,20 @@ struct hid_bpf_ops rdesc_fixup = {
> .hid_rdesc_fixup = (void *)hid_rdesc_fixup,
> };
>
> +SEC("?struct_ops.s/hid_rdesc_fixup")
> +int BPF_PROG(hid_rdesc_fixup_get_data_overflow, struct hid_bpf_ctx *hid_ctx)
> +{
> + if (!hid_bpf_get_data(hid_ctx, 2 /* offset */, ~0ULL /* size */))
> + get_data_overflow_check = 1;
> +
> + return 0;
> +}
This was raised on an earlier posting and does not look addressed:
the callback stores the result in the get_data_overflow_check global
and always returns 0, while the userspace test asserts on that global
rather than on the callback return value.
Could the callback just return the helper result so the test can check
the return value directly, instead of routing it through a separate
global?
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/27874238868
^ permalink raw reply
* [PATCH bpf-next v2 2/2] selftests/hid: Cover hid_bpf_get_data() size overflow
From: Yiyang Chen @ 2026-06-20 14:22 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, bpf, linux-input
Cc: Yiyang Chen, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Song Liu, Yonghong Song, Jiri Olsa,
linux-kselftest, linux-kernel
In-Reply-To: <cover.1781964949.git.chenyy23@mails.tsinghua.edu.cn>
Add a HID-BPF regression check for hid_bpf_get_data() requests whose
size would overflow when added to the offset.
The new rdesc fixup callback asks for offset 2 and size ~0ULL, then
records whether the helper returns NULL. A vulnerable kernel returns a
non-NULL pointer because the runtime check wraps the addition. A fixed
kernel rejects the request. The test only checks the helper result and
does not dereference the returned pointer.
Also add KHDR_INCLUDES to the HID selftest build so hid_bpf.c sees the
current kernel UAPI HID definitions on systems whose installed headers do
not provide enum hid_report_type.
Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
---
tools/testing/selftests/hid/Makefile | 2 +-
tools/testing/selftests/hid/hid_bpf.c | 11 +++++++++++
tools/testing/selftests/hid/progs/hid.c | 15 +++++++++++++++
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/hid/Makefile b/tools/testing/selftests/hid/Makefile
index 50ec9e0406aba..357c6eb5ff5ee 100644
--- a/tools/testing/selftests/hid/Makefile
+++ b/tools/testing/selftests/hid/Makefile
@@ -24,7 +24,7 @@ CXX ?= $(CROSS_COMPILE)g++
HOSTPKG_CONFIG := pkg-config
-CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(OUTPUT)
+CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(OUTPUT) $(KHDR_INCLUDES)
CFLAGS += -I$(OUTPUT)/tools/include
LDLIBS += -lelf -lz -lrt -lpthread
diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index 1e979fb3542ba..f0a210900e63d 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -887,6 +887,17 @@ TEST_F(hid_bpf, test_rdesc_fixup)
ASSERT_EQ(rpt_desc.value[4], 0x42);
}
+TEST_F(hid_bpf, test_rdesc_fixup_get_data_overflow)
+{
+ const struct test_program progs[] = {
+ { .name = "hid_rdesc_fixup_get_data_overflow" },
+ };
+
+ LOAD_PROGRAMS(progs);
+
+ ASSERT_EQ(self->skel->bss->get_data_overflow_check, 1);
+}
+
static int libbpf_print_fn(enum libbpf_print_level level,
const char *format, va_list args)
{
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index 5ecc845ef7921..b21fbb13c926f 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -13,6 +13,7 @@ struct attach_prog_args {
__u64 callback_check = 52;
__u64 callback2_check = 52;
+__u64 get_data_overflow_check;
SEC("?struct_ops/hid_device_event")
int BPF_PROG(hid_first_event, struct hid_bpf_ctx *hid_ctx, enum hid_report_type type)
@@ -240,6 +241,20 @@ struct hid_bpf_ops rdesc_fixup = {
.hid_rdesc_fixup = (void *)hid_rdesc_fixup,
};
+SEC("?struct_ops.s/hid_rdesc_fixup")
+int BPF_PROG(hid_rdesc_fixup_get_data_overflow, struct hid_bpf_ctx *hid_ctx)
+{
+ if (!hid_bpf_get_data(hid_ctx, 2 /* offset */, ~0ULL /* size */))
+ get_data_overflow_check = 1;
+
+ return 0;
+}
+
+SEC(".struct_ops.link")
+struct hid_bpf_ops rdesc_fixup_get_data_overflow = {
+ .hid_rdesc_fixup = (void *)hid_rdesc_fixup_get_data_overflow,
+};
+
SEC("?struct_ops/hid_device_event")
int BPF_PROG(hid_test_insert1, struct hid_bpf_ctx *hid_ctx, enum hid_report_type type)
{
--
2.34.1
^ permalink raw reply related
* [PATCH bpf-next v2 0/2] HID: bpf: Fix hid_bpf_get_data() range check
From: Yiyang Chen @ 2026-06-20 14:22 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, bpf, linux-input
Cc: Yiyang Chen, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Song Liu, Yonghong Song, Jiri Olsa,
linux-kselftest, linux-kernel
In-Reply-To: <cover.1781627122.git.chenyy23@mails.tsinghua.edu.cn>
hid_bpf_get_data() exposes a pointer into the HID-BPF context data when
the caller-provided offset and size fit inside ctx->allocated_size.
The helper currently checks that range with:
rdwr_buf_size + offset > ctx->allocated_size
Since both operands are unsigned, a very large size can wrap the sum and
make an out-of-range request look valid.
Patch 1 changes the helper to reject offset values beyond the allocation
and then compare the requested size against the remaining bytes.
Patch 2 adds a HID-BPF regression check that asks hid_bpf_get_data() for
offset 2 and size ~0ULL from an rdesc_fixup callback and expects NULL.
It also adds KHDR_INCLUDES to the HID selftest build so the userspace
test sees current kernel UAPI HID definitions.
Validation, rebased and tested on bpf-next master e4287bf34f97
("selftests/bpf: Work around llvm stack overflow in crypto progs"):
git diff --check e4287bf34f97..HEAD: OK
scripts/checkpatch.pl --strict -g e4287bf34f97..HEAD: OK
make O=/root/ebpf-verifier-bug-detection/kernel-build/bpf-next-hidbpf-20260616 \
drivers/hid/bpf/hid_bpf_dispatch.o: OK
make -C tools/testing/selftests/hid \
O=/root/ebpf-verifier-bug-detection/kernel-build/bpf-next-hidbpf-20260616 \
OUTPUT=/tmp/hid-selftest-026-v2 \
VMLINUX_BTF=/root/ebpf-verifier-bug-detection/kernel-build/bpf-next-hidbpf-20260616/vmlinux \
KHDR_INCLUDES=-isystem /root/ebpf-verifier-bug-detection/kernel-build/bpf-next-hidbpf-20260616/usr/include \
hid_bpf: OK
Changes in v2:
- Drop the temporary data variable around the overflow
hid_bpf_get_data() call in the selftest callback.
- Correct the Fixes tag to commit 658ee5a64fcf ("HID: bpf: allocate
data memory for device_event BPF programs").
v1: https://lore.kernel.org/bpf/cover.1781627122.git.chenyy23@mails.tsinghua.edu.cn/
Yiyang Chen (2):
HID: bpf: Fix hid_bpf_get_data() range check
selftests/hid: Cover hid_bpf_get_data() size overflow
drivers/hid/bpf/hid_bpf_dispatch.c | 3 ++-
tools/testing/selftests/hid/Makefile | 2 +-
tools/testing/selftests/hid/hid_bpf.c | 11 +++++++++++
tools/testing/selftests/hid/progs/hid.c | 15 +++++++++++++++
4 files changed, 29 insertions(+), 2 deletions(-)
base-commit: e4287bf34f97a88c7d9322f5bde828724c073a6b
--
2.34.1
^ permalink raw reply
* [PATCH bpf-next v2 1/2] HID: bpf: Fix hid_bpf_get_data() range check
From: Yiyang Chen @ 2026-06-20 14:22 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, bpf, linux-input
Cc: Yiyang Chen, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Song Liu, Yonghong Song, Jiri Olsa,
linux-kselftest, linux-kernel
In-Reply-To: <cover.1781964949.git.chenyy23@mails.tsinghua.edu.cn>
hid_bpf_get_data() returns a pointer into the HID-BPF context data when
the caller-provided offset and size fit inside ctx->allocated_size.
The current check adds rdwr_buf_size and offset before comparing the
result against ctx->allocated_size. Since both values are unsigned, a
very large size can wrap the sum below ctx->allocated_size and make the
helper return a pointer even though the requested range is not contained
in the backing buffer.
Use a non-wrapping range check instead: reject offsets beyond the
allocation, then compare the requested size with the remaining bytes
after the offset.
Fixes: 658ee5a64fcf ("HID: bpf: allocate data memory for device_event BPF programs")
Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
---
drivers/hid/bpf/hid_bpf_dispatch.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index d0130658091b0..09b45c40d84f0 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -299,7 +299,8 @@ hid_bpf_get_data(struct hid_bpf_ctx *ctx, unsigned int offset, const size_t rdwr
ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);
- if (rdwr_buf_size + offset > ctx->allocated_size)
+ if (offset > ctx->allocated_size ||
+ rdwr_buf_size > ctx->allocated_size - offset)
return NULL;
return ctx_kern->data + offset;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 RESEND] HID: appletb-kbd: add option to switch default layer on double pressing fn key
From: sashiko-bot @ 2026-06-20 11:40 UTC (permalink / raw)
To: Aditya Garg; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260620112635.17246-1-gargaditya08@proton.me>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Medium] The driver dynamically mutates a read-only global module parameter without synchronization.
- [Medium] Zero-initialization of `kbd->last_fn_press` paired with jiffies wrap logic causes spurious double-press detections during the first 5 minutes of system uptime.
Pre-existing issues:
- [High] Calling sleeping functions (`hid_hw_power`, `pm_runtime_get_sync`) from the atomic input event handler causes 'BUG: scheduling while atomic'.
- [High] Probe enables hardware events before setting driver data, causing a NULL pointer dereference.
--
commit e33a32d065a3f43246ae8063aed73e4573a2176b
Author: Aditya Garg <gargaditya08@proton.me>
HID: appletb-kbd: add option to switch default layer on double pressing fn key
This patch enables a user to switch the default layer from media to fn
keys and vice-versa upon double pressing the fn key. This behaviour can
be configured using the double_press_switch_time module parameter whose
value depicts the time in milliseconds within which the fn key must be
pressed again to switch the default layer. If set to 0, it simply
disables this behaviour.
> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
> index 462010a758993..34bfc595949dc 100644
> --- a/drivers/hid/hid-appletb-kbd.c
> +++ b/drivers/hid/hid-appletb-kbd.c
[ ... ]
> @@ -250,15 +269,42 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
> unsigned int code, int value)
> {
> struct appletb_kbd *kbd = handle->private;
>
> reset_inactivity_timer(kbd);
>
> - if (type == EV_KEY && code == KEY_FN && appletb_tb_fn_toggle &&
> - (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
> - kbd->current_mode == APPLETB_KBD_MODE_FN)) {
> + if (type == EV_KEY && code == KEY_FN &&
> + (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
> + kbd->current_mode == APPLETB_KBD_MODE_FN)) {
> +
> if (value == 1) {
> - kbd->saved_mode = kbd->current_mode;
> - appletb_kbd_set_mode(kbd, kbd->current_mode == APPLETB_KBD_MODE_SPCL
> - ? APPLETB_KBD_MODE_FN : APPLETB_KBD_MODE_SPCL);
> + if (appletb_tb_double_press_switch_layers > 0) {
> + unsigned long now = jiffies;
> +
> + if (time_before(now, kbd->last_fn_press +
> + msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {
[Severity: Medium]
Since the system jiffies timer initializes to a negative value at boot, does
evaluating time_before() with a zero-initialized kbd->last_fn_press cause
spurious double-press detections during the first 5 minutes of system uptime?
> +
> + appletb_tb_def_mode =
> + appletb_switch_mode(
> + appletb_tb_def_mode);
[Severity: Medium]
Since appletb_tb_def_mode is a global module parameter with read-only
permissions, does dynamically reassigning it here violate its read-only
contract and globally modify the default layer for all device instances
instead of applying the state locally to the specific kbd instance?
> +
> + appletb_kbd_set_mode(kbd,
> + appletb_tb_def_mode);
[Severity: High]
This isn't a bug introduced by this patch, but does appletb_kbd_set_mode()
unconditionally call hid_hw_power(), which can sleep via pm_runtime_get_sync()?
Since the input core invokes appletb_kbd_inp_event() from atomic context with
the dev->event_lock spinlock held and local IRQs disabled, can this trigger a
scheduling while atomic bug?
> +
> + kbd->saved_mode = appletb_tb_def_mode;
> + kbd->last_fn_press = 0;
[Severity: Medium]
Does explicitly resetting last_fn_press back to 0 here cause the spurious
evaluations to continue for subsequent presses within the first 5 minutes of
uptime?
> +
> + return;
> + }
> +
> + kbd->last_fn_press = now;
> + }
> + if (appletb_tb_fn_toggle) {
> + kbd->saved_mode = kbd->current_mode;
> +
> + appletb_kbd_set_mode(kbd,
> + appletb_switch_mode(kbd->current_mode));
> + }
> +
> } else if (value == 0) {
> - if (kbd->saved_mode != kbd->current_mode)
> + if (appletb_tb_fn_toggle &&
> + kbd->saved_mode != kbd->current_mode)
> appletb_kbd_set_mode(kbd, kbd->saved_mode);
> }
> }
[Severity: High]
This is a pre-existing issue, but does appletb_kbd_probe() enable hardware
events before setting the driver data?
drivers/hid/hid-appletb-kbd.c:appletb_kbd_probe() {
...
ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
...
hid_set_drvdata(hdev, kbd);
...
}
Once the hardware is opened, it can immediately begin delivering HID reports
which invoke appletb_kbd_hid_event(). Could appletb_kbd_hid_event() fetch an
uninitialized NULL pointer from hid_get_drvdata(hdev) and pass it to
reset_inactivity_timer(), causing a NULL pointer dereference?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260620112635.17246-1-gargaditya08@proton.me?part=1
^ permalink raw reply
* [PATCH v2 RESEND] HID: appletb-kbd: add option to switch default layer on double pressing fn key
From: Aditya Garg @ 2026-06-20 11:26 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Linux Input Mailing List, Linux Kernel Mailing List
From: Aditya Garg <gargaditya08@proton.me>
This patch enables a user to switch the default layer from media to fn
keys and vice-versa upon double pressing the fn key. This behaviour can
be configured using the double_press_switch_time module parameter whose
value depicts the time in milliseconds within which the fn key must be
pressed again to switch the default layer. If set to 0, it simply
disables this behaviour.
Signed-off-by: Aditya Garg <gargaditya08@proton.me>
---
v2: added a check to ensure negative fn switch times are ignored.
drivers/hid/hid-appletb-kbd.c | 60 +++++++++++++++++++++++++++++++----
1 file changed, 53 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
index 462010a75899..34bfc595949d 100644
--- a/drivers/hid/hid-appletb-kbd.c
+++ b/drivers/hid/hid-appletb-kbd.c
@@ -56,6 +56,12 @@ static int appletb_tb_idle_timeout = 15;
module_param_named(idle_timeout, appletb_tb_idle_timeout, int, 0644);
MODULE_PARM_DESC(idle_timeout, "Idle timeout in sec");
+static int appletb_tb_double_press_switch_layers;
+module_param_named(double_press_switch_time,
+ appletb_tb_double_press_switch_layers, int, 0644);
+MODULE_PARM_DESC(double_press_switch_time, "Time in ms within which if fn key is double "
+ "pressed will switch layers");
+
struct appletb_kbd {
struct hid_field *mode_field;
struct input_handler inp_handler;
@@ -68,6 +74,7 @@ struct appletb_kbd {
bool has_turned_off;
u8 saved_mode;
u8 current_mode;
+ unsigned long last_fn_press;
};
static const struct key_entry appletb_kbd_keymap[] = {
@@ -243,6 +250,18 @@ static int appletb_kbd_hid_event(struct hid_device *hdev, struct hid_field *fiel
return kbd->current_mode == APPLETB_KBD_MODE_OFF;
}
+static u8 appletb_switch_mode(u8 mode)
+{
+ switch (mode) {
+ case APPLETB_KBD_MODE_SPCL:
+ return APPLETB_KBD_MODE_FN;
+ case APPLETB_KBD_MODE_FN:
+ return APPLETB_KBD_MODE_SPCL;
+ default:
+ return mode;
+ }
+}
+
static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type,
unsigned int code, int value)
{
@@ -250,15 +269,42 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
reset_inactivity_timer(kbd);
- if (type == EV_KEY && code == KEY_FN && appletb_tb_fn_toggle &&
- (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
- kbd->current_mode == APPLETB_KBD_MODE_FN)) {
+ if (type == EV_KEY && code == KEY_FN &&
+ (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
+ kbd->current_mode == APPLETB_KBD_MODE_FN)) {
+
if (value == 1) {
- kbd->saved_mode = kbd->current_mode;
- appletb_kbd_set_mode(kbd, kbd->current_mode == APPLETB_KBD_MODE_SPCL
- ? APPLETB_KBD_MODE_FN : APPLETB_KBD_MODE_SPCL);
+ if (appletb_tb_double_press_switch_layers > 0) {
+ unsigned long now = jiffies;
+
+ if (time_before(now, kbd->last_fn_press +
+ msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {
+
+ appletb_tb_def_mode =
+ appletb_switch_mode(
+ appletb_tb_def_mode);
+
+ appletb_kbd_set_mode(kbd,
+ appletb_tb_def_mode);
+
+ kbd->saved_mode = appletb_tb_def_mode;
+ kbd->last_fn_press = 0;
+
+ return;
+ }
+
+ kbd->last_fn_press = now;
+ }
+ if (appletb_tb_fn_toggle) {
+ kbd->saved_mode = kbd->current_mode;
+
+ appletb_kbd_set_mode(kbd,
+ appletb_switch_mode(kbd->current_mode));
+ }
+
} else if (value == 0) {
- if (kbd->saved_mode != kbd->current_mode)
+ if (appletb_tb_fn_toggle &&
+ kbd->saved_mode != kbd->current_mode)
appletb_kbd_set_mode(kbd, kbd->saved_mode);
}
}
Range-diff against v1:
1: 5c9d8c8ba8c0 ! 1: 4f6a818b98b9 HID: appletb-kbd: add option to switch default layer on double pressing fn key
@@ drivers/hid/hid-appletb-kbd.c: static void appletb_kbd_inp_event(struct input_ha
- kbd->saved_mode = kbd->current_mode;
- appletb_kbd_set_mode(kbd, kbd->current_mode == APPLETB_KBD_MODE_SPCL
- ? APPLETB_KBD_MODE_FN : APPLETB_KBD_MODE_SPCL);
-+ if (appletb_tb_double_press_switch_layers) {
++ if (appletb_tb_double_press_switch_layers > 0) {
+ unsigned long now = jiffies;
+
+ if (time_before(now, kbd->last_fn_press +
--
2.53.0
^ permalink raw reply related
* [BUG] Get Kernel OOPS when detaching the stylus from laptop
From: Eric LIn @ 2026-06-19 18:07 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, linux-kernel
Dear Linux kernel maintainers:
I got a BUG after I upgrade the kernel to 7.1, when I detached the
stylus from my laptop, I got these kernel OOPS
```
[ 67.724440] BUG: unable to handle page fault for address: ffffffffffffffe4
[ 67.724455] #PF: supervisor read access in kernel mode
[ 67.724459] #PF: error_code(0x0000) - not-present page
[ 67.724463] PGD 642815067 P4D 642815067 PUD 642817067 PMD 0
[ 67.724471] Oops: Oops: 0000 [#1] SMP NOPTI
[ 67.724477] CPU: 2 UID: 0 PID: 574 Comm: irq/108-CUST000 Not
tainted 7.1.0-2-cachyos #1 PREEMPT(full)
aa2fc21b834663bf803613ae89f5425cc0ec2a9b
[ 67.724485] Hardware name: Micro-Star International Co., Ltd.
Prestige 16 Flip AI+ C3MTG/MS-2622, BIOS E2622IMS.117 04/27/2026
[ 67.724488] RIP: 0010:hidinput_hid_event+0x5c/0x750
[ 67.724501] Code: 4c 8b 76 68 41 83 f8 16 75 73 41 8b 76 30 8b 02
48 81 c7 20 1c 00 00 48 89 fb 0f 1f 40 00 48 8b 1b 48 39 fb 0f 84 af
06 00 00 <39> 73 e4 75 ef 48 83 fb 38 0f 84 a0 06 00 00 3d 44 00 85 00
0f 85
[ 67.724505] RSP: 0018:ffffd28ac1283cc8 EFLAGS: 00010203
[ 67.724509] RAX: 00000000000d003b RBX: 0000000000000000 RCX: 0000000000000000
[ 67.724512] RDX: ffff8d73c83d7e88 RSI: 0000000000000007 RDI: ffff8d73c6415c20
[ 67.724514] RBP: 0000000000000000 R08: 0000000000000016 R09: 0000000000000003
[ 67.724516] R10: 0000000000000000 R11: ffffffffc070a6c0 R12: 0000000000000001
[ 67.724518] R13: ffffffffc08c70d0 R14: ffff8d73c5bb4000 R15: ffff8d73c83d7e00
[ 67.724521] FS: 0000000000000000(0000) GS:ffff8d7b7f6fb000(0000)
knlGS:0000000000000000
[ 67.724525] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 67.724527] CR2: ffffffffffffffe4 CR3: 0000000642812005 CR4: 0000000108f72ef0
[ 67.724531] PKRU: 55555554
[ 67.724532] Call Trace:
[ 67.724538] <TASK>
[ 67.724550] hid_process_event+0x10e/0x160
[ 67.724558] hid_report_raw_event+0x763/0xac0
[ 67.724565] __hid_input_report.llvm.4774680079643314489+0x219/0x290
[ 67.724571] ? __pfx_irq_thread_fn+0x10/0x10
[ 67.724578] hid_safe_input_report+0x14/0x20
[ 67.724587] i2c_hid_irq+0x137/0x1c0 [i2c_hid
93a6a330bc5a69c91125ab6f51a82509be2e9c49]
[ 67.724598] irq_thread_fn+0x24/0x50
[ 67.724603] irq_thread+0x190/0x2a0
[ 67.724606] ? __pfx_irq_thread_dtor+0x10/0x10
[ 67.724609] ? __pfx_irq_thread+0x10/0x10
[ 67.724612] kthread+0xfb/0x120
[ 67.724619] ? __pfx_kthread+0x10/0x10
[ 67.724622] ret_from_fork+0xee/0x260
[ 67.724628] ? __pfx_kthread+0x10/0x10
[ 67.724631] ret_from_fork_asm+0x1a/0x30
[ 67.724641] </TASK>
[ 67.724642] Modules linked in: snd_seq_dummy rfcomm snd_hrtimer
snd_seq snd_seq_device ccm algif_aead des_generic libdes md4
nft_reject_inet nf_reject_ipv6 nf_reject_ipv4 nft_reject nft_limit
nft_ct nf_conntrack uhid nf_defrag_ipv6 nf_defrag_ipv4 algif_hash
algif_skcipher af_alg nf_tables bnep nfnetlink snd_ctl_led
snd_soc_sof_sdw snd_sof_probes snd_soc_intel_hda_dsp_common f2fs vfat
lz4hc_compress fat lz4_compress snd_soc_rt1320_sdw snd_soc_rt712_sdca
snd_hda_codec_intelhdmi regmap_sdw_mbq snd_hda_codec_hdmi regmap_sdw
snd_soc_dmic snd_hda_intel snd_sof_pci_intel_ptl snd_sof_pci_intel_lnl
snd_sof_pci_intel_mtl snd_sof_intel_hda_generic soundwire_intel
soundwire_cadence snd_sof_intel_hda_sdw_bpt snd_sof_intel_hda_common
snd_soc_hdac_hda snd_sof_intel_hda snd_sof_intel_hda_mlink snd_sof_pci
snd_sof_xtensa_dsp snd_sof snd_sof_utils snd_hda_ext_core
intel_rapl_msr intel_uncore_frequency snd_hda_codec
intel_uncore_frequency_common snd_hda_core intel_tcc_cooling
snd_soc_acpi_intel_match x86_pkg_temp_thermal
[ 67.724717] snd_soc_acpi_intel_sdca_quirks snd_intel_dspcfg
intel_powerclamp snd_intel_sdw_acpi soundwire_generic_allocation
coretemp snd_soc_sdw_utils snd_soc_acpi iwlmld snd_hwdep kvm_intel
soundwire_bus mac80211 snd_soc_sdca kvm uvcvideo snd_soc_core ptp
processor_thermal_device_pci uvc ac97_bus processor_thermal_device
pps_core videobuf2_vmalloc snd_pcm_dmaengine
processor_thermal_power_floor snd_compress libarc4
processor_thermal_wt_hint videobuf2_memops spi_nor irqbypass
hid_sensor_custom_intel_hinge snd_pcm iTCO_wdt hid_sensor_als
hid_sensor_accel_3d hid_sensor_incl_3d hid_sensor_prox
hid_sensor_gyro_3d videobuf2_v4l2 hid_sensor_magn_3d
hid_sensor_rotation processor_thermal_wt_req intel_cstate mtd
intel_pmc_bxt mei_gsc_proxy snd_timer processor_thermal_rfim
hid_sensor_trigger videobuf2_common iwlwifi intel_uncore snd kfifo_buf
processor_thermal_mbox videodev btintel_pcie i2c_i801
hid_sensor_iio_common intel_pmc_core platform_temperature_control
soundcore spi_intel_pci i2c_smbus btintel pcspkr mc wmi_bmof
[ 67.724792] dptf_power msi_wmi int3403_thermal msi_wmi_platform
spi_intel crc8 i2c_mux industrialio intel_hid pmt_telemetry
processor_thermal_rapl int3400_thermal soc_button_array cfg80211
acpi_tad bluetooth sparse_keymap acpi_thermal_rel intel_rapl_common
acpi_pad mei_me processor_thermal_soc_slider pmt_discovery
intel_pmc_ssram_telemetry pmt_class rfkill mei intel_vpu igen6_edac
int340x_thermal_zone joydev mousedev mac_hid sch_fq_codel dm_crypt
encrypted_keys trusted tee asn1_encoder hid_sensor_custom
intel_ishtp_hid xe ucsi_acpi drm_gpuvm typec_ucsi drm_gpusvm_helper
roles drm_buddy gpu_sched hid_multitouch aesni_intel typec nvme
drm_suballoc_helper drm_exec gf128mul nvme_core drm_display_helper
hid_sensor_hub nvme_keyring aead cec video nvme_auth i2c_hid_acpi
i2c_algo_bit intel_lpss_pci i2c_hid wmi intel_ish_ipc drm_ttm_helper
intel_lpss thunderbolt pinctrl_intel_platform ttm idma64 intel_vsec
intel_ishtp serio_raw msi_ec dm_mirror dm_region_hash dm_log dm_mod
i2c_dev ec_sys crypto_user pkcs8_key_parser
[ 67.724882] CR2: ffffffffffffffe4
[ 67.724886] ---[ end trace 0000000000000000 ]---
[ 67.724890] RIP: 0010:hidinput_hid_event+0x5c/0x750
[ 67.724896] Code: 4c 8b 76 68 41 83 f8 16 75 73 41 8b 76 30 8b 02
48 81 c7 20 1c 00 00 48 89 fb 0f 1f 40 00 48 8b 1b 48 39 fb 0f 84 af
06 00 00 <39> 73 e4 75 ef 48 83 fb 38 0f 84 a0 06 00 00 3d 44 00 85 00
0f 85
[ 67.724899] RSP: 0018:ffffd28ac1283cc8 EFLAGS: 00010203
[ 67.724902] RAX: 00000000000d003b RBX: 0000000000000000 RCX: 0000000000000000
[ 67.724904] RDX: ffff8d73c83d7e88 RSI: 0000000000000007 RDI: ffff8d73c6415c20
[ 67.724906] RBP: 0000000000000000 R08: 0000000000000016 R09: 0000000000000003
[ 67.724908] R10: 0000000000000000 R11: ffffffffc070a6c0 R12: 0000000000000001
[ 67.724909] R13: ffffffffc08c70d0 R14: ffff8d73c5bb4000 R15: ffff8d73c83d7e00
[ 67.724911] FS: 0000000000000000(0000) GS:ffff8d7b7f6fb000(0000)
knlGS:0000000000000000
[ 67.724913] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 67.724916] CR2: ffffffffffffffe4 CR3: 0000000642812005 CR4: 0000000108f72ef0
[ 67.724918] PKRU: 55555554
[ 67.724920] note: irq/108-CUST000[574] exited with irqs disabled
[ 67.724937] Missing ENDBR: exit_shm+0x2c/0x200
[ 67.724946] ------------[ cut here ]------------
[ 67.724948] kernel BUG at arch/x86/kernel/cet.c:133!
[ 67.724953] Oops: invalid opcode: 0000 [#2] SMP NOPTI
[ 67.724958] CPU: 2 UID: 0 PID: 574 Comm: irq/108-CUST000 Tainted: G
D 7.1.0-2-cachyos #1 PREEMPT(full)
aa2fc21b834663bf803613ae89f5425cc0ec2a9b
[ 67.724963] Tainted: [D]=DIE
[ 67.724965] Hardware name: Micro-Star International Co., Ltd.
Prestige 16 Flip AI+ C3MTG/MS-2622, BIOS E2622IMS.117 04/27/2026
[ 67.724967] RIP: 0010:do_kernel_cp_fault+0xd7/0xe0
[ 67.724972] Code: 06 48 0f 43 ca 48 8d 0c 89 48 8d 14 4d 40 54 71
9e 48 89 c6 67 48 0f b9 3a 5b c3 cc cc cc cc cc 48 c7 43 50 00 00 00
00 eb 9c <0f> 0b cc cc cc cc cc cc cc 90 90 90 90 90 90 90 90 90 90 90
90 90
[ 67.724974] RSP: 0018:ffffd28ac1283d58 EFLAGS: 00010097
[ 67.724977] RAX: 0000000000000022 RBX: ffffd28ac1283d88 RCX: 5455e76f979d9500
[ 67.724979] RDX: 3fffffffffffefff RSI: 0000000000000002 RDI: ffff8d7b1f69d488
[ 67.724981] RBP: 0000000000000000 R08: 0000000000000fff R09: ffffffff9f25ca50
[ 67.724983] R10: 0000000000002ffd R11: 0000000000000000 R12: 0000000000000000
[ 67.724985] R13: 0000000000000000 R14: 0000000000000003 R15: 0000000000000000
[ 67.724987] FS: 0000000000000000(0000) GS:ffff8d7b7f6fb000(0000)
knlGS:0000000000000000
[ 67.724989] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 67.724990] CR2: ffffffffffffffe4 CR3: 0000000642812005 CR4: 0000000108f72ef0
[ 67.724992] PKRU: 55555554
[ 67.724994] Call Trace:
[ 67.724996] <TASK>
[ 67.724997] exc_control_protection+0x40/0x60
[ 67.725006] asm_fred_entrypoint_kernel+0x41/0x60
[ 67.725011] RIP: 0010:exit_shm+0x2c/0x200
[ 67.725014] Code: 40 d6 0f 1f 44 00 00 55 41 57 41 56 41 55 41 54
53 49 89 fe 48 8d 9f 60 0e 00 00 48 8d af 68 0d 00 00 48 89 df e8 f4
06 ad 00 <4d> 8b b6 68 0d 00 00 eb 34 66 66 2e 0f 1f 84 00 00 00 00 00
4c 89
[ 67.725016] RSP: 0018:ffffd28ac1283ea8 EFLAGS: 00010286
[ 67.725018] RAX: 0000000000000000 RBX: ffff8d73e95345e4 RCX: 0000000000000001
[ 67.725020] RDX: ffffd28ac1283e90 RSI: 0000000000000282 RDI: ffffd28ac1283e98
[ 67.725022] RBP: 0000000000000001 R08: 0000000000000fff R09: ffffffff9f25ca50
[ 67.725023] R10: 0000000000002ffd R11: ffffffff9d48f3fc R12: 0000000000000000
[ 67.725025] R13: ffff8d73e95345e0 R14: ffff8d73e9533780 R15: ffffffffa0030a00
[ 67.725028] ? exit_shm+0x2c/0x200
[ 67.725033] ? complete+0x1f/0x90
[ 67.725038] ? exit_shm+0x2c/0x200
[ 67.725041] ? task_work_run+0x9d/0xc0
[ 67.725046] do_exit+0x34f/0xa80
[ 67.725052] make_task_dead+0x80/0x150
[ 67.725056] rewind_stack_and_make_dead+0x16/0x20
[ 67.725061] RIP: 0000:0x0
[ 67.725162] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
[ 67.725164] RSP: 0000:0000000000000000 EFLAGS: 00000000 ORIG_RAX:
0000000000000000
[ 67.725169] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[ 67.725171] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
[ 67.725173] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[ 67.725175] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[ 67.725177] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 67.725181] </TASK>
[ 67.725183] Modules linked in: snd_seq_dummy rfcomm snd_hrtimer
snd_seq snd_seq_device ccm algif_aead des_generic libdes md4
nft_reject_inet nf_reject_ipv6 nf_reject_ipv4 nft_reject nft_limit
nft_ct nf_conntrack uhid nf_defrag_ipv6 nf_defrag_ipv4 algif_hash
algif_skcipher af_alg nf_tables bnep nfnetlink snd_ctl_led
snd_soc_sof_sdw snd_sof_probes snd_soc_intel_hda_dsp_common f2fs vfat
lz4hc_compress fat lz4_compress snd_soc_rt1320_sdw snd_soc_rt712_sdca
snd_hda_codec_intelhdmi regmap_sdw_mbq snd_hda_codec_hdmi regmap_sdw
snd_soc_dmic snd_hda_intel snd_sof_pci_intel_ptl snd_sof_pci_intel_lnl
snd_sof_pci_intel_mtl snd_sof_intel_hda_generic soundwire_intel
soundwire_cadence snd_sof_intel_hda_sdw_bpt snd_sof_intel_hda_common
snd_soc_hdac_hda snd_sof_intel_hda snd_sof_intel_hda_mlink snd_sof_pci
snd_sof_xtensa_dsp snd_sof snd_sof_utils snd_hda_ext_core
intel_rapl_msr intel_uncore_frequency snd_hda_codec
intel_uncore_frequency_common snd_hda_core intel_tcc_cooling
snd_soc_acpi_intel_match x86_pkg_temp_thermal
[ 67.725244] snd_soc_acpi_intel_sdca_quirks snd_intel_dspcfg
intel_powerclamp snd_intel_sdw_acpi soundwire_generic_allocation
coretemp snd_soc_sdw_utils snd_soc_acpi iwlmld snd_hwdep kvm_intel
soundwire_bus mac80211 snd_soc_sdca kvm uvcvideo snd_soc_core ptp
processor_thermal_device_pci uvc ac97_bus processor_thermal_device
pps_core videobuf2_vmalloc snd_pcm_dmaengine
processor_thermal_power_floor snd_compress libarc4
processor_thermal_wt_hint videobuf2_memops spi_nor irqbypass
hid_sensor_custom_intel_hinge snd_pcm iTCO_wdt hid_sensor_als
hid_sensor_accel_3d hid_sensor_incl_3d hid_sensor_prox
hid_sensor_gyro_3d videobuf2_v4l2 hid_sensor_magn_3d
hid_sensor_rotation processor_thermal_wt_req intel_cstate mtd
intel_pmc_bxt mei_gsc_proxy snd_timer processor_thermal_rfim
hid_sensor_trigger videobuf2_common iwlwifi intel_uncore snd kfifo_buf
processor_thermal_mbox videodev btintel_pcie i2c_i801
hid_sensor_iio_common intel_pmc_core platform_temperature_control
soundcore spi_intel_pci i2c_smbus btintel pcspkr mc wmi_bmof
[ 67.725309] dptf_power msi_wmi int3403_thermal msi_wmi_platform
spi_intel crc8 i2c_mux industrialio intel_hid pmt_telemetry
processor_thermal_rapl int3400_thermal soc_button_array cfg80211
acpi_tad bluetooth sparse_keymap acpi_thermal_rel intel_rapl_common
acpi_pad mei_me processor_thermal_soc_slider pmt_discovery
intel_pmc_ssram_telemetry pmt_class rfkill mei intel_vpu igen6_edac
int340x_thermal_zone joydev mousedev mac_hid sch_fq_codel dm_crypt
encrypted_keys trusted tee asn1_encoder hid_sensor_custom
intel_ishtp_hid xe ucsi_acpi drm_gpuvm typec_ucsi drm_gpusvm_helper
roles drm_buddy gpu_sched hid_multitouch aesni_intel typec nvme
drm_suballoc_helper drm_exec gf128mul nvme_core drm_display_helper
hid_sensor_hub nvme_keyring aead cec video nvme_auth i2c_hid_acpi
i2c_algo_bit intel_lpss_pci i2c_hid wmi intel_ish_ipc drm_ttm_helper
intel_lpss thunderbolt pinctrl_intel_platform ttm idma64 intel_vsec
intel_ishtp serio_raw msi_ec dm_mirror dm_region_hash dm_log dm_mod
i2c_dev ec_sys crypto_user pkcs8_key_parser
[ 67.725393] ---[ end trace 0000000000000000 ]---
[ 67.725395] RIP: 0010:hidinput_hid_event+0x5c/0x750
[ 67.725400] Code: 4c 8b 76 68 41 83 f8 16 75 73 41 8b 76 30 8b 02
48 81 c7 20 1c 00 00 48 89 fb 0f 1f 40 00 48 8b 1b 48 39 fb 0f 84 af
06 00 00 <39> 73 e4 75 ef 48 83 fb 38 0f 84 a0 06 00 00 3d 44 00 85 00
0f 85
[ 67.725403] RSP: 0018:ffffd28ac1283cc8 EFLAGS: 00010203
[ 67.725406] RAX: 00000000000d003b RBX: 0000000000000000 RCX: 0000000000000000
[ 67.725408] RDX: ffff8d73c83d7e88 RSI: 0000000000000007 RDI: ffff8d73c6415c20
[ 67.725410] RBP: 0000000000000000 R08: 0000000000000016 R09: 0000000000000003
[ 67.725413] R10: 0000000000000000 R11: ffffffffc070a6c0 R12: 0000000000000001
[ 67.725415] R13: ffffffffc08c70d0 R14: ffff8d73c5bb4000 R15: ffff8d73c83d7e00
[ 67.725417] FS: 0000000000000000(0000) GS:ffff8d7b7f6fb000(0000)
knlGS:0000000000000000
[ 67.725420] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 67.725422] CR2: ffffffffffffffd6 CR3: 0000000642812005 CR4: 0000000108f72ef0
[ 67.725424] PKRU: 55555554
[ 67.725426] note: irq/108-CUST000[574] exited with irqs disabled
[ 67.725458] Fixing recursive fault but reboot is needed!
```
I asked the tech support from CachyOS, They respond me this might be
caused by a upstream bug in this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=227312b4a65c
Here is my system setup:
- Device name: MSI Prestiage Flip 16 AI+ C3MTG
- Stylus: MSI Nano Pen (CUST0000:00 04F3:4516)
- Kernel version: 7.1.0
- CPU: Intel Core Ultra X7 358H
- Architecture: x86-64
Here are the logs of initializing the stylus:
```
[ 2.200612] input: CUST0000:00 04F3:4516 as
/devices/pci0000:00/0000:00:19.0/i2c_designware.0/i2c-0/i2c-CUST0000:00/0018:04F3:4516.0003/input/input19
[ 2.200696] input: CUST0000:00 04F3:4516 UNKNOWN as
/devices/pci0000:00/0000:00:19.0/i2c_designware.0/i2c-0/i2c-CUST0000:00/0018:04F3:4516.0003/input/input20
[ 2.200765] input: CUST0000:00 04F3:4516 Stylus as
/devices/pci0000:00/0000:00:19.0/i2c_designware.0/i2c-0/i2c-CUST0000:00/0018:04F3:4516.0003/input/input21
[ 2.200823] input: CUST0000:00 04F3:4516 UNKNOWN as
/devices/pci0000:00/0000:00:19.0/i2c_designware.0/i2c-0/i2c-CUST0000:00/0018:04F3:4516.0003/input/input23
[ 2.200895] input: CUST0000:00 04F3:4516 UNKNOWN as
/devices/pci0000:00/0000:00:19.0/i2c_designware.0/i2c-0/i2c-CUST0000:00/0018:04F3:4516.0003/input/input24
[ 2.200968] input: CUST0000:00 04F3:4516 UNKNOWN as
/devices/pci0000:00/0000:00:19.0/i2c_designware.0/i2c-0/i2c-CUST0000:00/0018:04F3:4516.0003/input/input25
[ 2.201017] input: CUST0000:00 04F3:4516 UNKNOWN as
/devices/pci0000:00/0000:00:19.0/i2c_designware.0/i2c-0/i2c-CUST0000:00/0018:04F3:4516.0003/input/input26
```
^ permalink raw reply
* [PATCH v3] Input: xpad - add support for Azeron devices
From: Roberts Kursitis @ 2026-06-19 14:22 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Roberts Kursitis
In-Reply-To: <20260619132821.18764-1-roberts.kursitis@azeron.eu>
Add USB Vendor and Product IDs for the Azeron devices (Cyro, Cyborg,
Classic/Compact, Cyro Lefty, Cyborg II and Keyzen). These devices share
a firmware XInput implementation and expose the standard Xbox 360
interface signature.
Note that 0x16d0 is a shared vendor block (MCS Electronics / pid.codes
allocations), but binding here is safe as the XPAD_XBOX360_VENDOR macro
only binds interfaces presenting the strict Xbox 360 signature.
These devices reject Xbox 360 LED and rumble magic packets with a
stall (-EPIPE). Add FLAG_NO_LED and FLAG_NO_RUMBLE device flags,
set on all Azeron entries, so xpad_led_probe() and xpad_init_ff()
skip LED and force-feedback initialization for them.
Signed-off-by: Roberts Kursitis <roberts.kursitis@azeron.eu>
---
v2 -> v3:
- Move 0x16d0 entries after 0x1689 to keep both tables sorted by
vendor ID
- Move quirks from the mapping field to the flags field; mapping is
u8 and the new bits overflowed it
- Add FLAG_NO_RUMBLE; these devices stall on rumble packets too
drivers/input/joystick/xpad.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index feb8f368f..405fcd5e9 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -72,6 +72,9 @@
#define PKT_XBE2_FW_5_11 4
#define FLAG_DELAY_INIT BIT(0)
+#define FLAG_NO_LED BIT(1)
+#define FLAG_NO_RUMBLE BIT(2)
+#define FLAG_NO_LED_RUMBLE (FLAG_NO_LED | FLAG_NO_RUMBLE)
static bool dpad_to_buttons;
module_param(dpad_to_buttons, bool, S_IRUGO);
@@ -292,6 +295,12 @@ static const struct xpad_device {
{ 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
{ 0x1689, 0xfd01, "Razer Onza Classic Edition", 0, XTYPE_XBOX360 },
{ 0x1689, 0xfe00, "Razer Sabertooth", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x1103, "Azeron Cyro", 0, XTYPE_XBOX360, FLAG_NO_LED_RUMBLE },
+ { 0x16d0, 0x113c, "Azeron Cyborg", 0, XTYPE_XBOX360, FLAG_NO_LED_RUMBLE },
+ { 0x16d0, 0x1192, "Azeron Classic/Compact", 0, XTYPE_XBOX360, FLAG_NO_LED_RUMBLE },
+ { 0x16d0, 0x1212, "Azeron Cyro Lefty", 0, XTYPE_XBOX360, FLAG_NO_LED_RUMBLE },
+ { 0x16d0, 0x12f7, "Azeron Cyborg II", 0, XTYPE_XBOX360, FLAG_NO_LED_RUMBLE },
+ { 0x16d0, 0x13ea, "Azeron Keyzen", 0, XTYPE_XBOX360, FLAG_NO_LED_RUMBLE },
{ 0x17ef, 0x6182, "Lenovo Legion Controller for Windows", 0, XTYPE_XBOX360 },
{ 0x1949, 0x041a, "Amazon Game Controller", 0, XTYPE_XBOX360 },
{ 0x1a86, 0xe310, "Legion Go S", 0, XTYPE_XBOX360 },
@@ -533,6 +542,7 @@ static const struct usb_device_id xpad_table[] = {
XPAD_XBOX360_VENDOR(0x15e4), /* Numark Xbox 360 controllers */
XPAD_XBOX360_VENDOR(0x162e), /* Joytech Xbox 360 controllers */
XPAD_XBOX360_VENDOR(0x1689), /* Razer Onza */
+ XPAD_XBOX360_VENDOR(0x16d0), /* Azeron controllers */
XPAD_XBOX360_VENDOR(0x17ef), /* Lenovo */
XPAD_XBOX360_VENDOR(0x1949), /* Amazon controllers */
XPAD_XBOX360_VENDOR(0x1a86), /* Nanjing Qinheng Microelectronics (WCH) */
@@ -776,6 +786,8 @@ struct usb_xpad {
const char *name; /* name of the device */
struct work_struct work; /* init/remove device from callback */
time64_t mode_btn_down_ts;
+ bool no_led; /* device stalls on LED packets */
+ bool no_rumble; /* device stalls on rumble packets */
bool delay_init; /* init packets should be delayed */
bool delayed_init_done;
};
@@ -1615,6 +1627,9 @@ static int xpad_init_ff(struct usb_xpad *xpad)
if (xpad->xtype == XTYPE_UNKNOWN)
return 0;
+ if (xpad->no_rumble)
+ return 0;
+
input_set_capability(xpad->dev, EV_FF, FF_RUMBLE);
return input_ff_create_memless(xpad->dev, NULL, xpad_play_effect);
@@ -1719,6 +1734,9 @@ static int xpad_led_probe(struct usb_xpad *xpad)
struct led_classdev *led_cdev;
int error;
+ if (xpad->no_led)
+ return 0;
+
if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX360W)
return 0;
@@ -2082,6 +2100,10 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
xpad->name = xpad_device[i].name;
if (xpad_device[i].flags & FLAG_DELAY_INIT)
xpad->delay_init = true;
+ if (xpad_device[i].flags & FLAG_NO_LED)
+ xpad->no_led = true;
+ if (xpad_device[i].flags & FLAG_NO_RUMBLE)
+ xpad->no_rumble = true;
xpad->packet_type = PKT_XB;
INIT_WORK(&xpad->work, xpad_presence_work);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 1/1] Input: xpad - add support for Azeron devices
From: sashiko-bot @ 2026-06-19 13:43 UTC (permalink / raw)
To: Roberts Kursitis; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260619132821.18764-1-roberts.kursitis@azeron.eu>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] The newly added Azeron device IDs are not sorted by vendor ID, violating the explicit comments in the driver.
Pre-existing issues:
- [High] Azeron devices will stall (-EPIPE) on FF_RUMBLE packets, triggering a pre-existing infinite URB retry loop.
--
commit 32ac579407e3b61d52176dbd3f7d52733eb47d44
Author: Roberts Kursitis <roberts.kursitis@azeron.eu>
Input: xpad - add support for Azeron devices
Add USB Vendor and Product IDs for the Azeron devices (Cyro,
Cyborg, Classic/Compact, Cyro Lefty, Cyborg II and Keyzen). Adds
a QUIRK_NO_LED flag to skip LED initialization since these devices
reject the Xbox 360 LED magic packet with a stall.
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834e..4efcd16c3713c 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
[ ... ]
> @@ -289,6 +290,12 @@ static const struct xpad_device {
> { 0x15e4, 0x3f0a, "Xbox Airflo wired controller", 0, XTYPE_XBOX360 },
> { 0x15e4, 0x3f10, "Batarang Xbox 360 controller", 0, XTYPE_XBOX360 },
> { 0x162e, 0xbeef, "Joytech Neo-Se Take2", 0, XTYPE_XBOX360 },
> + { 0x16d0, 0x1103, "Azeron Cyro", QUIRK_NO_LED, XTYPE_XBOX360 },
[Severity: High]
This is a pre-existing issue, but mapping these devices as XTYPE_XBOX360 will
unconditionally enable the FF_RUMBLE capability in xpad_init_ff().
Since you noted in the commit message that these devices stall (-EPIPE) on LED
packets, will they also stall if userspace sends a rumble effect packet?
If they do stall on rumble, this will trigger a pre-existing bug in
xpad_irq_out(), which handles the -EPIPE status by falling through to the
default case:
drivers/input/joystick/xpad.c:xpad_irq_out() {
switch (status) {
...
default:
dev_dbg(dev, "%s - nonzero urb status received: %d\n", __func__, status);
break;
}
if (xpad->irq_out_active) {
usb_anchor_urb(urb, &xpad->irq_out_anchor);
error = usb_submit_urb(urb, GFP_ATOMIC);
...
}
Because xpad->irq_out_active is not cleared on -EPIPE, the driver will
infinitely resubmit the stalled OUT URB, creating an interrupt storm.
Should a QUIRK_NO_RUMBLE or similar be added to prevent rumble packets
from being sent to these devices?
[Severity: Low]
this isn't a bug, but the new entries with vendor ID 0x16d0 are placed before
the 0x1689 entries. Should these be moved down to maintain the numerical
sorting of the xpad_device array?
> + { 0x16d0, 0x113c, "Azeron Cyborg", QUIRK_NO_LED, XTYPE_XBOX360 },
> + { 0x16d0, 0x1192, "Azeron Classic/Compact", QUIRK_NO_LED, XTYPE_XBOX360 },
> + { 0x16d0, 0x1212, "Azeron Cyro Lefty", QUIRK_NO_LED, XTYPE_XBOX360 },
> + { 0x16d0, 0x12f7, "Azeron Cyborg II", QUIRK_NO_LED, XTYPE_XBOX360 },
> + { 0x16d0, 0x13ea, "Azeron Keyzen", QUIRK_NO_LED, XTYPE_XBOX360 },
> { 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
> { 0x1689, 0xfd01, "Razer Onza Classic Edition", 0, XTYPE_XBOX360 },
[ ... ]
> @@ -532,6 +539,7 @@ static const struct usb_device_id xpad_table[] = {
> XPAD_XBOXONE_VENDOR(0x1532), /* Razer Wildcat */
> XPAD_XBOX360_VENDOR(0x15e4), /* Numark Xbox 360 controllers */
> XPAD_XBOX360_VENDOR(0x162e), /* Joytech Xbox 360 controllers */
> + XPAD_XBOX360_VENDOR(0x16d0), /* Azeron controllers */
[Severity: Low]
this isn't a bug, but similarly here, should the 0x16d0 vendor block be
placed after 0x1689 to keep the list sorted by vendor ID as requested by the
comment at the top of xpad_table?
> XPAD_XBOX360_VENDOR(0x1689), /* Razer Onza */
> XPAD_XBOX360_VENDOR(0x17ef), /* Lenovo */
> XPAD_XBOX360_VENDOR(0x1949), /* Amazon controllers */
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619132821.18764-1-roberts.kursitis@azeron.eu?part=1
^ permalink raw reply
* [PATCH v2 1/1] Input: xpad - add support for Azeron devices
From: Roberts Kursitis @ 2026-06-19 13:28 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Roberts Kursitis
In-Reply-To: <20260619121901.17926-1-roberts.kursitis@azeron.eu>
Add USB Vendor and Product IDs for the Azeron devices (Cyro,
Cyborg, Classic/Compact, Cyro Lefty, Cyborg II and Keyzen).
These devices share a firmware XInput implementation and
expose the standard xbox 360 interface signature. Note that
0x16d0 is a shared vendor block (MCS Electronics / pid.codes
allocations), but is bound here as the XPAD_XBOX360_VENDOR
macro only binds interfaces presenting the strict xbox 360
signature.
Note: Devices may reject the LED magic-packet with -EPIPE.
These devices reject the Xbox 360 LED magic packet with a stall
(-EPIPE). Add a QUIRK_NO_LED flag, set on all Azeron entries, so
xpad_led_probe() skips LED initialization for them.
Signed-off-by: Roberts Kursitis <roberts.kursitis@azeron.eu>
---
drivers/input/joystick/xpad.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index feb8f368f..4efcd16c3 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -50,6 +50,7 @@
#define MAP_PADDLES BIT(4)
#define MAP_PROFILE_BUTTON BIT(5)
#define MAP_SHARE_OFFSET BIT(6)
+#define QUIRK_NO_LED BIT(7)
#define DANCEPAD_MAP_CONFIG (MAP_DPAD_TO_BUTTONS | \
MAP_TRIGGERS_TO_BUTTONS | MAP_STICKS_TO_NULL)
@@ -289,6 +290,12 @@ static const struct xpad_device {
{ 0x15e4, 0x3f0a, "Xbox Airflo wired controller", 0, XTYPE_XBOX360 },
{ 0x15e4, 0x3f10, "Batarang Xbox 360 controller", 0, XTYPE_XBOX360 },
{ 0x162e, 0xbeef, "Joytech Neo-Se Take2", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x1103, "Azeron Cyro", QUIRK_NO_LED, XTYPE_XBOX360 },
+ { 0x16d0, 0x113c, "Azeron Cyborg", QUIRK_NO_LED, XTYPE_XBOX360 },
+ { 0x16d0, 0x1192, "Azeron Classic/Compact", QUIRK_NO_LED, XTYPE_XBOX360 },
+ { 0x16d0, 0x1212, "Azeron Cyro Lefty", QUIRK_NO_LED, XTYPE_XBOX360 },
+ { 0x16d0, 0x12f7, "Azeron Cyborg II", QUIRK_NO_LED, XTYPE_XBOX360 },
+ { 0x16d0, 0x13ea, "Azeron Keyzen", QUIRK_NO_LED, XTYPE_XBOX360 },
{ 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
{ 0x1689, 0xfd01, "Razer Onza Classic Edition", 0, XTYPE_XBOX360 },
{ 0x1689, 0xfe00, "Razer Sabertooth", 0, XTYPE_XBOX360 },
@@ -532,6 +539,7 @@ static const struct usb_device_id xpad_table[] = {
XPAD_XBOXONE_VENDOR(0x1532), /* Razer Wildcat */
XPAD_XBOX360_VENDOR(0x15e4), /* Numark Xbox 360 controllers */
XPAD_XBOX360_VENDOR(0x162e), /* Joytech Xbox 360 controllers */
+ XPAD_XBOX360_VENDOR(0x16d0), /* Azeron controllers */
XPAD_XBOX360_VENDOR(0x1689), /* Razer Onza */
XPAD_XBOX360_VENDOR(0x17ef), /* Lenovo */
XPAD_XBOX360_VENDOR(0x1949), /* Amazon controllers */
@@ -1719,6 +1727,9 @@ static int xpad_led_probe(struct usb_xpad *xpad)
struct led_classdev *led_cdev;
int error;
+ if (xpad->mapping & QUIRK_NO_LED)
+ return 0;
+
if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX360W)
return 0;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] Input: xpad - add support for Azeron devices
From: sashiko-bot @ 2026-06-19 12:29 UTC (permalink / raw)
To: Roberts Kursitis; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260619121901.17926-1-roberts.kursitis@azeron.eu>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] The patch inadvertently introduces duplicate device entries and breaks the documented sorting order of the `xpad_device` array.
Pre-existing issues:
- [High] The newly supported devices permanently wedge the driver's OUT endpoint upon connection due to an unhandled -EPIPE stall on the LED packet.
--
commit 82abb48f58f1b666ded31cf35205e5974880b177
Author: Roberts Kursitis <roberts.kursitis@azeron.eu>
Input: xpad - add support for Azeron devices
Add USB Vendor and Product IDs for the Azeron devices (Cyro, Cyborg,
Classic/Compact, Cyro Lefty, Cyborg II and Keyzen).
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834e..82019cbe2a9c7 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -292,6 +292,14 @@ static const struct xpad_device {
> { 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
> { 0x1689, 0xfd01, "Razer Onza Classic Edition", 0, XTYPE_XBOX360 },
> { 0x1689, 0xfe00, "Razer Sabertooth", 0, XTYPE_XBOX360 },
> + { 0x162e, 0xbeef, "Joytech Neo-Se Take2", 0, XTYPE_XBOX360 },
> + { 0x16d0, 0x1103, "Azeron Cyro", 0, XTYPE_XBOX360 },
> + { 0x16d0, 0x113c, "Azeron Cyborg", 0, XTYPE_XBOX360 },
> + { 0x16d0, 0x1192, "Azeron Classic/Compact", 0, XTYPE_XBOX360 },
> + { 0x16d0, 0x1212, "Azeron Cyro Lefty", 0, XTYPE_XBOX360 },
> + { 0x16d0, 0x12f7, "Azeron Cyborg II", 0, XTYPE_XBOX360 },
> + { 0x16d0, 0x13ea, "Azeron Keyzen", 0, XTYPE_XBOX360 },
> + { 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
[Severity: Low]
It looks like this might be a copy-paste error, and it also breaks the
documented vendor/product ID sorting order of the xpad_device array.
Was it intended to duplicate the "Joytech Neo-Se Take2" and "Razer Onza
Tournament Edition" entries here?
[Severity: High]
This isn't a bug introduced by this patch, but adding these new devices here
unconditionally triggers it since they are mapped generically as XTYPE_XBOX360
without any quirks.
The commit message notes that these devices reject the LED magic-packet with
a stall (-EPIPE). Because they are mapped as XTYPE_XBOX360, xpad_led_probe()
unconditionally queues an LED initialization command during probe.
When the device stalls the endpoint, the xpad_irq_out() handler receives
-EPIPE but fails to clear the hardware halt via usb_clear_halt(). It leaves
the endpoint permanently halted at the hardware level:
drivers/input/joystick/xpad.c:xpad_irq_out() {
...
switch (status) {
case 0:
/* success */
xpad->irq_out_active = xpad_prepare_next_out_packet(xpad);
break;
...
default:
dev_dbg(dev, "%s - nonzero urb status received: %d\n",
__func__, status);
break;
}
...
}
The host controller will ignore further submissions to a halted endpoint,
causing all subsequent output commands (like force-feedback) to silently
fail or hang.
Should a quirk be implemented and applied to these devices to avoid sending
LED packets to devices that stall on them?
> { 0x17ef, 0x6182, "Lenovo Legion Controller for Windows", 0, XTYPE_XBOX360 },
> { 0x1949, 0x041a, "Amazon Game Controller", 0, XTYPE_XBOX360 },
> { 0x1a86, 0xe310, "Legion Go S", 0, XTYPE_XBOX360 },
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619121901.17926-1-roberts.kursitis@azeron.eu?part=1
^ permalink raw reply
* [PATCH] Input: xpad - add support for Azeron devices
From: Roberts Kursitis @ 2026-06-19 12:19 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Roberts Kursitis
Add USB Vendor and Product IDs for the Azeron devices (Cyro,
Cyborg, Classic/Compact, Cyro Lefty, Cyborg II and Keyzen).
These devices share a firmware XInput implementation and
expose the standard xbox 360 interface signature. Note that
0x16d0 is a shared vendor block (MCS Electronics / pid.codes
allocations), but is bound here as the XPAD_XBOX360_VENDOR
macro only binds interfaces presenting the strict xbox 360
signature.
Note: Devices may reject the LED magic-packet with -EPIPE.
Signed-off-by: Roberts Kursitis <roberts.kursitis@azeron.eu>
---
drivers/input/joystick/xpad.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index feb8f368f..82019cbe2 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -292,6 +292,14 @@ static const struct xpad_device {
{ 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
{ 0x1689, 0xfd01, "Razer Onza Classic Edition", 0, XTYPE_XBOX360 },
{ 0x1689, 0xfe00, "Razer Sabertooth", 0, XTYPE_XBOX360 },
+ { 0x162e, 0xbeef, "Joytech Neo-Se Take2", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x1103, "Azeron Cyro", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x113c, "Azeron Cyborg", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x1192, "Azeron Classic/Compact", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x1212, "Azeron Cyro Lefty", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x12f7, "Azeron Cyborg II", 0, XTYPE_XBOX360 },
+ { 0x16d0, 0x13ea, "Azeron Keyzen", 0, XTYPE_XBOX360 },
+ { 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
{ 0x17ef, 0x6182, "Lenovo Legion Controller for Windows", 0, XTYPE_XBOX360 },
{ 0x1949, 0x041a, "Amazon Game Controller", 0, XTYPE_XBOX360 },
{ 0x1a86, 0xe310, "Legion Go S", 0, XTYPE_XBOX360 },
@@ -533,6 +541,7 @@ static const struct usb_device_id xpad_table[] = {
XPAD_XBOX360_VENDOR(0x15e4), /* Numark Xbox 360 controllers */
XPAD_XBOX360_VENDOR(0x162e), /* Joytech Xbox 360 controllers */
XPAD_XBOX360_VENDOR(0x1689), /* Razer Onza */
+ XPAD_XBOX360_VENDOR(0x16d0), /* Azeron controllers */
XPAD_XBOX360_VENDOR(0x17ef), /* Lenovo */
XPAD_XBOX360_VENDOR(0x1949), /* Amazon controllers */
XPAD_XBOX360_VENDOR(0x1a86), /* Nanjing Qinheng Microelectronics (WCH) */
--
2.43.0
^ permalink raw reply related
* Re: [RFC PATCH] HID: BPF: add keyboard behavioral anomaly detection
From: sashiko-bot @ 2026-06-19 7:45 UTC (permalink / raw)
To: krishgulati7; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260619073607.393248-1-krishgulati7@gmail.com>
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Evasion of detector via large HID descriptors with padded payloads.
- [High] Missing Report ID validation causes false positives and state corruption on composite devices.
- [High] Mismatched report size rounding causes silent event dropping and logical out-of-bounds reads.
- [High] LRU map eviction permanently disables detection for a device instead of resetting its state.
- [High] Uninitialized padding bytes in the stack-allocated `struct dev_info` will cause the BPF verifier to reject the program.
- [High] Keystroke heuristic fails to detect keys when no empty report is sent between strokes (morphed keys).
- [Medium] Missing `bpf_spin_lock` protection allows data races between concurrent `kdb_hook` or `probe` invocations.
--
commit 103b2fc551543479bbac0ff3ea588c3508776a4c
Author: Krish Gulati <krishgulati7@gmail.com>
HID: BPF: add keyboard behavioral anomaly detection
This patch implements a HID-BPF struct_ops program that detects automated
HID injection attacks. It does this by measuring post-enumeration delay
and tracking inter-keystroke timing using Welford's online variance algorithm.
State is stored in an LRU hash map keyed by the HID ID.
Link: https://lore.kernel.org/linux-input/adSxXidgeWF0-Ewn@beelink/
> diff --git a/src/bpf/testing/0010-Generic__keyboard.bpf.c b/src/bpf/testing/0010-Generic__keyboard.bpf.c
> index 0000000000000..91145870768f6
> --- /dev/null
> +++ b/src/bpf/testing/0010-Generic__keyboard.bpf.c
[ ... ]
> +static __always_inline void welford(struct dev_info *dev_state,
> + __u64 interval_ms)
> +{
> + __s64 x = (__s64)interval_ms * HID_GUARD_WELFORD_SCALE;
> + __s64 delta, delta2, delta_abs, signed_div_res;
> + __u64 div_res;
> +
> + dev_state->count++;
[Severity: Medium]
Will this lockless compound update be corrupted by concurrent executions?
The dev_details LRU hash map stores per-device state which kdb_hook() reads
and modifies directly via a map value pointer locklessly. Updating Welford
variance (count, mean, M2) is a compound operation.
If kdb_hook() executes concurrently for the same device, such as across
multiple endpoints on a composite device, or if the userspace probe()
program is executed simultaneously, torn reads and writes will occur.
Does this structure require a bpf_spin_lock to prevent data races?
> + delta = x - dev_state->mean;
[ ... ]
> +SEC(HID_BPF_DEVICE_EVENT)
> +int BPF_PROG(kdb_hook, struct hid_bpf_ctx *hctx)
> +{
> + __u32 hid_id = hctx->hid->id;
> +
> + struct dev_info *info;
> +
> + info = bpf_map_lookup_elem(&dev_details, &hid_id);
> +
> + if (!info)
> + return 0;
[Severity: High]
Does an LRU map eviction permanently disable detection for a device instead
of resetting its state?
The dev_details map uses BPF_MAP_TYPE_LRU_HASH with 128 max entries.
When a device is evicted, bpf_map_lookup_elem() returns NULL, and
kdb_hook() immediately returns 0, completely ignoring the event.
Because elements are only inserted by the userspace probe() program, an
evicted device is never automatically re-inserted by the kernel hook.
Could an attacker deliberately present 128 dummy HID interfaces to flood
the map, permanently evict the payload device, and bypass the guard entirely?
> +
> + __u32 size = info->report_size;
> + __u32 fetch_size;
> + __u8 *data;
> +
> + if (size <= 8)
> + fetch_size = 8;
> + else if (size <= 16)
> + fetch_size = 16;
> + else
> + fetch_size = 32;
[Severity: Critical]
Does hardcapping the analysis size to 32 bytes allow attackers to evade
detection by padding their malicious HID descriptor?
An attacker could craft a custom USB HID descriptor that defines dummy
fields or padding for the first 32 bytes, placing the actual Keyboard Usage
page array at offset 32 or higher.
The loop will only iterate over the static padding and completely ignore
the keystrokes. Meanwhile, the Linux HID subsystem correctly parses the full
report and processes the hidden keycodes, allowing arbitrary keystroke
injection that bypasses this guard.
> +
> + data = hid_bpf_get_data(hctx, 0, fetch_size);
> +
> + if (!data)
> + return 0;
[Severity: High]
Will this size rounding cause silent event dropping and logical out-of-bounds
reads?
The BPF program rounds info->report_size up to a power-of-two fetch_size
(8, 16, or 32). If a keyboard has a 9-byte report, fetch_size becomes 16.
If the kernel's ctx->allocated_size for this device is exactly 9 bytes,
hid_bpf_get_data(hctx, 0, 16) will exceed the bounds and return NULL.
The program then returns 0, silently skipping analysis for the report
while the kernel continues processing the keystrokes.
If allocated_size is large enough, the loop uses fetch_size as the upper
bound, reading past valid report data into padding or uninitialized memory,
polluting the active key tracking.
> + int now_active_ks = 0, was_active_ks = 0;
> +#pragma unroll
> + for (int i = 0; i < 32; i++) {
> + if (i >= fetch_size)
> + break;
> + now_active_ks += ((__u8)data[i] + 255) >> 8;
[Severity: High]
Will this logic indiscriminately process all incoming raw reports, even
those not belonging to the keyboard?
Many HID keyboards are composite devices, such as wireless keyboard and
mouse receivers, that send multiple report types over the same interface.
Since the report ID is observed in probe() but not stored in dev_info,
this hook counts all non-zero bytes across all input reports as keystrokes.
Mouse movements, which send reports with changing non-zero bytes, will be
incorrectly interpreted as rapid keystrokes.
Will this corrupt the Welford variance tracking and cause false positive
anomaly detections?
> + was_active_ks += ((__u8)info->prev_report[i] + 255) >> 8;
> +
> + info->prev_report[i] = data[i];
> + }
> +
> + __u64 current_ms = bpf_ktime_get_ns() / HID_GUARD_NSEC_PER_MSEC;
> +
> + if (now_active_ks > was_active_ks) {
[Severity: High]
Does this heuristic fail to detect keys when no empty report is sent
between strokes?
The detection logic relies strictly on an increase in the total number of
non-zero bytes to identify a keypress.
Automated injection tools routinely optimize injections by morphing
keycodes directly without sending an intervening empty (all-zeros) report.
If an attacker's tool changes key 'A' to key 'B' in consecutive reports,
both reports have exactly one non-zero byte.
The condition 1 > 1 evaluates to false, causing the keystroke to be missed
and bypassing the variance check entirely.
[ ... ]
> +SEC("syscall")
> +int probe(struct hid_bpf_probe_args *ctx)
> +{
> + struct hid_rdesc_report *input;
> + struct hid_rdesc_field *field;
> + struct hid_rdesc_collection *col;
> +
> + hid_bpf_for_each_input_report(&HID_REPORT_DESCRIPTOR, input) {
> + __u32 size_in_bytes = (input->size_in_bits + 7) / 8;
> +
> + bpf_printk("Report size: %d\n", size_in_bytes);
> + if (input->report_id != 0)
> + size_in_bytes += 1;
[Severity: High]
Is the report ID needed here to validate incoming reports later?
The report ID is observed here but not stored in the map, making it
impossible for kdb_hook() to distinguish between keyboard reports and
other HID reports on a composite device.
> +
> + bpf_printk("Report size after report_id: %d\n", size_in_bytes);
> +
> + hid_bpf_for_each_field(input, field) {
> + hid_bpf_for_each_collection(field, col) {
> + if (col->usage_page ==
> + HidUsagePage_GenericDesktop &&
> + col->usage_id == HidUsage_GD_Keyboard) {
> + __u32 key = ctx->hid;
> + struct dev_info info = {
> + .connection_time =
> + bpf_ktime_get_ns(),
> + .count = 0,
> + .report_size = size_in_bytes
> + };
[Severity: High]
Will the uninitialized padding bytes in this struct cause the BPF verifier
to reject the program?
The dev_info object is initialized using a designated initializer list.
While this zero-initializes unspecified members, it does not guarantee that
the compiler will zero the 4 bytes of padding between report_size and
prev_report.
The BPF verifier strictly tracks initialization of stack memory at the byte
level. When bpf_map_update_elem() reads the struct from the stack, the
verifier will detect the uninitialized padding and reject the program with
an 'invalid indirect read from stack' error.
Should this structure be explicitly zeroed using __builtin_memset?
> + bpf_map_update_elem(&dev_details, &key,
> + &info, BPF_ANY);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619073607.393248-1-krishgulati7@gmail.com?part=1
^ permalink raw reply
* [RFC PATCH] HID: BPF: add keyboard behavioral anomaly detection
From: krishgulati7 @ 2026-06-19 7:36 UTC (permalink / raw)
To: bentiss; +Cc: linux-input, linux-kernel, jikos, Krish Gulati
In-Reply-To: <adSxXidgeWF0-Ewn@beelink>
From: Krish Gulati <krishgulati7@gmail.com>
Implements a HID-BPF struct_ops program that detects behaviorally
anomalous keyboard input consistent with automated HID injection
attacks (USB Rubber Ducky and similar).
This is a direct response to Benjamin Tissoires's recommendation in
the hid-omg-detect review thread [1], where a kernel driver approach
was rejected on the grounds that a HID driver can only bind exclusively
to a device, racing with and displacing the legitimate driver. The
HID-BPF struct_ops approach resolves this: the program attaches
alongside the existing driver, receives the raw event stream before
kernel processing, and never takes exclusive ownership of the device.
Two independent detection signals are implemented:
Post-Enumeration Delay (PED):
Measures the delta between device connection (recorded at probe()
time via bpf_ktime_get_ns()) and the first input packet. Legitimate
users take hundreds of milliseconds to react after plugging in a
device; injection tools begin transmitting within 25-100ms. Events
are classified into three bands: SUSPICIOUS (<50ms), WARNING
(<300ms), and NORMAL (>=300ms). Thresholds are placeholder values
and MUST be empirically calibrated before production use.
Welford online variance (inter-keystroke timing):
Tracks keydown-to-keydown intervals per device using Welford's
single-pass online algorithm. BPF forbids floating-point, so the
running mean is maintained in fixed-point (scaled by 1024) to
prevent integer truncation at small counts. Intervals exceeding
10 seconds are treated as typing-session breaks and excluded from
the variance calculation. Detection fires after
HID_GUARD_MIN_SAMPLES (5) samples when variance falls below
HID_GUARD_VARIANCE_THRESH_MS2 (1600 ms^2), flagging suspiciously
metronomic timing inconsistent with human input.
Per-device state (Welford accumulators, PED timestamp, report size)
is maintained in a BPF_MAP_TYPE_LRU_HASH keyed by hid->id. The LRU
eviction policy prevents map exhaustion at the cost of two documented
trade-offs: eviction can silently reset Welford history (potential
map-flood attack) and may clear connection_time (PED blindspot on
device re-wake after eviction).
Detection results are currently surfaced via bpf_printk(). A
BPF_MAP_TYPE_RINGBUF interface with configurable userspace daemon
is planned; deferred pending validation of detection heuristics.
Signal design is grounded in: Neuner et al., "USBlock: Blocking
USB-based Keylogger Attacks", DBSec 2018.
[1] https://lore.kernel.org/linux-input/adSxXidgeWF0-Ewn@beelink/
Link: https://lore.kernel.org/linux-input/adSxXidgeWF0-Ewn@beelink/
Signed-off-by: Krish Gulati <krishgulati7@gmail.com>
---
src/bpf/testing/0010-Generic__keyboard.bpf.c | 285 +++++++++++++++++++
1 file changed, 285 insertions(+)
create mode 100644 src/bpf/testing/0010-Generic__keyboard.bpf.c
diff --git a/src/bpf/testing/0010-Generic__keyboard.bpf.c b/src/bpf/testing/0010-Generic__keyboard.bpf.c
new file mode 100644
index 0000000..9114587
--- /dev/null
+++ b/src/bpf/testing/0010-Generic__keyboard.bpf.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include "hid_bpf.h"
+#include "hid_bpf_helpers.h"
+#include "hid_report_helpers.h"
+#include "hid_usages.h"
+#include "vmlinux.h"
+#include <bpf/bpf_tracing.h>
+
+#define HID_GUARD_NSEC_PER_MSEC 1000000ULL
+/*
+ * Post-enumeration delay thresholds (raw nanoseconds, matching
+ * connection_time and first_packet_time which are both __u64
+ * bpf_ktime_get_ns() values).
+ *
+ * LIMITATIONS: placeholder values based on rough figures from USB
+ * Rubber Ducky DETECT_READY research (modern hosts HID-ready in
+ * ~25-100ms) versus typical human reaction time after device
+ * connection (hundreds of ms to low seconds). NOT derived from a
+ * controlled study of this specific signal and MUST be empirically
+ * calibrated against real device/host combinations before any
+ * production use. Treat as a tunable, not a validated security
+ * boundary.
+ */
+#define HID_GUARD_PED_SUSPICIOUS_THRESH (50 * HID_GUARD_NSEC_PER_MSEC)
+#define HID_GUARD_PED_WARNING_THRESH (300 * HID_GUARD_NSEC_PER_MSEC)
+
+/*not derived from real typing data yet*/
+#define HID_GUARD_MIN_SAMPLES 5
+
+/*
+ * Variance is "too metronomic
+ * to be a human," expressed in ms^2 so we never need sqrt().
+ */
+#define HID_GUARD_VARIANCE_THRESH_MS2 (40ULL * 40ULL)
+
+/*
+ * Idle gaps this large are treated as a break in the typing session,
+ * not a sample — interval excluded from Welford, timer rebased.
+ * Borrowed threshold from hid-omg-detect (10s); not independently
+ * validated for this signal.
+ */
+#define HID_GUARD_IDLE_GAP_THRESH_MS (10ULL * 1000ULL)
+
+/*
+ * fixed-point factor: BPF has no floating values, so mean is stored as
+ * real_mean * HID_GUARD_WELFORD_SCALE.
+ * Without this, delta/count truncates to 0
+ * as soon as count exceeds the typical delta (in millisecond)
+ */
+#define HID_GUARD_WELFORD_SCALE 1024
+
+enum hid_guard_ped_flag {
+ HID_GUARD_PED_NO_ENTRY = 0,
+ HID_GUARD_PED_SUSPICIOUS = 1,
+ HID_GUARD_PED_WARNING = 2,
+ HID_GUARD_PED_NORMAL = 3,
+};
+
+struct dev_info {
+ __u64 connection_time;
+ __u32 report_size;
+ __u64 prev_report[32];
+ /*welford's variables*/
+ __u64 prev_keydown_ts;
+ __u64 count;
+ __s64 mean;
+ __u64 M2;
+ /*
+ * unsigned as delta * delta2 is always >= 0
+ * (as delta2 is delta scaled by the same sign)
+ */
+};
+
+/*
+ * BPF_MAP_TYPE_LRU_HASH:
+ * Using an LRU map automatically prevents exhaustion by silently evicting
+ * the oldest idle devices. It requires no syntax changes to the rest of the
+ * code (lookup/update helpers work identically), but introduces behavioral
+ * trade-offs:
+ *
+ * 1. Eviction wipes Welford variance history. An attacker
+ * could theoretically flood the map to flush their device and reset their
+ * score.
+ * 2. PED Blindspot: Eviction deletes the 'connection_time' set during probe().
+ * If an evicted device wakes up, it will bypass post-enumeration delay
+ * checks.
+ */
+struct {
+ __uint(type, BPF_MAP_TYPE_LRU_HASH);
+ __type(key, __u32);
+ __type(value, struct dev_info);
+ __uint(max_entries, 128);
+} dev_details SEC(".maps");
+
+/*
+ * POST ENUMERATION DELAY
+ *
+ * Computes the delta between the first input packet timestamp and
+ * connection_time (set at probe() time), both raw __u64 nanoseconds
+ * from bpf_ktime_get_ns(). Called once per device, the caller gates
+ * this on connection_time != 0, and this function clears it to 0
+ * afterward so it never runs twice for the same device.
+ *
+ * Returns a hid_guard_ped_flag classifying the delay.
+ */
+static __always_inline enum hid_guard_ped_flag
+post_enumeration_delay(struct dev_info *info, __u64 first_packet_time)
+{
+ __u64 delta;
+
+ if (first_packet_time < info->connection_time)
+ return HID_GUARD_PED_NO_ENTRY;
+
+ delta = first_packet_time - info->connection_time;
+
+ if (delta < HID_GUARD_PED_SUSPICIOUS_THRESH)
+ return HID_GUARD_PED_SUSPICIOUS;
+ if (delta < HID_GUARD_PED_WARNING_THRESH)
+ return HID_GUARD_PED_WARNING;
+
+ return HID_GUARD_PED_NORMAL;
+}
+
+static __always_inline void welford(struct dev_info *dev_state,
+ __u64 interval_ms)
+{
+ __s64 x = (__s64)interval_ms * HID_GUARD_WELFORD_SCALE;
+ __s64 delta, delta2, delta_abs, signed_div_res;
+ __u64 div_res;
+
+ dev_state->count++;
+ delta = x - dev_state->mean;
+ delta_abs = delta < 0 ? -delta : delta;
+ div_res = (__u64)delta_abs / (__u64)dev_state->count;
+ signed_div_res = delta < 0 ? -(__s64)div_res : (__s64)div_res;
+
+ dev_state->mean += signed_div_res;
+ delta2 = x - dev_state->mean;
+
+ dev_state->M2 += (__u64)(delta * delta2);
+
+ bpf_printk("W[Count:%llu] Int:%llu ms, scaled_x:%lld\n",
+ dev_state->count, interval_ms, x);
+ bpf_printk(" -> delta1:%lld, mean:%lld\n", delta, dev_state->mean);
+ bpf_printk(" -> delta2:%lld, M2:%llu\n", delta2, dev_state->M2);
+}
+
+HID_BPF_CONFIG(HID_DEVICE(BUS_USB, HID_GROUP_ANY, HID_VID_ANY, HID_PID_ANY),
+ HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_ANY, HID_VID_ANY,
+ HID_PID_ANY));
+
+SEC(HID_BPF_DEVICE_EVENT)
+int BPF_PROG(kdb_hook, struct hid_bpf_ctx *hctx)
+{
+ __u32 hid_id = hctx->hid->id;
+
+ struct dev_info *info;
+
+ info = bpf_map_lookup_elem(&dev_details, &hid_id);
+
+ if (!info)
+ return 0;
+
+ __u32 size = info->report_size;
+ __u32 fetch_size;
+ __u8 *data;
+
+ if (size <= 8)
+ fetch_size = 8;
+ else if (size <= 16)
+ fetch_size = 16;
+ else
+ fetch_size = 32;
+
+ data = hid_bpf_get_data(hctx, 0, fetch_size);
+
+ if (!data)
+ return 0;
+ int now_active_ks = 0, was_active_ks = 0;
+#pragma unroll
+ for (int i = 0; i < 32; i++) {
+ if (i >= fetch_size)
+ break;
+ now_active_ks += ((__u8)data[i] + 255) >> 8;
+ was_active_ks += ((__u8)info->prev_report[i] + 255) >> 8;
+
+ info->prev_report[i] = data[i];
+ }
+
+ __u64 current_ms = bpf_ktime_get_ns() / HID_GUARD_NSEC_PER_MSEC;
+
+ if (now_active_ks > was_active_ks) {
+ if (info->prev_keydown_ts != 0) {
+ __u64 interval_ms = current_ms - info->prev_keydown_ts;
+
+ if (interval_ms < HID_GUARD_IDLE_GAP_THRESH_MS) {
+ welford(info, interval_ms);
+ } else {
+ bpf_printk(
+ "hid %d: idle gap %llu ms excluded from sample\n",
+ hid_id, interval_ms);
+ }
+ }
+
+ if (info->count >= HID_GUARD_MIN_SAMPLES) {
+ __u64 variance_m2 =
+ info->M2 /
+ ((__u64)HID_GUARD_WELFORD_SCALE *
+ HID_GUARD_WELFORD_SCALE * (info->count - 1));
+ /*
+ * the initial interval x was multiplied by HID_GUARD_WELFORD_SCALE, both
+ * delta and delta2 are also scaled by that factor,
+ * thus scale^2 in the denominator
+ */
+ if (variance_m2 < HID_GUARD_VARIANCE_THRESH_MS2) {
+ bpf_printk(
+ "hid %d: Suspeciously regular typing, variance=%llu ms^2\n",
+ hid_id, variance_m2);
+ }
+ }
+
+ info->prev_keydown_ts = current_ms;
+ }
+
+ if (info->connection_time != 0) {
+ enum hid_guard_ped_flag ped_flag =
+ post_enumeration_delay(info, bpf_ktime_get_ns());
+
+ bpf_printk("PED flag for hid %d: %d\n", hid_id, ped_flag);
+
+ /*
+ * Prevent re-evaluation on subsequent packets for this device
+ */
+ info->connection_time = 0;
+ }
+ return 0;
+}
+
+HID_BPF_OPS(hook_keyboard) = {
+ .hid_device_event = (void *)kdb_hook,
+};
+
+struct hid_rdesc_descriptor HID_REPORT_DESCRIPTOR;
+
+SEC("syscall")
+int probe(struct hid_bpf_probe_args *ctx)
+{
+ struct hid_rdesc_report *input;
+ struct hid_rdesc_field *field;
+ struct hid_rdesc_collection *col;
+
+ hid_bpf_for_each_input_report(&HID_REPORT_DESCRIPTOR, input) {
+ __u32 size_in_bytes = (input->size_in_bits + 7) / 8;
+
+ bpf_printk("Report size: %d\n", size_in_bytes);
+ if (input->report_id != 0)
+ size_in_bytes += 1;
+
+ bpf_printk("Report size after report_id: %d\n", size_in_bytes);
+
+ hid_bpf_for_each_field(input, field) {
+ hid_bpf_for_each_collection(field, col) {
+ if (col->usage_page ==
+ HidUsagePage_GenericDesktop &&
+ col->usage_id == HidUsage_GD_Keyboard) {
+ __u32 key = ctx->hid;
+ struct dev_info info = {
+ .connection_time =
+ bpf_ktime_get_ns(),
+ .count = 0,
+ .report_size = size_in_bytes
+ };
+ bpf_map_update_elem(&dev_details, &key,
+ &info, BPF_ANY);
+ ctx->retval = 0;
+ return 0;
+ }
+ }
+ }
+ }
+ ctx->retval = -EINVAL;
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 1/4] dt-bindings: input: Add binding for Qualcomm SPMI PMIC haptics
From: Krzysztof Kozlowski @ 2026-06-19 4:18 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: <c7191868-d141-4095-b703-230237721f28@oss.qualcomm.com>
On 17/06/2026 13:02, Fenglin Wu wrote:
>
> On 6/17/2026 6:35 PM, Krzysztof Kozlowski wrote:
>> On Tue, Jun 16, 2026 at 03:08:24AM -0700, Fenglin Wu wrote:
>>> ....
>>> +
>>> + qcom,lra-period-us:
>>> + description:
>>> + LRA actuator initial resonance period in microseconds
>>> + (1,000,000 / resonant_freq_hz). Used to configure T_LRA-based play
>>> + rates and the auto-resonance zero-crossing window.
>> This does not feel like static characteristic. Isn't period depending on
>> intensity of vibration you want to have? Why would that be fixed per
>> board?
>
> This period is specifically used for playbacks that require
> auto-resonance to be enabled, which I referred to as "T_LRA-based" and
> "auto-resonance zero-crossing window." It plays a key role in the
> "DIRECT_PLAY" mode, which produces a constant vibration effect. To
> adjust the vibration intensity during this constant effect, the hardware
> does it by scaling the peak voltage of the driver signals, rather than
> changing the frequency.
But maybe changing frequency runtime still would be useful?
What does the frequency depend on? IOW, how do you choose the value here
- based on some motor-part characteristics?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers
From: Vicki Pfau @ 2026-06-19 4:17 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260514005353.EF8D8C19425@smtp.kernel.org>
Let's tackle some more hallucinations.
On 5/13/26 5:53 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Mixing jiffies and milliseconds in delayed work scheduling logic causes an integer underflow, permanently stalling the haptic rumble.
> - [High] A guaranteed NULL pointer dereference occurs in the error logging path of switch2_rumble_work, as well as a TOCTOU race condition.
> - [Medium] `ns2->ctlr_type` is checked before it is dynamically initialized, causing union corruption for GameCube controllers.
> - [Low] Implicit type promotion in the max() macro causes a compiler warning.
> - [Low] Kconfig documentation claims individual motor control for the Pro Controller, but the implementation identicalizes the rumble data for both motors.
> --
>
> commit d72f0f8e0dadde2cacc27050b92088c2e5cbea0e
> Author: Vicki Pfau <vi@endrift.com>
> Subject: HID: nintendo: Add rumble support for Switch 2 controllers
>
> This commit introduces rumble support for Switch 2 controllers, handling both HD
> Rumble linear resonant actuators and eccentric rotating mass haptics. It adds
> state machines for rumble scheduling and output reporting via hdev.
>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 26ef589f652c6..a7fe74d982475 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -850,10 +850,10 @@ config NINTENDO_FF
>> depends on HID_NINTENDO
>> select INPUT_FF_MEMLESS
>> help
>> - Say Y here if you have a Nintendo Switch controller and want to enable
>> - force feedback support for it. This works for both joy-cons, the pro
>> - controller, and the NSO N64 controller. For the pro controller, both
>> - rumble motors can be controlled individually.
>> + Say Y here if you have a Nintendo Switch or Switch 2 controller and want
>> + to enable force feedback support for it. This works for Joy-Cons, the Pro
>> + Controllers, and the NSO N64 and GameCube controller. For the Pro
>> + Controller, both rumble motors can be controlled individually.
>
> This isn't a bug, but is the help text accurate regarding individual motor
> control? The implementation in switch2_rumble_work() appears to encode the
> exact same rumble configuration to both the left and right motor offsets,
> which would make both motors vibrate identically.
I didn't add this text, so I'm ingoring this.
>
>> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
>> index 2193afcdefabc..9801799a90758 100644
>> --- a/drivers/hid/hid-nintendo.c
>> +++ b/drivers/hid/hid-nintendo.c
>
> [ ... ]
>
>> @@ -3103,6 +3155,107 @@ static inline bool switch2_ctlr_is_joycon(enum switch2_ctlr_type type)
>> return type == NS2_CTLR_TYPE_JCL || type == NS2_CTLR_TYPE_JCR;
>> }
>>
>> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
>> +static void switch2_encode_rumble(struct switch2_hd_rumble *rumble, uint8_t buffer[5])
>> +{
>> + buffer[0] = rumble->hi_freq;
>> + buffer[1] = (rumble->hi_freq >> 8) | (rumble->hi_amp << 2);
>> + buffer[2] = (rumble->hi_amp >> 6) | (rumble->lo_freq << 4);
>> + buffer[3] = (rumble->lo_freq >> 4) | (rumble->lo_amp << 6);
>> + buffer[4] = rumble->lo_amp >> 2;
>> +}
>> +
>> +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
>> +{
>> + struct switch2_controller *ns2 = input_get_drvdata(dev);
>> +
>> + if (effect->type != FF_RUMBLE)
>> + return 0;
>> +
>> + guard(spinlock_irqsave)(&ns2->rumble_lock);
>> + if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) {
>> + ns2->rumble.sd.amplitude = max(effect->u.rumble.strong_magnitude,
>> + effect->u.rumble.weak_magnitude >> 1);
>
> Will this max calculation trigger a compiler warning? The strict type checking
> in the max macro might fail because effect->u.rumble.weak_magnitude >> 1
> promotes to an int, while strong_magnitude is a u16.
It doesn't cause a warning locally, but my warnings seem lax for some reason. I'm not sure how to fix that, but I guess adding a cast here is harmless.
>
>> + } else {
>> + ns2->rumble.hd.hi_amp = effect->u.rumble.weak_magnitude * RUMBLE_MAX >> 16;
>> + ns2->rumble.hd.lo_amp = effect->u.rumble.strong_magnitude * RUMBLE_MAX >> 16;
>> + }
>> +
>> + if (ns2->hdev)
>> + schedule_delayed_work(&ns2->rumble_work, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static void switch2_rumble_work(struct work_struct *work)
>> +{
>> + struct switch2_controller *ns2 = container_of(to_delayed_work(work),
>> + struct switch2_controller, rumble_work);
>> + unsigned long current_ms = jiffies_to_msecs(get_jiffies_64());
>> + unsigned long flags;
>> + bool active;
>> + int ret;
>
> [ ... ]
>
>> + if (active) {
>> + unsigned long interval = msecs_to_jiffies(4);
>> +
>> + if (!ns2->last_rumble_work)
>> + ns2->last_rumble_work = current_ms;
>> + else
>> + ns2->last_rumble_work += interval;
>> + schedule_delayed_work(&ns2->rumble_work,
>> + ns2->last_rumble_work + interval - current_ms);
>
> Does this code underflow the scheduled delay?
>
> The current_ms variable is retrieved in milliseconds using jiffies_to_msecs(),
> but interval is calculated in jiffies using msecs_to_jiffies(). Since
> ns2->last_rumble_work is initialized to milliseconds but incremented by
> jiffies, subtracting current_ms could yield a negative value.
>
> On systems where HZ is not 1000, could this underflow the delay parameter and
> schedule the next work millions of jiffies in the future?
Fixing in v6.
>
>> + } else {
>> + ns2->last_rumble_work = 0;
>> + }
>> + spin_unlock_irqrestore(&ns2->rumble_lock, flags);
>> +
>> + if (!ns2->hdev) {
>> + cancel_delayed_work(&ns2->rumble_work);
>> + ret = -ENODEV;
>> + } else {
>> + ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64);
>> + }
>> +
>> + if (ret < 0)
>> + hid_dbg(ns2->hdev, "Failed to send output report ret=%d\n", ret);
>
> If ns2->hdev is NULL, we set ret = -ENODEV and then reach this hid_dbg call.
> Could this result in a NULL pointer dereference, as the hid_dbg macro passes
> ns2->hdev to dev_dbg()?
Fixing in v6
>
> Also, since ns2->hdev is accessed locklessly and without READ_ONCE(), is there
> a time-of-check to time-of-use race where the compiler might reload the pointer
> between the check and hid_hw_output_report()?
This can only get changed in switch2_remove, which is missing a cancel_delayed_work_sync. Fixing in v6.
>
> [ ... ]
>
>> @@ -3881,6 +4048,14 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id
>>
>> switch2_leds_create(ns2);
>>
>> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
>> + if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) {
>> + ns2->rumble.hd.hi_freq = RUMBLE_HI_FREQ;
>> + ns2->rumble.hd.lo_freq = RUMBLE_LO_FREQ;
>> + }
>
> Are we checking ns2->ctlr_type before it is dynamically initialized?
>
> During probe, ns2->ctlr_type appears to be 0 since the struct is freshly
> allocated and the type is discovered later during the init state machine.
> This would cause the high and low frequencies to always be written.
>
> Since hd and sd share a union, could this corrupt the sd.error and
> sd.amplitude fields for actual GameCube controllers?
Really, really unimportant and technically "works by accident" territory but I'll move the logic anyway.
>
>> + spin_lock_init(&ns2->rumble_lock);
>> + INIT_DELAYED_WORK(&ns2->rumble_work, switch2_rumble_work);
>> +#endif
>> hid_set_drvdata(hdev, ns2);
>
^ permalink raw reply
* Re: [PATCH v5 1/3] HID: nintendo: Add preliminary Switch 2 controller driver
From: Vicki Pfau @ 2026-06-19 3:56 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260514001510.964E7C19425@smtp.kernel.org>
Guess I'm doing LLM review replies now.
On 5/13/26 5:15 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
> - [High] Sleeping in atomic context and AB-BA deadlock in `switch2_usb_disconnect`
> - [High] Race condition in IN URB tracking causes permanent starvation
> - [High] Dangling pointers and use-after-free on probe failure
> - [High] Memory corruption and double free due to incorrect devm input lifecycle management
> - [High] NULL pointer dereference in USB probe error path
> - [High] Division by zero crash from unvalidated trigger calibration data
> - [High] BUG_ON kernel panic when `ida_free` is called with U32_MAX
> - [High] Out-of-bounds read when parsing NS2_CMD_FW_INFO response
> - [High] Race condition due to URB submission before initialization of synchronization primitives
> - [Medium] Infinite USB retry storm on protocol errors
> - [Medium] Unaligned memory access on byte array casts
> --
>
> commit 00cb7265ad5983144c6923702e2ee2c3c047d29e
> Author: Vicki Pfau <vi@endrift.com>
> Subject: HID: nintendo: Add preliminary Switch 2 controller driver
>
> This adds a new driver for the Switch 2 controllers. The Switch 2 uses an
> unusual split-interface design such that input and rumble occur on the main
> HID interface, but all other communication occurs over a "configuration"
> interface.
>
>> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
>> index 29008c2cc5304..2193afcdefabc 100644
>> --- a/drivers/hid/hid-nintendo.c
>> +++ b/drivers/hid/hid-nintendo.c
> [ ... ]
>> +static int switch2_init_input(struct switch2_controller *ns2)
>> +{
> [ ... ]
>> + input = devm_input_allocate_device(&hdev->dev);
>> + if (!input)
>> + return -ENOMEM;
> [ ... ]
>> + ret = input_register_device(input);
>> + if (ret < 0) {
>> + hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret);
>> + input_free_device(input);
>
> Calling input_free_device or input_unregister_device on a devm-managed
> input device can cause double frees when devres cleanup fires. Can this
> lead to memory corruption?
False positive. The documentation clearly states there is no devm equivalent and input_unregister_device works fine on devm-managed devices. The source of input_free_device also clearly shows logic for handling devm, and the source of input_device_unregister calls into a static devm_input_device_unregister if needed.
>
>> + return ret;
>> + }
> [ ... ]
>> +static void switch2_controller_put(struct switch2_controller *ns2)
>> +{
>> + struct input_dev *input;
>> + bool do_free;
> [ ... ]
>> + if (input)
>> + input_unregister_device(input);
>
> Calling input_unregister_device on a devm-managed input device can cause
> issues. If the input device remains alive (tied to the HID device devres)
> and its child LEDs are not released, will reconnecting the USB cable
> allocate a new input device and corrupt the LED lists by re-registering
> the exact same ns2->leds structures?
Same false positive
>
>> +
>> + if (do_free) {
> [ ... ]
>> +static void switch2_report_trigger(struct input_dev *input, uint8_t zero, int abs, uint8_t data)
>> +{
>> + int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero);
>
> The zero variable is loaded directly from the controller's factory calibration
> flash. If a device reports a zero point of exactly 232, will this trigger a
> division by zero exception and crash the kernel?
Ok, an actual finding. I'll have this fixed in v6.
>
>> +
>> + input_report_abs(input, abs, clamp(value, 0, NS2_TRIGGER_RANGE));
>> +}
> [ ... ]
>> +static int switch2_read_flash(struct switch2_controller *ns2, uint32_t address,
>> + uint8_t size)
>> +{
>> + uint8_t message[8] = { size, 0x7e };
>> +
>> + if (!ns2->cfg)
>> + return -ENOTCONN;
>> + *(__le32 *)&message[4] = __cpu_to_le32(address);
>
> The driver directly casts &message[4] to __le32. Since message is a uint8_t
> array, it is only guaranteed to be 1-byte aligned. On strict architectures,
> will this unaligned memory write trigger a hardware alignment fault?
Good catch. Fixed in v6.
>
>> + return ns2->cfg->send_command(NS2_CMD_FLASH, NS2_SUBCMD_FLASH_READ, message,
>> + sizeof(message), ns2->cfg);
>> +}
> [ ... ]
>> +int switch2_receive_command(struct switch2_controller *ns2,
>> + const uint8_t *message, size_t length)
>> +{
> [ ... ]
>> + header = (const struct switch2_cmd_header *)message;
>> + if (!(header->flags & NS2_FLAG_OK)) {
>> + ret = -EIO;
>> + goto exit;
>> + }
>> + message = &message[8];
>> + switch (header->command) {
> [ ... ]
>> + case NS2_CMD_FW_INFO:
>> + if (header->subcommand == NS2_SUBCMD_FW_INFO_GET) {
>> + if (length < sizeof(ns2->version)) {
>> + ret = -EINVAL;
>> + goto exit;
>> + }
>> + memcpy(&ns2->version, message, sizeof(ns2->version));
>
> The message pointer is advanced by 8 bytes to skip the header, but length
> is not decremented. The bounds check compares length against
> sizeof(ns2->version). If length is exactly sizeof(ns2->version), the check
> passes, but the advanced message pointer only has length - 8 valid bytes
> remaining. Will the memcpy over-read into uninitialized memory?
Fixed in v6.
>
>> + ns2->ctlr_type = ns2->version.ctlr_type;
>> + switch2_init_step_done(ns2, NS2_INIT_GET_FIRMWARE_INFO);
>> + }
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> +exit:
>> + if (ns2->init_step < NS2_INIT_DONE)
>> + switch2_init_controller(ns2);
>
> If a command response has the NACK flag set or encounters another error,
> the code jumps to exit and invokes switch2_init_controller. Since
> ns2->init_step has not advanced, the state machine will immediately re-send
> the exact same command. Can this cause an infinite tight feedback loop of
> failures and retries, flooding the USB bus?
This doesn't happen for problems already described in the thread previously.
>
>> +
>> + return ret;
>> +}
> [ ... ]
>> +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg)
>> +{
> [ ... ]
>> + guard(mutex)(&ns2->lock);
>> + WARN_ON(ns2->cfg);
>> + ns2->cfg = cfg;
>> +
>> + if (ns2->hdev)
>> + return switch2_init_controller(ns2);
>
> If switch2_init_controller fails here or in switch2_probe, the error is
> returned immediately without unrolling the state. Will ns2->cfg or ns2->hdev
> remain set, potentially leading to a use-after-free when subsequent device
> events occur?
This is a good catch. I'm not sure what the right fix is though; obviously a half-configured state is bad, but I'm not sure a full teardown is the best approach; it can leave it in a weird state, even if it's not a UAF risk. I'll go with a full teardown for now at least.
>
>> + return 0;
>> +}
> [ ... ]
>> +static void switch2_remove(struct hid_device *hdev)
>> +{
>> + struct switch2_controller *ns2 = hid_get_drvdata(hdev);
>> +
>> + hid_hw_close(hdev);
>> + mutex_lock(&ns2->lock);
>> + WARN_ON(ns2->hdev != hdev);
>> + ns2->hdev = NULL;
>> + mutex_unlock(&ns2->lock);
>> + ida_free(&nintendo_player_id_allocator, ns2->player_id);
>
> If ida_alloc fails during probe, ns2->player_id remains at its default
> value of U32_MAX. When ida_free is called, the IDA framework enforces a
> check on the ID. Casting U32_MAX to a signed integer yields -1. Will this
> trigger the BUG_ON and cause a kernel panic?
I can't find any evidence of such a BUG_ON. A negative number will just early return. This seems like a hallucination.
>
>> + switch2_controller_put(ns2);
>> + hid_hw_stop(hdev);
>> +}
> [ ... ]
>> diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c
>> new file mode 100644
>> index 0000000000000..ebd89d852e21a
>> --- /dev/null
>> +++ b/drivers/input/joystick/nintendo-switch2-usb.c
> [ ... ]
>> +static void switch2_bulk_in(struct urb *urb)
>> +{
> [ ... ]
>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
>> + for (i = 0; i < NS2_IN_URBS; i++) {
>> + int err;
>> + struct switch2_urb *ns2_urb;
>> +
>> + if (ns2_usb->bulk_in[i].urb == urb) {
>> + ns2_usb->message_in = i;
>
> If multiple URBs complete in quick succession before the work item executes,
> the message_in index is overwritten. Will this cause the work item to only
> process the last completed URB, leaving earlier URBs indefinitely active
> and leading to starvation of the URB pool?
>
In practice this probably won't happen but I guess it's worth fixing.
>> + continue;
>> + }
> [ ... ]
>> +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
>> +{
> [ ... ]
>> + ns2_usb->bulk_in[0].active = true;
>> + ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC);
>> + if (ret < 0)
>> + goto err_free_out;
>> +
>> + init_usb_anchor(&ns2_usb->bulk_out_anchor);
>> + spin_lock_init(&ns2_usb->bulk_out_lock);
>> + init_usb_anchor(&ns2_usb->bulk_in_anchor);
>> + spin_lock_init(&ns2_usb->bulk_in_lock);
>> + INIT_WORK(&ns2_usb->message_in_work, switch2_usb_message_in_work);
>
> usb_submit_urb is invoked for bulk_in[0] before the initialization of the
> spinlocks, anchors, and work struct. If the URB completes immediately, the
> completion handler will execute and attempt to use uninitialized
> synchronization primitives. Can this cause a crash?
I don't know if this is possible but I guess I'll fix it.
>
>> +
>> + usb_set_intfdata(intf, ns2_usb);
> [ ... ]
>> +err_kill_urb:
>> + usb_kill_urb(ns2_usb->bulk_in[0].urb);
>> +err_free_out:
>> + for (i = 0; i < NS2_OUT_URBS; i++) {
>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data,
>> + ns2_usb->bulk_out[i].urb->transfer_dma);
>> + usb_free_urb(ns2_usb->bulk_out[i].urb);
>> + }
>> +err_free_in:
>> + for (i = 0; i < NS2_IN_URBS; i++) {
>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data,
>> + ns2_usb->bulk_in[i].urb->transfer_dma);
>
> If usb_alloc_urb fails during the loop, it jumps to err_free_in. During
> cleanup, it accesses ns2_usb->bulk_in[i].urb->transfer_dma. Since urb is
> NULL for the failed index, will this trigger a NULL pointer dereference?
I removed the NULL check due to checkpatch telling me I didn't need it but I guess I'll put it back.
>
>> + usb_free_urb(ns2_usb->bulk_in[i].urb);
>> + }
>> + devm_kfree(&intf->dev, ns2_usb);
>> +
>> + return ret;
>> +}
>> +
>> +static void switch2_usb_disconnect(struct usb_interface *intf)
>> +{
>> + struct switch2_usb *ns2_usb = usb_get_intfdata(intf);
>> + unsigned long flags;
>> + int i;
>> +
>> + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags);
>> + usb_kill_anchored_urbs(&ns2_usb->bulk_out_anchor);
>
> Both usb_kill_anchored_urbs and cancel_work_sync can sleep. Is it safe to
> call them while holding a spinlock?
Fixing in v6.
>
>> + for (i = 0; i < NS2_OUT_URBS; i++) {
>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data,
>> + ns2_usb->bulk_out[i].urb->transfer_dma);
>> + usb_free_urb(ns2_usb->bulk_out[i].urb);
>> + }
>> + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags);
>> +
>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
>> + usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
>> + cancel_work_sync(&ns2_usb->message_in_work);
>
> Furthermore, cancel_work_sync waits for message_in_work which attempts to
> acquire bulk_in_lock. Does this cause a deadlock since the lock is already
> held by the disconnecting thread?
Likewise.
>
>> + for (i = 0; i < NS2_IN_URBS; i++) {
>
^ permalink raw reply
* Re: [PATCH v5 1/3] HID: nintendo: Add preliminary Switch 2 controller driver
From: Vicki Pfau @ 2026-06-19 4:02 UTC (permalink / raw)
To: Silvan Jegen, sashiko-reviews; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <25DJXAMCCCNLA.234CWZVQ8IOM4@homearch.localdomain>
On 5/23/26 1:57 AM, Silvan Jegen wrote:
> Heyhey!
>
> Below some comments regarding the findings (note that I am not an expert
> on neither Linux kernel drivers in general nor USB HID specifically).
>
> sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
>> - [High] Sleeping in atomic context and AB-BA deadlock in `switch2_usb_disconnect`
>> - [High] Race condition in IN URB tracking causes permanent starvation
>> - [High] Dangling pointers and use-after-free on probe failure
>> - [High] Memory corruption and double free due to incorrect devm input lifecycle management
>> - [High] NULL pointer dereference in USB probe error path
>> - [High] Division by zero crash from unvalidated trigger calibration data
>> - [High] BUG_ON kernel panic when `ida_free` is called with U32_MAX
>> - [High] Out-of-bounds read when parsing NS2_CMD_FW_INFO response
>> - [High] Race condition due to URB submission before initialization of synchronization primitives
>> - [Medium] Infinite USB retry storm on protocol errors
>> - [Medium] Unaligned memory access on byte array casts
>> --
>>
>> commit 00cb7265ad5983144c6923702e2ee2c3c047d29e
>> Author: Vicki Pfau <vi@endrift.com>
>> Subject: HID: nintendo: Add preliminary Switch 2 controller driver
>>
>> This adds a new driver for the Switch 2 controllers. The Switch 2 uses an
>> unusual split-interface design such that input and rumble occur on the main
>> HID interface, but all other communication occurs over a "configuration"
>> interface.
>>
>>> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
>>> index 29008c2cc5304..2193afcdefabc 100644
>>> --- a/drivers/hid/hid-nintendo.c
>>> +++ b/drivers/hid/hid-nintendo.c
>> [ ... ]
>>> +static int switch2_init_input(struct switch2_controller *ns2)
>>> +{
>> [ ... ]
>>> + input = devm_input_allocate_device(&hdev->dev);
>>> + if (!input)
>>> + return -ENOMEM;
>> [ ... ]
>>> + ret = input_register_device(input);
>>> + if (ret < 0) {
>>> + hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret);
>>> + input_free_device(input);
>>
>> Calling input_free_device or input_unregister_device on a devm-managed
>> input device can cause double frees when devres cleanup fires. Can this
>> lead to memory corruption?
>
> I think sashiko is actually correct here, unfortunately. The documentation
> for devm_input_allocate_device states that
>
> Managed input devices do not need to be explicitly unregistered or
> freed as it will be done automatically when owner device unbinds from
> its driver (or binding fails). ...
>
> Looks like we can remove all the input_unregister_device and
> input_free_device calls here.
sashiko is not correct. See my previous reply.
>
>>
>>> + return ret;
>>> + }
>> [ ... ]
>>> +static void switch2_controller_put(struct switch2_controller *ns2)
>>> +{
>>> + struct input_dev *input;
>>> + bool do_free;
>> [ ... ]
>>> + if (input)
>>> + input_unregister_device(input);
>>
>> Calling input_unregister_device on a devm-managed input device can cause
>> issues. If the input device remains alive (tied to the HID device devres)
>> and its child LEDs are not released, will reconnecting the USB cable
>> allocate a new input device and corrupt the LED lists by re-registering
>> the exact same ns2->leds structures?
>
> This input_unregister_device can also be removed.
>
>>
>>> +
>>> + if (do_free) {
>> [ ... ]
>>> +static void switch2_report_trigger(struct input_dev *input, uint8_t zero, int abs, uint8_t data)
>>> +{
>>> + int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero);
>>
>> The zero variable is loaded directly from the controller's factory calibration
>> flash. If a device reports a zero point of exactly 232, will this trigger a
>> division by zero exception and crash the kernel?
>
> Not sure this is an issue in practice, but we might want to protect the
> code in any case by doing something like the below perhaps (untested).
>
> int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / max_t(int, (232 - zero), 1);
>
>
It won't be an issue in practice but it's harmless to fix.
>>
>>> +
>>> + input_report_abs(input, abs, clamp(value, 0, NS2_TRIGGER_RANGE));
>>> +}
>> [ ... ]
>>> +static int switch2_read_flash(struct switch2_controller *ns2, uint32_t address,
>>> + uint8_t size)
>>> +{
>>> + uint8_t message[8] = { size, 0x7e };
>>> +
>>> + if (!ns2->cfg)
>>> + return -ENOTCONN;
>>> + *(__le32 *)&message[4] = __cpu_to_le32(address);
>>
>> The driver directly casts &message[4] to __le32. Since message is a uint8_t
>> array, it is only guaranteed to be 1-byte aligned. On strict architectures,
>> will this unaligned memory write trigger a hardware alignment fault?
>
> I assume this could be an issue and we should just use something like
> the below (only compile-tested).
>
> put_unaligned_le32(__cpu_to_le32(address), &message[4]);
>
Correct.
>
>>
>>> + return ns2->cfg->send_command(NS2_CMD_FLASH, NS2_SUBCMD_FLASH_READ, message,
>>> + sizeof(message), ns2->cfg);
>>> +}
>> [ ... ]
>>> +int switch2_receive_command(struct switch2_controller *ns2,
>>> + const uint8_t *message, size_t length)
>>> +{
>> [ ... ]
>>> + header = (const struct switch2_cmd_header *)message;
>>> + if (!(header->flags & NS2_FLAG_OK)) {
>>> + ret = -EIO;
>>> + goto exit;
>>> + }
>>> + message = &message[8];
>>> + switch (header->command) {
>> [ ... ]
>>> + case NS2_CMD_FW_INFO:
>>> + if (header->subcommand == NS2_SUBCMD_FW_INFO_GET) {
>>> + if (length < sizeof(ns2->version)) {
>>> + ret = -EINVAL;
>>> + goto exit;
>>> + }
>>> + memcpy(&ns2->version, message, sizeof(ns2->version));
>>
>> The message pointer is advanced by 8 bytes to skip the header, but length
>> is not decremented. The bounds check compares length against
>> sizeof(ns2->version). If length is exactly sizeof(ns2->version), the check
>> passes, but the advanced message pointer only has length - 8 valid bytes
>> remaining. Will the memcpy over-read into uninitialized memory?
>
> Sounds like something that we should fix by either decreasing length or
> increasing the size in the check.
>
>>
>>> + ns2->ctlr_type = ns2->version.ctlr_type;
>>> + switch2_init_step_done(ns2, NS2_INIT_GET_FIRMWARE_INFO);
>>> + }
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> +
>>> +exit:
>>> + if (ns2->init_step < NS2_INIT_DONE)
>>> + switch2_init_controller(ns2);
>>
>> If a command response has the NACK flag set or encounters another error,
>> the code jumps to exit and invokes switch2_init_controller. Since
>> ns2->init_step has not advanced, the state machine will immediately re-send
>> the exact same command. Can this cause an infinite tight feedback loop of
>> failures and retries, flooding the USB bus?
>
> We have discussed this before and it doesn't seem to be an issue in
> practice.
>
> If we want to be on the safe side, we could pass a retry count and abort
> on too many error retries, perhaps?
I had considered a retry count but getting it to work properly was fiddly. This should only go wrong if there's a malicious device attached, as in practice the Switch 2 controllers won't do this, and bus abuse is trivial with a malicious device regardless.
>
>
>>> +
>>> + return ret;
>>> +}
>> [ ... ]
>>> +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg)
>>> +{
>> [ ... ]
>>> + guard(mutex)(&ns2->lock);
>>> + WARN_ON(ns2->cfg);
>>> + ns2->cfg = cfg;
>>> +
>>> + if (ns2->hdev)
>>> + return switch2_init_controller(ns2);
>>
>> If switch2_init_controller fails here or in switch2_probe, the error is
>> returned immediately without unrolling the state. Will ns2->cfg or ns2->hdev
>> remain set, potentially leading to a use-after-free when subsequent device
>> events occur?
>
> AFAICT, since we are always checking for both ns2->cfg and ns2->hdev
> before doing any processing in switch2_init_controller, there should
> never be a use-after-free
>
> It *is* true that we don't call hid_hw_{close,stop} in switch2_probe,
> if the switch2_init_controller call fails though. It might make sense
> to return a specific error and use it to check whether we want to give
> up and call these?
The problem was that it will return an error after setting ns2->cfg, which causes the caller to free it without removing it. That's a real bug that can cause UAF.
>
>>
>>> + return 0;
>>> +}
>> [ ... ]
>>> +static void switch2_remove(struct hid_device *hdev)
>>> +{
>>> + struct switch2_controller *ns2 = hid_get_drvdata(hdev);
>>> +
>>> + hid_hw_close(hdev);
>>> + mutex_lock(&ns2->lock);
>>> + WARN_ON(ns2->hdev != hdev);
>>> + ns2->hdev = NULL;
>>> + mutex_unlock(&ns2->lock);
>>> + ida_free(&nintendo_player_id_allocator, ns2->player_id);
>>
>> If ida_alloc fails during probe, ns2->player_id remains at its default
>> value of U32_MAX. When ida_free is called, the IDA framework enforces a
>> check on the ID. Casting U32_MAX to a signed integer yields -1. Will this
>> trigger the BUG_ON and cause a kernel panic?
>
> ida_free seems to return early on
>
> if ((int)id < 0)
> return;
>
> so that should be fine as is.
>
>>
>>> + switch2_controller_put(ns2);
>>> + hid_hw_stop(hdev);
>>> +}
>> [ ... ]
>>> diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c
>>> new file mode 100644
>>> index 0000000000000..ebd89d852e21a
>>> --- /dev/null
>>> +++ b/drivers/input/joystick/nintendo-switch2-usb.c
>> [ ... ]
>>> +static void switch2_bulk_in(struct urb *urb)
>>> +{
>> [ ... ]
>>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
>>> + for (i = 0; i < NS2_IN_URBS; i++) {
>>> + int err;
>>> + struct switch2_urb *ns2_urb;
>>> +
>>> + if (ns2_usb->bulk_in[i].urb == urb) {
>>> + ns2_usb->message_in = i;
>>
>> If multiple URBs complete in quick succession before the work item executes,
>> the message_in index is overwritten. Will this cause the work item to only
>> process the last completed URB, leaving earlier URBs indefinitely active
>> and leading to starvation of the URB pool?
>>
>>> + continue;
>>> + }
>> [ ... ]
>>> +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>> +{
>> [ ... ]
>>> + ns2_usb->bulk_in[0].active = true;
>>> + ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC);
>>> + if (ret < 0)
>>> + goto err_free_out;
>>> +
>>> + init_usb_anchor(&ns2_usb->bulk_out_anchor);
>>> + spin_lock_init(&ns2_usb->bulk_out_lock);
>>> + init_usb_anchor(&ns2_usb->bulk_in_anchor);
>>> + spin_lock_init(&ns2_usb->bulk_in_lock);
>>> + INIT_WORK(&ns2_usb->message_in_work, switch2_usb_message_in_work);
>>
>> usb_submit_urb is invoked for bulk_in[0] before the initialization of the
>> spinlocks, anchors, and work struct. If the URB completes immediately, the
>> completion handler will execute and attempt to use uninitialized
>> synchronization primitives. Can this cause a crash?
>
> As the documentation for usb_submit_urb mentions that the call is async,
> I assume that this is indeed a theoretical possibility.
>
>>
>>> +
>>> + usb_set_intfdata(intf, ns2_usb);
>> [ ... ]
>>> +err_kill_urb:
>>> + usb_kill_urb(ns2_usb->bulk_in[0].urb);
>>> +err_free_out:
>>> + for (i = 0; i < NS2_OUT_URBS; i++) {
>>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data,
>>> + ns2_usb->bulk_out[i].urb->transfer_dma);
>>> + usb_free_urb(ns2_usb->bulk_out[i].urb);
>>> + }
>>> +err_free_in:
>>> + for (i = 0; i < NS2_IN_URBS; i++) {
>>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data,
>>> + ns2_usb->bulk_in[i].urb->transfer_dma);
>>
>> If usb_alloc_urb fails during the loop, it jumps to err_free_in. During
>> cleanup, it accesses ns2_usb->bulk_in[i].urb->transfer_dma. Since urb is
>> NULL for the failed index, will this trigger a NULL pointer dereference?
>
> Yes, most likely we would need a new label for this case.
>
>>
>>> + usb_free_urb(ns2_usb->bulk_in[i].urb);
>>> + }
>>> + devm_kfree(&intf->dev, ns2_usb);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void switch2_usb_disconnect(struct usb_interface *intf)
>>> +{
>>> + struct switch2_usb *ns2_usb = usb_get_intfdata(intf);
>>> + unsigned long flags;
>>> + int i;
>>> +
>>> + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags);
>>> + usb_kill_anchored_urbs(&ns2_usb->bulk_out_anchor);
>>
>> Both usb_kill_anchored_urbs and cancel_work_sync can sleep. Is it safe to
>> call them while holding a spinlock?
>
> According to some googling, this indeed seems to be an isuse. The
> non-sleeping way to cancel transfer requests would be to use
> https://www.kernel.org/doc/html/v6.1/driver-api/usb/usb.html#c.usb_unlink_anchored_urbs
This was removed in 6.17 and has been unused since 2009.
>
>
>>
>>> + for (i = 0; i < NS2_OUT_URBS; i++) {
>>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data,
>>> + ns2_usb->bulk_out[i].urb->transfer_dma);
>>> + usb_free_urb(ns2_usb->bulk_out[i].urb);
>>> + }
>>> + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags);
>>> +
>>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
>>> + usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
>>> + cancel_work_sync(&ns2_usb->message_in_work);
>>
>> Furthermore, cancel_work_sync waits for message_in_work which attempts to
>> acquire bulk_in_lock. Does this cause a deadlock since the lock is already
>> held by the disconnecting thread?
>
> Yes, this looks like a real concern as well.
>
> Cheers,
> Silvan
>
>>
>>> + for (i = 0; i < NS2_IN_URBS; i++) {
^ permalink raw reply
* Re: [PATCH v5 4/5] HID: asus: add support for xgm led
From: sashiko-bot @ 2026-06-19 0:23 UTC (permalink / raw)
To: Denis Benato; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260619001103.1189200-5-denis.benato@linux.dev>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] A failure to initialize the cosmetic XG Mobile LED aborts the entire HID device probe.
- [High] Failing to restore the XGM LED brightness causes the entire HID device resume process to fail.
Pre-existing issues:
- [High] The devm-managed battery is not explicitly unregistered before stopping the hardware, leading to use-after-stop during teardown.
--
commit 7aa41eaa0bd3e715bef4c850703c7d47445cad14
Author: Denis Benato <denis.benato@linux.dev>
HID: asus: add support for xgm led
XG mobile stations have very bright leds behind the fan that can be
turned either ON or OFF: add a cled interface to allow controlling the
brightness of those red leds.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 0fb8cd6437b77..06e4888f090ca 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1361,7 +1432,17 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
> if (drvdata->listener.brightness_set)
> asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>
> + if (drvdata->xgm_led) {
> + ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> + if (ret) {
> + hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
> + goto asus_resume_err;
> + }
> + }
> +
> return 0;
> +asus_resume_err:
> + return ret;
[Severity: High]
In asus_resume(), if asus_xgm_led_set() fails to restore the LED state,
returning an error causes the entire HID device resume process to fail.
Will this leave the device in an inconsistent state and break input
functionality?
As documented in the comments for the backlight, non-fatal LED state
restorations shouldn't fail the system resume. Could this just ignore the error
here as well?
[ ... ]
> @@ -1491,6 +1572,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
> }
>
> + if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> + ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> + (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> + ret = asus_xgm_init(hdev, drvdata);
> + if (ret) {
> + hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
> + goto err_stop_hw;
> + }
> + }
> +
[Severity: High]
If asus_xgm_init() fails due to a transient USB error during the initial
asus_xgm_led_set() call, will this completely abort the HID device probe and
disable the keyboard?
Since similar features like the keyboard backlight only emit a warning on
failure, should a non-fatal cosmetic LED initialization failure also just warn
and allow the driver to continue normal operation?
[ ... ]
> @@ -1540,6 +1631,9 @@ static void asus_remove(struct hid_device *hdev)
> if (drvdata->listener.brightness_set)
> asus_hid_unregister_listener(&drvdata->listener);
>
> + if (drvdata->xgm_led)
> + devm_led_classdev_unregister(&hdev->dev, &drvdata->xgm_led->cdev);
> +
> asus_worker_stop(drvdata->worker);
> hid_hw_stop(hdev);
> }
[Severity: High]
This is a pre-existing issue, but the devm-managed battery is not explicitly
unregistered before stopping the hardware.
Because the battery is registered via devm_power_supply_register() before
hid_hw_start(), and hid_hw_stop() is called here before the devres cleanup,
could this lead to a use-after-stop?
If a concurrent read of the battery sysfs properties happens during teardown, it
triggers asus_battery_get_property() which calls hid_hw_raw_request() on a
stopped HID device. Can this cause bus hangs or undefined behavior?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619001103.1189200-1-denis.benato@linux.dev?part=4
^ permalink raw reply
* Re: [PATCH v5 1/5] HID: asus: refactor the two workqueues and init sequence
From: sashiko-bot @ 2026-06-19 0:23 UTC (permalink / raw)
To: Denis Benato; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260619001103.1189200-2-denis.benato@linux.dev>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ABBA deadlock between driver removal/probe failure and the deferred WMI_FAN action in the workqueue due to inverted lock acquisition order of `hdev->driver_input_lock`.
- [High] Unbounded dynamic allocation of workqueue actions allows an untrusted USB device or unprivileged user to exhaust kernel atomic memory pools.
- [Medium] Passing the raw packet size as `bufsize` to `hid_report_raw_event()` bypasses the HID core's automatic padding, leading to valid short packets being incorrectly rejected.
--
commit 656f5a6cc923dccccd418cd0aefa2239f221bf47
Author: Denis Benato <denis.benato@linux.dev>
HID: asus: refactor the two workqueues and init sequence
Multiple issues have been found within the hid-asus driver. This patch
joins the two workqueues into one and reuses the stopping mechanism
of the brightness workqueue. It also moves the asus_wmi_send_event()
sleeping call away from atomic context.
Link: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index d34d74df3dc0b..40191eb9e2e86 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
> }
[ ... ]
> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
> + size_t report_size)
> +{
> + struct asus_work_action *action;
> + unsigned long flags;
> +
> + if (report_size > FEATURE_KBD_REPORT_SIZE) {
> + hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
> + return -EINVAL;
> }
>
> + action = kzalloc(sizeof(struct asus_work_action), GFP_NOWAIT);
[Severity: High]
Does this code allow an unbounded number of allocations?
If a device sends a flood of fan events, or if a local user rapidly changes
the brightness, it seems this could queue an unlimited number of actions
using GFP_NOWAIT or GFP_ATOMIC.
Could this exhaust kernel atomic memory pools and lead to a local denial of
service?
> + if (!action)
> + return -ENOMEM;
> +
> + action->type = WMI_FAN;
> + action->data.fan_hid_data.report_size = report_size;
> + memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
> + INIT_LIST_HEAD(&action->node);
> +
> + spin_lock_irqsave(&drvdata->worker->lock, flags);
> + asus_worker_schedule(drvdata->worker, action);
> + spin_unlock_irqrestore(&drvdata->worker->lock, flags);
>
> return 0;
> }
[ ... ]
> @@ -569,59 +668,157 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
[ ... ]
> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
> {
[ ... ]
> + /*
> + * Fallback: pass the raw event to the HID core; to avoid
> + * racing against the hid_report_raw_event() that generated
> + * this event use the same locking mechanism and wait for
> + * that function to terminate and signal the deferred execution
> + * before raising the stored event.
> + */
> + down(&hdev->driver_input_lock);
[Severity: High]
Can this code cause an ABBA deadlock during device probe failure or removal?
The HID core holds hdev->driver_input_lock during hid_device_probe() and
hid_device_remove().
If asus_remove() or the error path in asus_probe() calls asus_worker_stop()
and blocks on cancel_work_sync(&worker->work) while driver_input_lock is
held:
HID core thread:
hid_device_remove() (acquires driver_input_lock)
-> asus_remove()
-> asus_worker_stop()
-> cancel_work_sync() (waits for worker)
Worker thread:
asus_work()
-> asus_kbd_wmi_fan()
-> down(&hdev->driver_input_lock) (waits for HID core thread)
Would this lead to an unrecoverable deadlock?
> + hid_report_raw_event(hdev, HID_INPUT_REPORT,
> + data->report_data, data->report_size,
> + data->report_size, 1);
[Severity: Medium]
Does passing data->report_size as the bufsize argument drop valid short
packets?
The HID core natively handles short packets by padding the buffer up to the
expected report descriptor size (rsize), provided the allocated buffer size
(bufsize) is large enough.
If bufsize is artificially restricted to data->report_size, the safety check
"if (bsize < rsize)" inside hid_report_raw_event() might falsely trigger and
reject the valid short event with -EINVAL.
Should FEATURE_KBD_REPORT_SIZE be used as the bufsize parameter instead?
> + up(&hdev->driver_input_lock);
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619001103.1189200-1-denis.benato@linux.dev?part=1
^ permalink raw reply
* [PATCH v5 0/5] HID: asus: security fixes and more hardware support
From: Denis Benato @ 2026-06-19 0:10 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Luke D . Jones,
Mateusz Schyboll, Denis Benato, Denis Benato, Antheas Kapenekakis,
Connor Belli
Hi all,
I have added support for controlling the (way too bright) XG mobile
LEDs in hid-asus and added the i2c version of already supported
hardware that was probed only when it's a USB: these are two separate
features changes and are the only two that are not fixes for
pre-existing issue (see below).
Auto-review bot has spotted a bunch of pre-existing problems alongside
problems in my own code therefore at this point I am going to fix the
more problems I can and including those fixes and improvements in this
patchset.
For v4 I decided to follow Antheas' suggestion of reusing the existing
workqueue and by making it more generic I solved a good bunch of issues.
The v5 iteration is simply me fixing a bunch of bugs in new code spotted
by the bot. Thanks for providing such a useful tool!
On a side node this patchset has a few more warnings: specifically
"WARNING: Prefer kzalloc_obj over kzalloc with sizeof" but it's a false
positive as that would introduce sleeping calls in atomic contexts.
Link: https://lore.kernel.org/all/20260612124830.984592-1-denis.benato@linux.dev/
Link: https://lore.kernel.org/all/30258f28-4036-4e99-9494-031212544dc2@linux.dev/
Best regards,
Denis Benato
Link v2: https://lore.kernel.org/all/20260612142326.1704858-1-denis.benato@linux.dev/
Link v3: https://lore.kernel.org/all/20260613153029.2559774-1-denis.benato@linux.dev/
Link v4: https://lore.kernel.org/all/20260615165058.3845-1-denis.benato@linux.dev/
Changelog:
- v1
- Initial submission (as separate patches)
- v2
- HID: asus: add i2c entry for FA808UM and other TUFs
- Joined to the patchset
- HID: asus: add support for xgm led
- Use the correct callback set_brightness_blocking
- Stop hardcoding the cled name
- HID: asus: prevent wrong pointer cast
- Added
- HID: asus: mitigate possible use-after-free
- Added
- v3
- HID: asus: add support for xgm led
- check devm_kasprintf result
- HID: asus: prevent a late KEY_FN_ESC to trigger a use-after-free
- Added
- HID: asus: avoid sleeping calls in atomic context
- Added
- HID: asus: cleanup keyboard listener on failure: avoid use-after-free
- Added
- v4
- HID: asus: refactor the two workqueues and init sequence
- Added to replace previous patches in a clearer way
- HID: asus: add support for xgm led
- Simplify cled interface allocation and registration
- v5
- HID: asus: refactor the two workqueues and init sequence
- replace kzalloc_obj in atomix context with kzalloc(..., GFP_ATOMIC/NOWAIT)
- better destroy the queue on _probe in case of errors
- Fix a data race for deferred asus_kbd_wmi_fan()
- HID: asus: add support for xgm led
- improve teardown of the cled interface
Denis Benato (5):
HID: asus: refactor the two workqueues and init sequence
HID: asus: remove extraneous OOM error
HID: asus: fix a off-by-one in mcu_parse_version_string() validation
HID: asus: add support for xgm led
HID: asus: add i2c entry for FA808UM and other TUFs
drivers/hid/hid-asus.c | 491 ++++++++++++++++++++++++++++++++---------
1 file changed, 382 insertions(+), 109 deletions(-)
--
2.47.3
^ permalink raw reply
* [PATCH v5 5/5] HID: asus: add i2c entry for FA808UM and other TUFs
From: Denis Benato @ 2026-06-19 0:11 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Luke D . Jones,
Mateusz Schyboll, Denis Benato, Denis Benato, Antheas Kapenekakis,
Connor Belli
In-Reply-To: <20260619001103.1189200-1-denis.benato@linux.dev>
On newer TUF laptops the keyboard HID device uses the same PID/VID of a
USB device that was found in ROG laptops: add it to hid-asus as i2c too.
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
drivers/hid/hid-asus.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 06e4888f090c..b155eaf106a5 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1770,6 +1770,9 @@ static const struct hid_device_id asus_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_HID_FN_LOCK },
+ { HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
+ USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
+ QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_HID_FN_LOCK },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
--
2.47.3
^ permalink raw reply related
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