Re: [PATCH v4 02/12] Input: xpad - Remove Xbox One support

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

New issues:
- [Low] Incomplete feature removal leaves behind dead code related to Xbox One support.

Pre-existing issues:
- [High] Use-after-free due to missing cancel_work_sync() in xpad_probe error path.
--

--- Patch [2]: [PATCH v4 02/12] Input: xpad - Remove Xbox One support ---
commit 5a5aa152af89fbca852a5ae95c5acba0ea842ea5
Author: Vicki Pfau <[email protected]>

Input: xpad - Remove Xbox One support

This commit removes Xbox One controller support from the xpad driver, as
it has been superseded by the xbox_gip driver.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c

[ ... ]

> @@ -754,7 +476,6 @@ struct usb_xpad {
>  	struct urb *irq_out;		/* urb for interrupt out report */
>  	struct usb_anchor irq_out_anchor;
>  	bool irq_out_active;		/* we must not use an active URB */
> -	u8 odata_serial;		/* serial number for xbox one protocol */
>  	unsigned char *odata;		/* output data */
>  	dma_addr_t odata_dma;
>  	spinlock_t odata_lock;

[Severity: Low]
Since Xbox One support is being removed, is it intentional that fields like
init_seq, packet_type, delay_init, and delayed_init_done were left in struct
usb_xpad? They appear to be completely unused now and could cause confusion.

For example, xpad_probe() still contains a large block of logic detecting
Elite firmware revisions purely to set xpad->packet_type:

xpad_probe() {
...
        /* Packet type detection */
        if (le16_to_cpu(udev->descriptor.idVendor) == 0x045e) {
                if (le16_to_cpu(udev->descriptor.idProduct) == 0x02e3) {
                        /* The original elite controller always uses the oldest
...
}

Could this dead code be cleaned up as part of the removal?

[ ... ]

> @@ -2090,8 +1484,6 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
>  		if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
>  			if (intf->cur_altsetting->desc.bInterfaceProtocol == 129)
>  				xpad->xtype = XTYPE_XBOX360W;
> -			else if (intf->cur_altsetting->desc.bInterfaceProtocol == 208)
> -				xpad->xtype = XTYPE_XBOXONE;
>  			else
>  				xpad->xtype = XTYPE_XBOX360;
>  		} else {

[Severity: High]
This is a pre-existing issue, but does the error path in xpad_probe() contain
a use-after-free risk if xpad360w_start_input() fails?

If xpad360w_start_input() fails after submitting the input URB (for instance,
if xpad_inquiry_pad_presence() fails), it returns an error. The error path in
xpad_probe() then frees the xpad struct:

err_deinit_output:
    xpad_deinit_output(xpad);
err_free_in_urb:
    usb_free_urb(xpad->irq_in);
err_free_idata:
    usb_free_coherent(udev, XPAD_PKT_LEN, xpad->idata, xpad->idata_dma);
err_free_mem:
    kfree(xpad);
    return error;

However, if the URB completes before being fully killed and
xpad360w_process_packet() handles a presence change, it calls
schedule_work(&xpad->work).

Because the error path never calls cancel_work_sync(&xpad->work), the
workqueue might dereference the freed xpad pointer when xpad_presence_work()
executes. Could a malicious USB device trigger this race by intentionally
failing the presence inquiry while sending a valid URB completion?

[ ... ]

> @@ -2291,15 +1672,6 @@ static int xpad_resume(struct usb_interface *intf)
>  	if (input_device_enabled(input))
>  		return xpad_start_input(xpad);
>  
> -	if (xpad->xtype == XTYPE_XBOXONE) {
> -		/*
> -		 * Even if there are no users, we'll send Xbox One pads
> -		 * the startup sequence so they don't sit there and
> -		 * blink until somebody opens the input device again.
> -		 */
> -		return xpad_start_xbox_one(xpad);
> -	}
> -
>  	return 0;
>  }

[Severity: Low]
The delayed_init_done flag is still being cleared at the start of
xpad_resume() despite being unused elsewhere. Should this assignment be
removed as well?

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