Re: [PATCH] Input: xpad: validate Xbox One report lengths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Handling GIP_CMD_ANNOUNCE in xpadone_process_packet calls xpad_start_input(), leading to sleeping functions being called in atomic context and a potential deadlock.
--

--- Patch [1]: [PATCH] Input: xpad: validate Xbox One report lengths ---
commit b48d8bd27dd206f904c7c21ce961f7a9225d4190
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Subject: Input: xpad: validate Xbox One report lengths

This commit adds bounds checking to xpadone_process_packet() to prevent
out-of-bounds reads on short USB reports. It validates that mandatory and
optional fields fall within urb->actual_length before processing them.

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

[ ... ]

> @@ -1079,6 +1093,9 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
>  					 error);
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does the GIP_CMD_ANNOUNCE
handling in the code right above this snippet sleep in atomic context?

xpadone_process_packet() executes in the atomic context of the xpad_irq_in
URB completion handler. When handling a GIP_CMD_ANNOUNCE packet, it calls
xpad_start_input().

This leads to a sleep-in-atomic violation due to a sleeping allocation:

xpadone_process_packet()
  -> xpad_start_input()
       -> usb_submit_urb(xpad->irq_in, GFP_KERNEL)

It also takes a mutex during USB control messages, which can sleep:

xpadone_process_packet()
  -> xpad_start_input()
       -> xpad_start_xbox_one()
            -> usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0)

Finally, if xpad_start_input() fails, does the error path deadlock? 
It appears to wait indefinitely for the completion handler it is 
currently executing inside of:

xpadone_process_packet()
  -> xpad_start_input()
       -> usb_kill_urb(xpad->irq_in)

>  	} else if (data[0] == GIP_CMD_INPUT) { /* The main valid packet type for inputs */
> +		if (len < 18)
> +			return;
> +
>  		/* menu/view buttons */
>  		input_report_key(dev, BTN_START,  data[4] & BIT(2));
>  		input_report_key(dev, BTN_SELECT, data[4] & BIT(3));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715083424.25944-1-pengpeng@iscas.ac.cn?part=1