Re: [PATCH v2 3/3] Input: xpad - add size checks to xpad360 packet processing

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

New issues:
- [Medium] Logical out-of-bounds read due to incorrect order of length check.

Pre-existing issues:
- [High] Use-After-Free or NULL Pointer Dereference in error logging path.
--

--- Patch [3]: [PATCH v2 3/3] Input: xpad - add size checks to xpad360 packet processing ---
commit 2a90e36a133f256901ec08114f8164654060dd73
Author: Griffin Kroah-Hartman <[email protected]>

Input: xpad - add size checks to xpad360 packet processing

This commit adds size checks to xbox360_process_packet() and
xbox360w_process_packet() to prevent out-of-bounds reads with the data array.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 83bc361b15f76..6f15eafc64775 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -870,10 +870,10 @@ 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)
>  {
>  	/* valid pad data */
> -	if (data[0] != 0x00)
> +	if (data[0] != 0x00 || len < 14)

[Severity: Medium]
Will this evaluate data[0] before checking len? C logical OR short-circuits
from left to right, meaning data[0] could be read from the DMA buffer
before we verify len is large enough to contain it.

If we receive a zero-length packet (ZLP) where urb->actual_length is 0,
could this result in an out-of-bounds read of the data array?

[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but I noticed a potential use-after-free or NULL pointer dereference in the
error logging path when xpad_init_input() fails.

If input_allocate_device() fails, xpad->dev remains NULL:

drivers/input/joystick/xpad.c:xpad_init_input() {
    ...
	input_dev = input_allocate_device();
	if (!input_dev)
		return -ENOMEM;

	xpad->dev = input_dev;
    ...
}

Or if a later initialization step fails, xpad_init_input() frees input_dev
but leaves xpad->dev pointing to the freed memory:

drivers/input/joystick/xpad.c:xpad_init_input() {
    ...
 err_free_input:
	input_free_device(input_dev);
	return error;
}

If xpad_init_input() returns an error, xpad_presence_work() logs it:

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

Could this dev_err() call dereference the NULL or dangling xpad->dev pointer?

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