Re: [PATCH 2/3] Input: xpad - add sdata_check() to xpad controllers

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a use-after-free of `input_dev` in the error path of `xpad_presence_work()` when `xpad_init_input()` fails.
--

--- Patch [2]: [PATCH 2/3] Input: xpad - add sdata_check() to xpad controllers ---
commit 559bf574edac1c4551addcfb6bbff7b5065fbbc5
Author: Griffin Kroah-Hartman <[email protected]>

Input: xpad - add sdata_check() to xpad controllers

This commit adds the sdata_check() safety wrapper to the
xpad_process_packet() branch. It passes the actual length of the urb
to ensure packet bounds are checked properly. This prevents out-of-bounds
reads during interrupt handling for the original Xbox Controller.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index c516860711a82..319a4c4a695fe 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -813,9 +813,13 @@ static void xpad360w_poweroff_controller(struct usb_xpad *xpad);
>   *	The used report descriptor was taken from ITO Takayuki's website:
>   *	 http://euc.jp/periphs/xbox-controller.ja.html
>   */
> -static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> +static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data, u32 len)
>  {
>  	struct input_dev *dev = xpad->dev;
> +	struct safe_data sdata;
> +
> +	sdata.data = data;
> +	sdata.len = len;

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but there is a use-after-free
of input_dev in the error path of xpad_presence_work() when xpad_init_input()
fails.

In xpad_init_input(), xpad->dev is assigned to the newly allocated input_dev.
If initialization fails later, the function jumps to err_free_input and frees
input_dev, but leaves xpad->dev pointing to the freed memory:

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

When connecting a wireless Xbox 360 controller, xpad_presence_work() calls
xpad_init_input(). If this fails, the error path dereferences the dangling
xpad->dev pointer to log the error:

drivers/input/joystick/xpad.c:xpad_presence_work() {
    ...
		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);
    ...
}

Can we avoid dereferencing xpad->dev here, or wait to assign xpad->dev until
xpad_init_input() is fully successful?

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