Re: [PATCH v2] Input: adi - bound the two-stream packet merge

[Dmitry Torokhov]

Hi Guanglei,

On Tue, Nov 03, 2026 at 07:17:14PM +0800, Guanglei Zhu wrote:
> When a second device shares the wire, adi_move_bits() appends the
> second packet to the first one at an offset taken from the
> 10-bit length field of the ID packet:
>
> 	for (i = 1; i <= adi[1].ret; i++)
> 		adi[0].data[((length - 1) >> 1) + i + 1] = adi[1].data[i];
>
> 	adi[0].ret += adi[1].ret;
>
> Nothing compares the merged packet against the size of the buffer:
> adi[0].data is the last member of struct adi, and struct adi_port
> holds two of them back to back, so an oversized length field makes
> the loop write over the second struct adi - its device pointer, its
> length and its packet - and the inflated ret later walks the reads
> past the buffer as well. The connect path also calls
> adi_move_bits() before the length has been validated at all.
>
> Reject a merge that does not fit in the buffer. Use >= for the
> combined length so adi_get_bits() can never walk to data[ret] with
> ret == ADI_MAX_LENGTH, and invalidate both half-streams on the
> rejection so the unmerged packets are not decoded as standalone
> reads.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Guanglei Zhu <zhugl3@xxxxxxxxxxxx>
> ---
> v2: per review, use >= so a combined ret of ADI_MAX_LENGTH is rejected
>     as well (adi_get_bits() allows idx == ret, and data[idx] with
>     idx == ADI_MAX_LENGTH reads one byte past the array), and set
>     adi[0].ret = adi[1].ret = -1 on the rejection path so the
>     unmerged half-streams are not decoded as standalone packets.
>
>  drivers/input/joystick/adi.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/input/joystick/adi.c b/drivers/input/joystick/adi.c
> index e07b719..41262b7 100644
> --- a/drivers/input/joystick/adi.c
> +++ b/drivers/input/joystick/adi.c
> @@ -168,6 +168,11 @@ static void adi_move_bits(struct adi_port *port, int length)
>  
>  	if (adi[0].ret <= 0 || adi[1].ret <= 0) return;
>  	if (adi[0].data[0] & 0x20 || ~adi[1].data[0] & 0x20) return;
> +	if (adi[0].ret + adi[1].ret >= ADI_MAX_LENGTH ||
> +	    ((length - 1) >> 1) + adi[1].ret + 1 >= ADI_MAX_LENGTH) {
> +		adi[0].ret = adi[1].ret = -1;
> +		return;
> +	}
>  
>  	for (i = 1; i <= adi[1].ret; i++)
>  		adi[0].data[((length - 1) >> 1) + i + 1] = adi[1].data[i];

In adi_move_bits(), length can also be 0 (either from the 10-bit header
in adi_connect() or when port->adi[0].length is 0 in adi_read()), so
please reject length <= 0 as well. It would also be cleaner to compute
the start offset (for example, offset = ((length - 1) >> 1) + 1) once
and use offset + adi[1].ret and offset + i in both the bounds check and
the copy loop.

Since you are hardening this driver against malformed packets on the
gameport wire, there are a few more bounds issues in the same file worth
addressing:

In adi_read_packet(), the condition adi[i].ret < ADI_MAX_LENGTH is off
by one because adi[i].data[++adi[i].ret] pre-increments ret, allowing a
write to adi[i].data[ADI_MAX_LENGTH] when ret is ADI_MAX_LENGTH - 1. It
should check adi[i].ret < ADI_MAX_LENGTH - 1.

In adi_id_decode(), when ADI_FLAG_10BIT is set, adi->axes10 =
adi->axes8 - i can underflow if i > adi->axes8, and for ADI_ID_TPD and
ADI_ID_WGP, adi->buttons -= 4 can underflow if adi->buttons < 4. Please
also switch the non-string byte fields and buffers in struct adi (such
as id, buttons, axes10, axes8, hats, and data) from char/unsigned char
to u8 (and s8 for pad) so their signedness does not vary across
architectures. Finally, the decoded axis, hat, and button counts are
not checked against the sizes of the static adi_abs[] and adi_key[]
tables for the device ID before adi_init_input(), adi_init_center(),
and adi_decode() walk adi->abs and adi->key.

Could you please fold these fixes in (or split them into a small series
if you prefer)?

Thanks.

-- 
Dmitry