Re: [PATCH] HID: nintendo: register input device after capabilities are set

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 <[email protected]>

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/[email protected]?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/[email protected]?part=1