Re: [PATCH v12] Input: xpad - overhaul device data for wireless devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] Input: xpad - missing xpad_stop_output() on xpad_probe()
  error path
- [Medium] Input: xpad - uncancelled discovery work on xpad_resume()
  error path
- [Medium] Input: xpad - use-after-free race on xpad_led in
  xpad_resume()
--

commit 8fc5697265d2347744860ac1eec114fe9c075964
Author: Sanjay Govind <sanjay.govind9@gmail.com>

Input: xpad - overhaul device data for wireless devices

This commit updates wireless Xbox 360 controller handling to populate input
device identification from link and capabilities reports, deferring input
device registration until discovery completes or times out and protecting
discovery state across connect, disconnect, and suspend/resume transitions.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 810e52782ae89..a273d820910e0 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c

[ ... ]

> @@ -2204,8 +2496,18 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
>  		 * we're waiting for.
>  		 */
>  		error = xpad360w_start_input(xpad);
> -		if (error)
> +		if (error) {
> +			/*
> +			 * Reports received before the inquiry failed may have
> +			 * queued discovery work, or even registered an input
> +			 * device. The irq is already stopped, so nothing can
> +			 * queue more work or use x360w_dev.
> +			 */
> +			cancel_delayed_work_sync(&xpad->work);
> +			RCU_INIT_POINTER(xpad->x360w_dev, NULL);
> +			xpad_deinit_input(xpad);
>  			goto err_deinit_output;
> +		}

[Severity: Medium]
Can jumping to err_deinit_output without calling xpad_stop_output(xpad)
cause a use-after-free on xpad->irq_out, xpad->odata, and xpad?

Before xpad360w_start_input() kills xpad->irq_in on failure, an early link
report can submit xpad->irq_out via xpad_inquiry_pad_capabilities(), and
xpad_deinit_input() -> xpad_led_disconnect() -> led_classdev_unregister()
also submits xpad->irq_out to turn off the LED if an input device was
registered.  Because err_deinit_output calls xpad_deinit_output() and
kfree(xpad) without waiting for anchored output URBs to complete via
xpad_stop_output() (unlike xpad_disconnect()), xpad_irq_out() can
dereference the freed structures when the URB completes.

[ ... ]

> @@ -2262,6 +2565,7 @@ static int xpad_suspend(struct usb_interface *intf, pm_message_t message)
>  {
>  	struct usb_xpad *xpad = usb_get_intfdata(intf);
>  	struct input_dev *input = xpad->dev;
> +	struct xpad360w_pad_info info;
>  
>  	if (xpad->xtype == XTYPE_XBOX360W) {
>  		/*
> @@ -2271,14 +2575,37 @@ static int xpad_suspend(struct usb_interface *intf, pm_message_t message)
>  		 */
>  		xpad360w_stop_input(xpad);
>  
> +		scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
> +			info = xpad->pad_info;
> +		}
> +
>  		/*
>  		 * The wireless adapter is going off now, so the
>  		 * gamepads are going to become disconnected.
>  		 * Unless explicitly disabled, power them down
>  		 * so they don't just sit there flashing.
>  		 */
> -		if (auto_poweroff && xpad->pad_present)
> +		if (auto_poweroff && info.present)
>  			xpad360w_poweroff_controller(xpad);
> +
> +		/* Finish teardown if suspend cancelled work for an old pad. */
> +		if (xpad->input_created &&
> +		    (!info.present || xpad->input_gen != info.gen)) {
> +			RCU_INIT_POINTER(xpad->x360w_dev, NULL);
> +			synchronize_rcu();
> +			xpad_deinit_input(xpad);
> +		}
> +
> +		/*
> +		 * The controller may have changed while input was stopped.
> +		 * Force resume to rediscover it instead of reusing stale data.
> +		 */
> +		scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
> +			RCU_INIT_POINTER(xpad->x360w_dev, NULL);
> +			xpad->pad_info = (struct xpad360w_pad_info) {
> +				.gen = xpad->pad_info.gen,
> +			};
> +		}
>  	} else {
>  		guard(mutex)(&input->mutex);
>  
> @@ -2297,8 +2624,15 @@ static int xpad_resume(struct usb_interface *intf)
>  	struct input_dev *input = xpad->dev;
>  
>  	xpad->delayed_init_done = false;
> -	if (xpad->xtype == XTYPE_XBOX360W)
> +	if (xpad->xtype == XTYPE_XBOX360W) {
> +		/*
> +		 * Tear down the old input device even if the resume inquiry
> +		 * reports that no controller is currently present.
> +		 */
> +		mod_delayed_work(system_percpu_wq, &xpad->work, 0);

[Severity: Medium]
Can deferring xpad_deinit_input() to xpad->work during resume race with
dpm_resume() and cause a use-after-free on xpad->led?

When a wireless controller was present at suspend, xpad_suspend() leaves
xpad->input_created set (and xpad->led->led_cdev.dev on dpm_suspended_list)
while clearing xpad->pad_info.present.  During system resume, xpad_resume()
queues xpad->work to run xpad_deinit_input() asynchronously while
dpm_resume() moves on to resume the child LED device:

CPU0 (dpm_resume)                 CPU1 (system_percpu_wq)
device_resume(&udev->dev)
  ...
    xpad_resume()
      mod_delayed_work(..., &xpad->work, 0)
device_resume(led_cdev->dev)
  dpm_wait_for_superior()
                                  xpad_presence_work()
                                    xpad_deinit_input(xpad)
                                      xpad_led_disconnect(xpad)
                                        led_classdev_unregister()
                                        kfree(xpad_led)
  led_resume()
    led_classdev_resume(led_cdev)  <-- use-after-free

Since xpad_suspend() unconditionally clears pad_info to force rediscovery,
calling xpad_deinit_input() synchronously in xpad_suspend() whenever
xpad->input_created is true would avoid tearing down child devices
asynchronously during dpm_resume().

> +
>  		return xpad360w_start_input(xpad);
> +	}

[Severity: Medium]
If xpad360w_start_input(xpad) fails here after xpad->irq_in was briefly
submitted, can xpad->work remain scheduled and register an input device
while input is stopped?

Inside xpad360w_start_input(), usb_submit_urb(xpad->irq_in, GFP_KERNEL)
runs before xpad_inquiry_pad_presence().  If a presence or link report
arrives before xpad_inquiry_pad_presence() fails and kills xpad->irq_in,
xpad360w_process_packet() sets pad_info.present and arms xpad->work for
discovery.  Unlike the error path in xpad_probe(), xpad_resume() returns
the error without cancelling xpad->work, clearing pad_info, or calling
xpad_deinit_input().

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