Re: [PATCH V8] xpad: Overhaul device data for wireless devices

[sashiko-bot@kernel.org]

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

New issues:
- [High] Forcing `xpad->pad_present = false` during suspend creates a permanent state mismatch, leading to an input device leak on disconnect and reusing wrong device properties on reconnect.
- [Medium] Stale wireless properties (`wireless_vid`, `pid`, etc.) leak to subsequent controllers if a controller disconnects before the 500ms initialization delay completes.
- [Medium] Wireless properties are written in interrupt context and read in process context without synchronization, causing a data race.

Pre-existing issues:
- [High] Missing `cancel_delayed_work_sync` in the `xpad_probe` error path causes a Use-After-Free if initialization fails.
--

--- Patch [1]: [PATCH V8] xpad: Overhaul device data for wireless devices ---
commit 3361aff664031bdf598a6c0d59485911918ea3b4
Author: Sanjay Govind <sanjay.govind9@gmail.com>

xpad: Overhaul device data for wireless devices

Xbox 360 wireless controllers expose information in the link and
capabilities reports. Extract and use the vendor id for wireless controllers,
and use the subtype to build a nicer device name and product id.

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

[ ... ]

> @@ -958,19 +982,12 @@ static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
>  
>  static void xpad_presence_work(struct work_struct *work)
>  {
> -	struct usb_xpad *xpad = container_of(work, struct usb_xpad, work);
> +	struct usb_xpad *xpad = container_of(work, struct usb_xpad, work.work);
>  	int error;
> -
> -	if (xpad->pad_present) {
> -		error = xpad_init_input(xpad);
> -		if (error) {
> -			/* complain only, not much else we can do here */
> -			dev_err(&xpad->dev->dev,
> -				"unable to init device: %d\n", error);
> -		} else {
> -			rcu_assign_pointer(xpad->x360w_dev, xpad->dev);
> -		}
> -	} else {
> +	/* Check if the pad presence has changed */
> +	if (xpad->pad_present == xpad->input_created)
> +		return;

[Severity: Medium]
If a controller connects and schedules xpad->work with a 500ms delay, and
then disconnects before the timer expires, xpad->pad_present becomes false.
When xpad_presence_work() runs, it will see pad_present (false) == 
input_created (false) and return early.

Does this early return prevent xpad_deinit_input() from clearing the stale
wireless properties, allowing a subsequent controller to erroneously inherit
them if it connects without a Link report?

[ ... ]

> @@ -995,10 +1021,11 @@ static void xpad_presence_work(struct work_struct *work)
>   * 01.1 - Pad state (Bytes 4+) valid
>   *
>   */
> -static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> +static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data, u32 len)
>  {
>  	struct input_dev *dev;
>  	bool present;
> +	u16 parsed_vid;

[ ... ]

> +	/* Link report */
> +	if (len >= 26 && data[0] == 0x00 && data[1] == 0x0F) {
> +		xpad->sub_type = data[25] & 0x7f;
> +
> +		/* Decode vendor id from link report */
> +		parsed_vid = ((data[0x16] & 0xf) | data[0x18] << 4) << 8 | data[0x17];
> +
> +		/*
> +		 * If the link report doesn't provide a proper vid, it sets the vid to 1.
> +		 * In that case we zero out wireless_vid, so that we fall back to the vid
> +		 * from the receiver instead.
> +		 */
> +		if (parsed_vid == 1)
> +			parsed_vid = 0;
> +
> +		/*
> +		 * x360w controllers on windows put the subtype into the product
> +		 * for wheels and gamepads, but it makes sense to do it for all
> +		 * subtypes. This will be used if the capabilities report
> +		 * doesn't provide us with a product id later.
> +		 */
> +		xpad->wireless_vid = parsed_vid;
> +		xpad->wireless_pid = 0x02a0 + xpad->sub_type;
> +		xpad->wireless_version = 0;
> +
> +		if ((data[25] & 0x80) != 0)
> +			xpad->flags |= FLAG_FORCE_FEEDBACK;

[Severity: Medium]
This function executes in interrupt context, modifying properties like
wireless_vid, sub_type, and flags.

Concurrently, xpad_init_input() runs in process context (workqueue) after
the 500ms delay and reads these exact fields. Since the irq_in URB remains
active, a late Link or Capabilities report can arrive while the workqueue
executes.

Could this data race lead to torn reads or an inconsistent property state
being applied to the device?

[ ... ]

> @@ -2084,7 +2251,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
>  		xpad->delay_init = true;
>  
>  	xpad->packet_type = PKT_XB;
> -	INIT_WORK(&xpad->work, xpad_presence_work);
> +	INIT_DELAYED_WORK(&xpad->work, xpad_presence_work);

[Severity: High]
This is a pre-existing issue, but by changing this to a delayed work with a
500ms window, it becomes much easier to trigger.

If a connection packet arrives immediately after the irq_in URB is submitted,
the interrupt handler will schedule xpad->work with a 500ms delay.

If a subsequent initialization step in xpad_probe() fails, the error path
frees the xpad struct:

err_free_idata:
        usb_free_coherent(udev, XPAD_PKT_LEN, xpad->idata, xpad->idata_dma);
err_free_mem:
        kfree(xpad);
        return error;

Since the error path does not call cancel_delayed_work_sync(), will the
pending work execute 500ms later and trigger a use-after-free on the freed
xpad struct?

[ ... ]

> @@ -2265,6 +2432,7 @@ static int xpad_suspend(struct usb_interface *intf, pm_message_t message)
>  		 */
>  		if (auto_poweroff && xpad->pad_present)
>  			xpad360w_poweroff_controller(xpad);
> +		xpad->pad_present = false;
>  	} else {
>  		guard(mutex)(&input->mutex);

[Severity: High]
If the system suspends, xpad->pad_present is forced to false here, but
xpad->input_created remains true.

If the controller disconnects during suspend, the resume sequence receives a
status packet with present = false. Then xpad360w_process_packet() sees
xpad->pad_present != present (false != false) and drops the event.

Will this prevent xpad_presence_work() from ever being scheduled to call
xpad_deinit_input(), leaking the input device?

Furthermore, if a new controller connects, pad_present becomes true. Would
xpad_presence_work() see pad_present (true) == input_created (true) and
return early, failing to apply the new controller's properties?

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