Re: [PATCH 3/3] Input: xpad - add sdata_check() to xpad360 branches

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

New issues:
- [Medium] Zero-length packets incorrectly pass validation and are processed as valid empty states, spuriously releasing all controller buttons.

Pre-existing issues:
- [High] Blocking USB APIs and GFP_KERNEL allocations are unsafely invoked from the atomic URB completion handler.
- [High] Error paths during initialization destroy the input device while leaving a dangling pointer for presence_work to dereference.
- [Medium] 16-bit multi-byte reads and direct array modifications completely bypass the new length validation, leading to stale data leakage.
--

--- Patch [3]: [PATCH 3/3] Input: xpad - add sdata_check() to xpad360 branches ---
commit 0a84609d4ff81bead2c2c2c26f83c5c8f9cc506a
Author: Griffin Kroah-Hartman <[email protected]>

Input: xpad - add sdata_check() to xpad360 branches

This commit adds the sdata_check() safety wrapper to the xpad360_process_packet()
and xpad360w_process_packet() functions. It bounds-checks array accesses to prevent
out-of-bounds reads on the controller data buffer.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 319a4c4a695fe..304229782e455 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -891,19 +891,24 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d
>   */
>  
>  static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
> -				   u16 cmd, unsigned char *data)
> +				   u16 cmd, unsigned char *data, u32 len)
>  {
> +	struct safe_data sdata;
> +
> +	sdata.data = data;
> +	sdata.len = len;
> +
>  	/* valid pad data */
> -	if (data[0] != 0x00)
> +	if (sdata_check(&sdata, 0) != 0x00)
>  		return;

[Severity: Medium]
Does this inadvertently allow zero-length packets to be treated as valid
pad data? Because sdata_check() safely returns 0 when the index is out of
bounds, a zero-length packet will successfully pass this != 0x00 check
and spuriously release all controller buttons.

>  
>  	/* digital pad */
>  	if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {

[ ... ]

> @@ -915,27 +920,29 @@ static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
>  	if (!(xpad->mapping & MAP_DPAD_TO_BUTTONS) ||
>  	    xpad->xtype == XTYPE_XBOX360W) {

[ ... ]

>  	/* buttons A,B,X,Y,TL,TR and MODE */
> -	input_report_key(dev, BTN_A,	data[3] & BIT(4));
> -	input_report_key(dev, BTN_B,	data[3] & BIT(5));
> -	input_report_key(dev, BTN_X,	data[3] & BIT(6));
> -	input_report_key(dev, BTN_Y,	data[3] & BIT(7));
> -	input_report_key(dev, BTN_TL,	data[3] & BIT(0));
> -	input_report_key(dev, BTN_TR,	data[3] & BIT(1));
> -	input_report_key(dev, BTN_MODE,	data[3] & BIT(2));
> +	input_report_key(dev, BTN_A,	sdata_check(&sdata, 3) & BIT(4));
> +	input_report_key(dev, BTN_B,	sdata_check(&sdata, 3) & BIT(5));
> +	input_report_key(dev, BTN_X,	sdata_check(&sdata, 3) & BIT(6));
> +	input_report_key(dev, BTN_Y,	sdata_check(&sdata, 3) & BIT(7));
> +	input_report_key(dev, BTN_TL,	sdata_check(&sdata, 3) & BIT(0));
> +	input_report_key(dev, BTN_TR,	sdata_check(&sdata, 3) & BIT(1));
> +	input_report_key(dev, BTN_MODE,	sdata_check(&sdata, 3) & BIT(2));
>  
>  	if (!(xpad->mapping & MAP_STICKS_TO_NULL)) {
>  		/* left stick */

[Severity: Medium]
This isn't a bug introduced by this patch, but will the 16-bit stick axis 
reads immediately following this code bypass the new length checks?

xpad_process_packet(), xpad360_process_packet(), and xpadone_process_packet()
read multi-byte values using le16_to_cpup() directly against the raw data
buffer:

    input_report_abs(dev, ABS_X,
                     (__s16) le16_to_cpup((__le16 *)(data + 6)));

A short packet would still cause out-of-bounds reads here since these
accesses completely bypass the sdata_check() wrapper, potentially leaking
uninitialized DMA memory into input events.

[ ... ]

> @@ -1019,14 +1026,18 @@ 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;
> +	struct safe_data sdata;
>  	bool present;
>  
> +	sdata.data = data;
> +	sdata.len = len;
> +
>  	/* Presence change */
> -	if (data[0] & 0x08) {
> -		present = (data[1] & 0x80) != 0;
> +	if (sdata_check(&sdata, 0) & 0x08) {
> +		present = (sdata_check(&sdata, 1) & 0x80) != 0;
>  
>  		if (xpad->pad_present != present) {
>  			xpad->pad_present = present;
>  			schedule_work(&xpad->work);

[Severity: High]
This is a pre-existing issue, but can the xpad_presence_work thread
dereference a freed pointer if initialization fails?

If xpad_init_input() encounters an error (like during device registration),
it takes the err_free_input path:

xpad_init_input() {
    ...
err_free_input:
    input_free_device(input_dev);
    return error;
}

This frees the device but leaves xpad->dev pointing to the freed memory.
When xpad_presence_work() processes the failure, it logs the error:

xpad_presence_work() {
    ...
    dev_err(&xpad->dev->dev, "unable to init device: %d\n", error);
}

Could this use-after-free lead to memory corruption or a crash?

[ ... ]

> @@ -1262,10 +1273,10 @@ static void xpad_irq_in(struct urb *urb)
>  
>  	switch (xpad->xtype) {
>  	case XTYPE_XBOX360:
> -		xpad360_process_packet(xpad, xpad->dev, 0, xpad->idata);
> +		xpad360_process_packet(xpad, xpad->dev, 0, xpad->idata, urb->actual_length);
>  		break;
>  	case XTYPE_XBOX360W:
> -		xpad360w_process_packet(xpad, 0, xpad->idata);
> +		xpad360w_process_packet(xpad, 0, xpad->idata, urb->actual_length);
>  		break;
>  	case XTYPE_XBOXONE:
>  		xpadone_process_packet(xpad, 0, xpad->idata, urb->actual_length);

[Severity: High]
This is a pre-existing issue, but does xpadone_process_packet() execute
blocking calls and GFP_KERNEL allocations from an atomic context?

When xpad_irq_in() processes an Xbox One GIP_CMD_ANNOUNCE packet from
interrupt context, it follows this call chain:

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

xpad_start_input() also calls xpad_start_xbox_one(), which invokes
usb_set_interface(), a synchronous and blocking USB API. Calling these
from the atomic URB completion handler could lead to scheduling while
atomic warnings and potential deadlocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3