Re: [PATCH] Input: iforce - validate packet lengths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Zero-length USB packets cause an integer underflow that bypasses length checks, triggering a massive out-of-bounds kernel memory read and panic.
- [High] Incomplete bounds checks for joystick and wheel packets allow logical out-of-bounds reads.
- [High] Out-of-bounds read in status report packet processing loop.
--

commit ebc2b4788d350db685692da195ae9d9cd27098a0
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

Input: iforce - validate packet lengths

iforce_process_packet() decodes several packet formats from a
variable-length input buffer. Add minimum length checks for joystick,
wheel and status packets before reading their fixed fields.

> diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c
> index effa76bfd8f9a..54697b252b84d 100644
> --- a/drivers/input/joystick/iforce/iforce-packets.c
> +++ b/drivers/input/joystick/iforce/iforce-packets.c
> @@ -155,6 +155,9 @@ void iforce_process_packet(struct iforce *iforce,
>  	switch (packet_id) {
>  
>  	case 0x01:	/* joystick position data */
> +		if (len < 5)
> +			break;
> +

[Severity: Critical]
This isn't a bug introduced by this patch, but does len underflow when a
zero-length USB packet is received?

Looking at iforce_usb_irq() in drivers/input/joystick/iforce/iforce-usb.c, it
calls iforce_process_packet() with urb->actual_length - 1:

    iforce_process_packet(iforce, iforce_usb->data_in[0],
                          iforce_usb->data_in + 1, urb->actual_length - 1);

If urb->actual_length is 0, this underflows to SIZE_MAX. The new bounds check
here will be bypassed, and subsequent loops can read linearly out of bounds
into unmapped memory, potentially causing a panic.

[Severity: High]
This is a pre-existing issue, but is the len < 5 check sufficient to prevent
out-of-bounds reads when processing hats and buttons?

When iforce_process_packet() calls iforce_report_hats_buttons(), it
unconditionally accesses data[6]:

    input_report_abs(dev, ABS_HAT0X, iforce_hat_to_axis[data[6] >> 4].x);

If a short packet arrives (e.g., actual length 6, leading to len = 5), the
check passes, but we still read stale memory when accessing data[6],
potentially reporting phantom hat movements or button presses.

[ ... ]

>  	case 0x03:	/* wheel position data */
> +		if (len < 4)
> +			break;
> +

[Severity: High]
This is also a pre-existing issue, but this check is similarly insufficient
because iforce_report_hats_buttons() requires at least 7 bytes of data to
safely access data[6].

[ ... ]

>  	case 0x02:	/* status report */
> +		if (len < 2)
> +			break;
> +

[Severity: High]
This isn't a bug introduced by this patch, but could the status report
processing loop read out of bounds?

Downstream in this case block, there is a loop:

    for (j = 3; j < len; j += 2)
        mark_core_as_ready(iforce, get_unaligned_le16(data + j));

If a packet arrives with an even length (e.g., len = 4), the loop condition
j < len allows get_unaligned_le16() to read two bytes starting at offset 3.
This extends beyond the valid payload length of 4, inadvertently reading stale
data that could match and corrupt force-feedback state tracking for unrelated
effects.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706092016.78176-1-pengpeng@iscas.ac.cn?part=1