Re: [PATCH v2] HID: nintendo: stop device IO before hid_hw_stop on probe failure

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver registers the input device before fully initializing its capabilities and force-feedback callbacks, violating subsystem rules and introducing a race condition that can lead to a NULL pointer dereference and broken userspace device recognition.
--

--- Patch [1]: [PATCH v2] HID: nintendo: stop device IO before hid_hw_stop on probe failure ---
commit 175b2ac9211bb7f8a122c8a3e3d05eeac1ea8dac
Author: Jiangshan Yi <[email protected]>

HID: nintendo: stop device IO before hid_hw_stop on probe failure

nintendo_hid_probe() calls hid_device_io_start() before joycon_init()
and joycon_leds_create().  If either fails, the error path jumps to
err_close which calls hid_hw_close()/hid_hw_stop() without first calling
hid_device_io_stop().

hid_hw_stop() does not stop device IO, so hid_input_report() may still
run and access driver data that is being torn down, resulting in a
use-after-free.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index e7302ec01ff1f..0db5535288263 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
[ ... ]
> @@ -2766,7 +2766,8 @@ static int nintendo_hid_probe(struct hid_device *hdev,
>  
>  err_ida:
>  	ida_free(&nintendo_player_id_allocator, ctlr->player_id);
> -err_close:
> +err_io_stop:
> +	hid_device_io_stop(hdev);
>  	hid_hw_close(hdev);
>  err_stop:
>  	hid_hw_stop(hdev);

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition in how
the input device is registered in joycon_input_create(), which is called 
slightly further down in nintendo_hid_probe().

In joycon_input_create(), input_register_device() is called before the device 
capabilities and force-feedback callbacks are fully configured:

    ret = input_register_device(ctlr->input);
    if (ret)
        return ret;

    if (joycon_type_is_right_joycon(ctlr)) {
        joycon_config_right_stick(ctlr->input);
...
    if (joycon_has_rumble(ctlr))
        joycon_config_rumble(ctlr);

And in joycon_config_rumble():

    input_set_capability(ctlr->input, EV_FF, FF_RUMBLE);
    input_ff_create_memless(ctlr->input, NULL, joycon_play_effect);

Because input_register_device() makes the device visible to userspace 
immediately, couldn't a userspace process open the device and trigger the 
EVIOCSFF ioctl concurrently?

If evdev calls input_ff_upload() while joycon_config_rumble() is running, it
might observe the EV_FF bit but dereference an uninitialized dev->ff,
potentially causing a NULL pointer dereference. Additionally, registering
before setting capabilities means the initial udev event might lack button
and axis information, which could cause input managers like Wayland to
ignore the device.

Should the call to input_register_device() be moved to the end of 
joycon_input_create() after all capabilities and force-feedback are 
initialized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1