Re: [PATCH v2 1/3] Input: xpad - add out-of-bounds checks for xpadone

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

Pre-existing issues:
- [High] The xpadone_process_packet function processes GIP_CMD_ANNOUNCE packets by directly calling xpad_start_input() from within the atomic xpad_irq_in URB completion handler, causing a kernel panic due to blocking operations.
--

--- Patch [1]: [PATCH v2 1/3] Input: xpad - add out-of-bounds checks for xpadone ---
commit d812a96c06a9e629fcf6e5c81511be2d91b754b4
Author: Griffin Kroah-Hartman <[email protected]>

Input: xpad - add out-of-bounds checks for xpadone
    
This commit adds size checks for the "len" variable in xpadone_process_packet().
It prevents out-of-bounds accesses to the "data" buffer by ensuring the packet
length is sufficient for various commands.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834e..9ce792503b3a4 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
[ ... ]
> @@ -1079,9 +1087,13 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
>  					 error);
>  		}

[Severity: High]
This is a pre-existing issue, but does calling xpad_start_input() here cause
a deadlock or scheduling while atomic bug?

In the preceding GIP_CMD_ANNOUNCE block (shown partially in the context above),
xpad_start_input() is called from xpadone_process_packet():

drivers/input/joystick/xpad.c:xpadone_process_packet() {
    ...
    } else if (data[0] == GIP_CMD_ANNOUNCE) {
        ...
        error = xpad_start_input(xpad);
    ...
}

Since xpadone_process_packet() is called from the atomic URB completion handler
(xpad_irq_in), wouldn't this execution context be atomic?

Looking at xpad_start_input(), it uses GFP_KERNEL and can call usb_kill_urb()
on xpad->irq_in if xpad_start_xbox_one() fails:

drivers/input/joystick/xpad.c:xpad_start_input() {
    if (usb_submit_urb(xpad->irq_in, GFP_KERNEL))
        return -EIO;

    if (xpad->xtype == XTYPE_XBOXONE) {
        error = xpad_start_xbox_one(xpad);
        if (error) {
            usb_kill_urb(xpad->irq_in);
            return error;
        }
    }
    ...
}

Could the GFP_KERNEL allocation cause scheduling while atomic? Also, if an error
occurs, wouldn't usb_kill_urb(xpad->irq_in) wait on the very URB handler that is
currently executing, leading to a deadlock?

>  	} 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/[email protected]?part=1