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

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

Pre-existing issues:
- [High] Missing workqueue cancellation in xpad_probe error path leads to Use-After-Free.
- [High] Sleeping in atomic context when processing GIP_CMD_ANNOUNCE from the URB completion handler.
--

--- Patch [2]: [PATCH v2 2/2] Input: xpad - add wired Beitong BTP-KP20D support ---
commit 600bda7a2ef7ea6aabd37796bfb7164b015f813b
Author: Aaron Ma <[email protected]>

Input: xpad - add wired Beitong BTP-KP20D support

This patch adds support for the wired Beitong BTP-KP20D controller and
introduces a FLAG_KEEPALIVE quirk. The quirk keeps the input URB running
from probe to disconnect to prevent the device from resetting itself when idle.

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

[ ... ]

> @@ -2227,10 +2232,24 @@ 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);

[Severity: High]
This is a pre-existing issue, but does xpad_start_input() sleep in atomic
context when called from the URB completion handler?

For XTYPE_XBOXONE devices with FLAG_DELAY_INIT, a GIP_CMD_ANNOUNCE packet
triggers xpadone_process_packet() from the URB completion handler xpad_irq_in(),
which executes in an atomic context (softirq).

xpadone_process_packet() then invokes xpad_start_input():

	if (xpad->delay_init && !xpad->delayed_init_done) {
		xpad->delayed_init_done = true;
		error = xpad_start_input(xpad);

xpad_start_input() calls usb_submit_urb() with GFP_KERNEL, which can sleep:

	if (usb_submit_urb(xpad->irq_in, GFP_KERNEL))
		return -EIO;

It also calls xpad_start_xbox_one(), which invokes usb_set_interface() to
disable the audio interface using synchronous USB control messages that can wait
and sleep.

Can this trigger a kernel bug or panic if a malicious USB device sends a
GIP_CMD_ANNOUNCE packet?

> +			if (error)
> +				goto err_deinit_input;
> +		}
>  	}
>  	return 0;
>  
> +err_deinit_input:
> +	xpad_deinit_input(xpad);
>  err_deinit_output:
> +	xpad_stop_output(xpad);
>  	xpad_deinit_output(xpad);
>  err_free_in_urb:

[Severity: High]
This is a pre-existing issue, but is it possible for xpad_probe() to free the
xpad structure without canceling the workqueue on the error path?

For XTYPE_XBOX360W devices, xpad_probe() calls xpad360w_start_input(), which
submits the irq_in URB and then calls xpad_inquiry_pad_presence():

	error = xpad_inquiry_pad_presence(xpad);
	if (error) {
		usb_kill_urb(xpad->irq_in);
		return error;
	}

If a malicious or buggy device responds immediately via the IN endpoint,
xpad_irq_in() will run and xpad360w_process_packet() may schedule xpad->work.

If xpad_inquiry_pad_presence() fails, xpad360w_start_input() kills the URB and
returns an error without canceling the scheduled work.

xpad_probe() then takes the err_deinit_output path:

	if (xpad->xtype == XTYPE_XBOX360W) {
		...
		error = xpad360w_start_input(xpad);
		if (error)
			goto err_deinit_output;

This eventually frees the xpad structure via kfree(xpad).

Can the workqueue execute xpad_presence_work() later, causing a Use-After-Free
when accessing the freed xpad structure?

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