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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Canceling the delayed presence work during suspend leaves the device state machine permanently stuck upon resume.
- [Low] Stale wireless controller properties leak across rapid connect/disconnect cycles.

Pre-existing issues:
- [High] Missing cancellation of delayed work in the xpad_probe() error path causes a Use-After-Free.
--

--- Patch [1]: [PATCH V7 RESEND] xpad: Overhaul device data for wireless devices ---
commit fc7641a4c39d3980083db9bce9e3e7093ef644d0
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.

> ---
> drivers/input/joystick/xpad.c | 209 ++++++++++++++++++++++++++++++----
>  1 file changed, 188 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834e..f5b61406adbc2 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: Low]
If a controller disconnects before the 500ms delayed work executes, does this
early return bypass xpad_deinit_input()?

Because wireless controller properties (like flags and sub_type) are
accumulated using bitwise OR (xpad->flags |= data[20]) and not cleared upon a
new connection, could rapid reconnects of different device types into the same
wireless receiver slot cause state to leak? For example, could a Rock Band
drum kit be misidentified as a Guitar Hero drum kit if plugged in immediately
after one?

> +	if (xpad->input_created) {
>  		RCU_INIT_POINTER(xpad->x360w_dev, NULL);
>  		synchronize_rcu();
[ ... ]
> @@ -1870,8 +1980,8 @@ static void xpad360w_stop_input(struct usb_xpad *xpad)
>  {
>  	usb_kill_urb(xpad->irq_in);
>  
> -	/* Make sure we are done with presence work if it was scheduled */
> -	flush_work(&xpad->work);
> +	/* Cancel any pending presence work */
> +	cancel_delayed_work_sync(&xpad->work);

[Severity: Medium]
If the system suspends while the 500ms delayed presence work is pending, this
cancels the work, leaving the state machine with pad_present = true but
input_created = false.

Upon resume, when xpad360w_start_input() sends a presence inquiry, and the
controller replies that it is present, xpad360w_process_packet() will compare
the new state with pad_present. Since both are true, it ignores the packet:

xpad360w_process_packet()
    if (data[0] & 0x08) {
        present = (data[1] & 0x80) != 0;
        if (xpad->pad_present != present) {
            ...

Does this mean input_created will permanently remain false, and the controller
will be unresponsive to all inputs until physically replugged?

>  }
>  
>  static int xpad_open(struct input_dev *dev)
[ ... ]
> @@ -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 isn't a bug introduced by this patch, but missing cancellation of the work
in the xpad_probe() error path can cause a Use-After-Free, and the addition of a
500ms delay in this patch makes it highly reliable to hit.

If xpad360w_start_input() succeeds in submitting the URB but fails later during
xpad_inquiry_pad_presence(), it kills the URB and returns an error:

xpad_probe()
    error = xpad360w_start_input(xpad);
    if (error)
        goto err_deinit_output;
    ...
err_free_mem:
    kfree(xpad);
    return error;

If the hardware replied immediately to the URB submission, the URB completion
handler may have already queued the delayed presence work. xpad_probe() then
jumps to the error path and frees the xpad memory without cancelling the work.
Could this result in the delayed work executing and dereferencing the freed
xpad memory?

>  
>  	if (xpad->xtype == XTYPE_UNKNOWN) {
>  		if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {

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