Re: [PATCH 1/6] Input: mms114 - fix touch indexing for MMS134S and MMS136

[Bryam Vargas <hexlabsecurity@proton.me>]

Hi Dmitry,

The indexing fix looks correct -- deriving the byte offset from event_size
instead of leaning on sizeof(struct mms114_touch) is the right call, and the
cast is fine since the struct is __packed (no alignment issue at the 6-byte
stride, and the consumers only touch bytes 0..5).

Two things that might be worth folding into the series:

1) Fixes: tag.  The 6-byte event path for MMS136 -- and therefore this
   stride-vs-index mismatch -- predates ab108678195f.  It was introduced in

       53fefdd1d3a3 ("Input: mms114 - support MMS136")

   which added MMS136_EVENT_SIZE and the "packet_size / MMS136_EVENT_SIZE"
   branch while the loop already indexed with the 8-byte struct stride;
   ab108678195f ("support MMS134S") only extended that branch to MMS134S.
   So MMS136 hardware has mis-parsed multi-touch since v5.13.  Tagging

       Fixes: 53fefdd1d3a3 ("Input: mms114 - support MMS136")

   (in addition to, or instead of, ab108678195f) lets stable pick it up for
   the MMS136 range as well.

2) Unbounded packet_size.  Since 1/6 already rewrites this handler: packet_size
   comes straight from the device's PACKET_SIZE register (a single byte, so
   1..255 after the "<= 0" guard) and is then used unclamped both as the read
   length

       __mms114_read_reg(data, MMS114_INFORMATION, packet_size, (u8 *)touch);

   into the 80-byte touch[MMS114_MAX_TOUCH] stack buffer, and as the divisor
   for touch_size -- which is never bounded by MMS114_MAX_TOUCH.  A device that
   reports packet_size > 80 overflows the stack buffer on the read, and even
   with the indexing fix the loop still walks past it (touch_size > 10).  A
   one-liner before the division closes both:

       if (packet_size <= 0)
               goto out;
   +   packet_size = min_t(int, packet_size, sizeof(touch));

   This one is pre-existing -- the unbounded read goes back to the original
   driver -- so it is really a separate patch in the series rather than part
   of the indexing fix, but it seemed worth raising while the code is in
   flight.

Thanks,
Bryam