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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Input: adi: off-by-one check and uninvalidated streams in adi_move_bits()
--

commit 72de23854de8740c131cf08cbda4e0025c231b6c
Author: Guanglei Zhu <zhugl3@xiaopeng.com>

Input: adi - bound the two-stream packet merge

This commit adds bounds checks in adi_move_bits() before merging the
second ADI stream into adi[0].data to prevent out-of-bounds reads and
writes when the packet length is oversized.

> diff --git a/drivers/input/joystick/adi.c b/drivers/input/joystick/adi.c
> index e07b71978509b..03535e58d6a60 100644
> --- a/drivers/input/joystick/adi.c
> +++ b/drivers/input/joystick/adi.c
> @@ -168,6 +168,9 @@ 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)
> +		return;

[Severity: Medium]
Should the first check here use >= ADI_MAX_LENGTH instead of > ADI_MAX_LENGTH,
and should adi[0].ret and adi[1].ret be invalidated before returning?

In struct adi, data is declared as unsigned char data[ADI_MAX_LENGTH], with
index 0 storing the start bit and payload bits stored at 1-based indices
1..ret. In adi_get_bits(), the index is incremented before checking against
adi->ret:

	if ((adi->idx += count) > adi->ret) return 0;
	for (i = 0; i < count; i++)
		bits |= ((adi->data[adi->idx - i] >> 5) & 1) << i;

Because adi[0].ret + adi[1].ret > ADI_MAX_LENGTH allows adi[0].ret to reach
ADI_MAX_LENGTH (256), if adi_decode() consumes 256 bits, adi_get_bits() will
reach adi->idx == adi->ret == 256 and read adi[0].data[256] one byte past the
end of adi[0].data[].

In addition, once adi_move_bits() has verified that the packet is in 2-stream
mode (!(adi[0].data[0] & 0x20) && (adi[1].data[0] & 0x20)), returning early
without setting adi[0].ret = adi[1].ret = -1 leaves both unmerged half-streams
marked valid (ret > 0). Wouldn't that cause adi_connect() and adi_read() to
decode the unmerged half-streams in adi[0] and adi[1] as if they were valid
standalone packets instead of rejecting the malformed 2-stream read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103084135.426629-3-zhugl3@xiaopeng.com?part=1