Re: [PATCH 2/2] Input: xpad - add wired Beitong BTP-KP20D support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Probe error path after input_register_device() causes a Use-After-Free risk because xpad driver data is unconditionally freed while the input device is visible to userspace.

Pre-existing issues:
- [Critical] The probe error path frees the output URB (irq_out) and its DMA buffer without killing the active URB, causing memory corruption.
--

--- Patch [2]: [PATCH 2/2] Input: xpad - add wired Beitong BTP-KP20D support ---
commit ec428f1a8b8e609c623920e0839f0af31cf70bb0
Author: Aaron Ma <mapengyu@gmail.com>

Input: xpad - add wired Beitong BTP-KP20D support

The wired BTP-KP20D resets itself when its interrupt-IN pipe is idle
for a few seconds, causing periodic re-enumeration whenever no
userspace client holds the input device open. Add a FLAG_KEEPALIVE
quirk that keeps the input URB running from probe to disconnect,
mirroring the XTYPE_XBOX360W URB lifecycle.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 1616047e65f5e..5369b880c6a08 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c

[ ... ]

> @@ -2227,9 +2232,22 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
>  		error = xpad_init_input(xpad);
>  		if (error)
>  			goto err_deinit_output;
> +
> +		if (xpad->keepalive) {
> +			/*
> +			 * The device resets itself when the interrupt-IN pipe
> +			 * is idle for a few seconds, so keep the URB running
> +			 * from probe until disconnect.
> +			 */
> +			error = xpad_start_input(xpad);
> +			if (error)
> +				goto err_deinit_input;

[Severity: High]
If xpad_start_input() fails here, we jump to err_deinit_input which 
unregisters the input device, and eventually falls through to kfree(xpad) 
in xpad_probe(). 

Because xpad_init_input() has already successfully called 
input_register_device(), the device is visible to userspace.

Could a local user open the evdev node immediately after registration and 
issue an EVIOCSFF ioctl concurrently? If so, input_ff_upload() would call 
xpad_play_effect(), which might dereference xpad after it has been freed 
since input_unregister_device() does not block concurrent ioctls that 
have already passed the evdev->exist check.

> +		}
>  	}
>  	return 0;
>  
> +err_deinit_input:
> +	xpad_deinit_input(xpad);
>  err_deinit_output:
>  	xpad_deinit_output(xpad);

[Severity: Critical]
This is a pre-existing issue, but this new fallible xpad_start_input() 
call creates a new way to trigger it.

If we take the error path to err_deinit_output after xpad_init_input() 
succeeds (which calls xpad_led_probe() and submits the irq_out URB for 
Xbox 360 controllers), xpad_deinit_output() will free the irq_out URB 
and its odata DMA buffer.

However, it does not appear to call xpad_stop_output() or usb_kill_urb(). 
Can this lead to memory corruption if the USB host controller continues 
writing to the freed odata DMA buffer, and the URB completion handler 
xpad_irq_out() executes and dereferences the freed xpad structure?

>  err_free_in_urb:
>  	usb_free_urb(xpad->irq_in);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725072813.339112-1-mapengyu@gmail.com?part=2