Re: [PATCH] Input: xpad - add support for GameSir G7 Pro dongle in Xbox mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] Input: xpad - use-after-free in xpad_probe() [1]
--

commit 9da1d473121c33d6a51bd41a4411539bd2d0fc9a
Author: Alexander Gollings <levirootsmanuva2@gmail.com>

Input: xpad - add support for GameSir G7 Pro dongle in Xbox mode

This commit adds an Xbox One device entry for the GameSir G7 Pro 2.4GHz
dongle and skips Xbox 360 entries in xpad_probe() when the USB interface
uses the GIP protocol.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index d2eb48d98fead..f0ceb78e40a36 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c

[ ... ]

> @@ -2065,8 +2090,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
>  		return -ENODEV;
>  
>  	for (i = 0; xpad_device[i].idVendor; i++) {
> -		if ((le16_to_cpu(udev->descriptor.idVendor) == xpad_device[i].idVendor) &&
> -		    (le16_to_cpu(udev->descriptor.idProduct) == xpad_device[i].idProduct))
> +		if (xpad_device_match(&xpad_device[i], udev, intf))
>  			break;
>  	}

[Severity: Critical]
This problem wasn't introduced by this patch, but can xpad_probe() cause a
use-after-free on xpad [1] if xpad360w_start_input() fails after
submitting xpad->irq_in?

When probing an XTYPE_XBOX360W device, xpad360w_start_input() submits
xpad->irq_in before calling xpad_inquiry_pad_presence():

drivers/input/joystick/xpad.c:xpad360w_start_input() {
	error = usb_submit_urb(xpad->irq_in, GFP_KERNEL);
	if (error)
		return -EIO;

	/*
	 * Send presence packet.
	 * This will force the controller to resend connection packets.
	 * This is useful in case we have connected a controller before
	 * the module was loaded.
	 */
	error = xpad_inquiry_pad_presence(xpad);
	if (error) {
		usb_kill_urb(xpad->irq_in);
		return error;
	}
}

If a packet arrives before xpad_inquiry_pad_presence() fails, xpad_irq_in()
schedules xpad->work, and the error path in xpad_probe() frees xpad without
canceling the work:

CPU0                              CPU1
xpad_probe()
  INIT_WORK(&xpad->work, ...)
  xpad360w_start_input()
    usb_submit_urb(xpad->irq_in)
                                  xpad_irq_in()
                                    xpad360w_process_packet()
                                      schedule_work(&xpad->work)
    xpad_inquiry_pad_presence()
      /* fails */
    usb_kill_urb(xpad->irq_in)
  goto err_deinit_output;
  ...
  kfree(xpad)
                                  xpad_presence_work()
                                    /* dereferences freed xpad */
                                    <-- use-after-free

Calling cancel_work_sync(&xpad->work) after usb_kill_urb(xpad->irq_in) in
xpad360w_start_input() (matching xpad360w_stop_input()) would prevent the
work item from running after xpad is freed.

[1] https://sashiko.dev/#/bug/linux-0c0def42-a340-45bb-860d-c99ba757f43b

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