* Re: [PATCH] HID: nintendo: register input device after capabilities are set
2026-07-30 10:15 [PATCH] HID: nintendo: register input device after capabilities are set Jiangshan Yi
@ 2026-07-30 10:44 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-30 10:44 UTC (permalink / raw)
To: Jiangshan Yi; +Cc: dmitry.torokhov, linux-input
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] The function `joycon_config_rumble()` sets the `EV_FF` capability unconditionally and ignores the return value of `input_ff_create_memless()`. If memory allocation fails, `dev->ff` remains `NULL`. The device is subsequently successfully registered. When a userspace application issues a force-feedback ioctl (`EVIOCSFF`), `input_ff_upload()` checks `test_bit(EV_FF, dev->evbit)` which passes, and then dereferences the `NULL` `dev->ff` pointer, causing a kernel panic.
- [High] Insufficient length checking on incoming HID reports allows an out-of-bounds read on the input buffer, potentially leaking uninitialized kernel heap data or causing a crash.
- [Medium] The allocated `player_id` IDA slot is leaked if `joycon_leds_create()` fails after successful IDA allocation.
- [Medium] Calling `ida_free()` with `U32_MAX` on a probe failure path causes a kernel warning.
--
--- Patch [1]: [PATCH] HID: nintendo: register input device after capabilities are set ---
commit 346545e7ce7a9520ab6a8be23f83f920de4db331
Author: Jiangshan Yi <yijiangshan@kylinos.cn>
HID: nintendo: register input device after capabilities are set
This commit moves input_register_device() to the end of
joycon_input_create(). This prevents a race condition where EVIOCSFF could
dereference a NULL dev->ff, and ensures the initial udev event has complete
button and axis information.
Link: https://sashiko.dev/#/patchset/20260730031927.25444-1-yijiangshan@kylinos.cn?part=1
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index e7302ec01ff1f..7f014f3718004 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2162,10 +2162,6 @@ static int joycon_input_create(struct joycon_ctlr *ctlr)
> ctlr->input->phys = hdev->phys;
> input_set_drvdata(ctlr->input, ctlr);
>
> - ret = input_register_device(ctlr->input);
> - if (ret)
> - return ret;
> -
> if (joycon_type_is_right_joycon(ctlr)) {
> joycon_config_right_stick(ctlr->input);
[ ... ]
> @@ -2208,6 +2204,10 @@ static int joycon_input_create(struct joycon_ctlr *ctlr)
> if (joycon_has_rumble(ctlr))
> joycon_config_rumble(ctlr);
[Severity: High]
This is a pre-existing issue, but does joycon_config_rumble() properly
handle memory allocation failures?
If input_ff_create_memless() fails due to memory pressure, dev->ff remains
NULL. However, the EV_FF capability is still unconditionally set:
joycon_config_rumble() {
...
input_set_capability(ctlr->input, EV_FF, FF_RUMBLE);
input_ff_create_memless(ctlr->input, NULL, joycon_play_effect);
...
}
Could this lead to a kernel panic? If a userspace application subsequently
issues an EVIOCSFF ioctl, input_ff_upload() checks for EV_FF, which passes,
and then it might dereference the NULL dev->ff pointer.
> + ret = input_register_device(ctlr->input);
> + if (ret)
> + return ret;
> +
> return 0;
> }
[Severity: Medium]
This isn't a bug introduced by this patch, but does the driver leak the
player_id IDA slot if joycon_leds_create() fails?
In joycon_probe(), if a later step fails, the error path goes to err_close,
which bypasses the err_ida cleanup label:
joycon_probe() {
...
ret = joycon_leds_create(ctlr);
if (ret) {
hid_err(hdev, "Failed to create leds; ret=%d\n", ret);
goto err_close;
}
...
err_close:
...
}
Could this result in permanently leaking the allocated IDA slot since
err_ida is skipped?
[Severity: Medium]
This is a pre-existing issue, but can the driver trigger a kernel warning
by freeing an invalid IDA?
If ida_alloc() fails in joycon_leds_create(), ctlr->player_id is set to
U32_MAX:
joycon_leds_create() {
ctlr->player_id = U32_MAX;
ret = ida_alloc(&nintendo_player_id_allocator, GFP_KERNEL);
if (ret < 0) {
hid_warn(hdev, "Failed to allocate player ID...\n", ret);
goto home_led;
}
...
}
If a subsequent initialization step in joycon_probe() fails, the error path
jumps to err_ida:
joycon_probe() {
...
err_ida:
ida_free(&nintendo_player_id_allocator, ctlr->player_id);
}
Does this unconditionally call ida_free() with U32_MAX, triggering a
WARN_ON_ONCE in the IDA subsystem?
[Severity: High]
This isn't a bug introduced by this patch, but is there an out-of-bounds
read vulnerability when parsing incoming HID reports?
In joycon_ctlr_read_handler(), the length check only ensures the payload is
at least 12 bytes:
joycon_ctlr_read_handler() {
...
if (size >= 12)
joycon_parse_report(ctlr, (struct joycon_input_report *)data);
...
}
However, if the report ID is JC_INPUT_IMU_DATA, joycon_parse_report()
delegates to joycon_parse_imu_report():
joycon_parse_report() {
...
if ((rep->id == JC_INPUT_IMU_DATA) && joycon_has_imu(ctlr))
joycon_parse_imu_report(ctlr, rep);
...
}
Since joycon_parse_imu_report() expects a full 49-byte payload and reads
36 bytes of IMU data starting at offset 13, could an attacker send a
truncated 12-byte report to force an out-of-bounds read, potentially leaking
uninitialized kernel heap data to userspace?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260730101506.547340-1-yijiangshan@kylinos.cn?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread