Re: [PATCH v2 2/3] Input: xpad - add packet length check for xpad_process_packet()

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

New issues:
- [Low] Using `min(len, XPAD_PKT_LEN)` causes a compiler warning due to mismatched types.

Pre-existing issues:
- [High] The patch adds a packet length check to `xpad_process_packet` to prevent OOB reads, but misses identical vulnerabilities in `xpad360_process_packet` and `xpad360w_process_packet`.
- [High] The `xpad->work` workqueue item can execute after `xpad` is freed in the `xpad_probe()` error path for wireless Xbox 360 controllers.
--

--- Patch [2]: [PATCH v2 2/3] Input: xpad - add packet length check for xpad_process_packet() ---
commit 871137dca87547e72930981b59cce6b2b69f5992
Author: Griffin Kroah-Hartman <[email protected]>

Input: xpad - add packet length check for xpad_process_packet()

This patch adds a bounds check to xpad_process_packet() to ensure that the
processed packet is at least 20 bytes long. This prevents a potential
out-of-bounds read if a shorter packet is received.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 9ce792503b3a4..83bc361b15f76 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
[ ... ]
> @@ -1259,7 +1262,7 @@ static void xpad_irq_in(struct urb *urb)
>  		xpadone_process_packet(xpad, 0, xpad->idata, urb->actual_length);

[Severity: Low]
Does the min() check in xpadone_process_packet() cause a compiler warning?

Comparing a u32 (len) with a literal 64 (XPAD_PKT_LEN) using min() causes a
'comparison of distinct pointer types lacks a cast' warning due to strict
type checking. Would min_t(u32, len, XPAD_PKT_LEN) be more appropriate here
to avoid build failures when compiled with -Werror?

>  		break;
>  	default:
> -		xpad_process_packet(xpad, 0, xpad->idata);
> +		xpad_process_packet(xpad, 0, xpad->idata, urb->actual_length);

[Severity: High]
This isn't a bug introduced by this patch, but does this same vulnerability
exist for the Xbox 360 packet handlers?

In xpad_irq_in(), a malicious or malfunctioning USB device sending short
interrupt packets can cause xpad360_process_packet() or
xpad360w_process_packet() to be called without passing or verifying
urb->actual_length. This allows unconditional reads of up to 14 bytes
from the xpad->idata buffer, potentially reading stale data in the same
way this patch prevents for xpad_process_packet().

[Severity: High]
This isn't a bug introduced by this patch, but can the xpad->work workqueue
execute after xpad is freed in the xpad_probe() error path?

If an XTYPE_XBOX360W controller is detected during probe,
xpad360w_start_input() submits xpad->irq_in and then calls
xpad_inquiry_pad_presence(). If this fails, it kills the URB and returns
an error, causing xpad_probe() to free xpad via kfree().

However, if the USB device completes an interrupt transfer immediately
after irq_in is submitted, xpad_irq_in() will call
xpad360w_process_packet(), which schedules xpad->work:

drivers/input/joystick/xpad.c:xpad360w_process_packet() {
    ...
    if (xpad->pad_present != present) {
        xpad->pad_present = present;
        schedule_work(&xpad->work);
    }
    ...
}

Since the error path doesn't call cancel_work_sync(&xpad->work), can this
result in a use-after-free when xpad_presence_work() dereferences the freed
xpad struct?

>  	}
>  
>  exit:

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