pm_interrupt() stores each incoming byte into pm->data[] before the
packet parser gets a chance to reset pm->idx. If the incoming serial
stream never matches one of the expected packet headers, pm->idx can
advance past the fixed receive buffer and the next IRQ will write beyond
PM_MAX_LENGTH.

Reset stale indices before storing the next byte. Once pm->idx has
already moved past the valid packet buffer state, the current partial
packet can no longer be trusted, so the smallest local recovery is to
drop that stale state and resynchronize from the current byte instead of
carrying the invalid index into the next interrupt.

Found by static code analysis.
Fixes: 98b013eb7a94 ("Input: penmount - rework handling of different protocols")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
v2:
- note that the issue was found by static code analysis
- explain why resetting the stale index is the preferred resynchronization path
- add a Fixes tag

 drivers/input/touchscreen/penmount.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/touchscreen/penmount.c b/drivers/input/touchscreen/penmount.c
index 4b57b6664e37..ba09096c6573 100644
--- a/drivers/input/touchscreen/penmount.c
+++ b/drivers/input/touchscreen/penmount.c
@@ -163,6 +163,9 @@ static irqreturn_t pm_interrupt(struct serio *serio,
 {
 	struct pm *pm = serio_get_drvdata(serio);
 
+	if (pm->idx >= pm->packetsize || pm->idx >= PM_MAX_LENGTH)
+		pm->idx = 0;
+
 	pm->data[pm->idx] = data;
 
 	pm->parse_packet(pm);
-- 
2.50.1 (Apple Git-155)

Hi Dmitry,

You are right, thanks for pointing this out.

I rechecked the control flow and my analysis was wrong here. If the
first byte does not match a supported packet header, the left side of
the && fails and ++pm->idx is not evaluated, so pm->idx stays at 0. If
the first byte does match, pm->idx only advances until packetsize and
is then reset to 0 in the handler.

So this does not provide a path for pm->idx to grow past the packet
buffer. My analyzer missed the short-circuiting behavior of && here.

I will drop this patch.

Best regards,
Pengpeng

Hi Pengpeng,

On Tue, Mar 24, 2026 at 09:14:42PM +0800, Pengpeng Hou wrote:
> pm_interrupt() stores each incoming byte into pm->data[] before the
> packet parser gets a chance to reset pm->idx. If the incoming serial
> stream never matches one of the expected packet headers, pm->idx can
> advance past the fixed receive buffer and the next IRQ will write beyond
> PM_MAX_LENGTH.

How will it advance? The handlers do:

	if (byte_0_check() && pm->packetsize == ++pm->idx)
		...

If we never match any of the protocols then pm->idx will never advance
past 0 (and we will keep overwriting the first byte of the packet
array).

Does your analyzer miss the "short-circuiting" nature of && operator?

Thanks.

-- 
Dmitry

On Tue, Mar 24, 2026 at 09:14:42PM +0800, Pengpeng Hou wrote:
> pm_interrupt() stores each incoming byte into pm->data[] before the
> packet parser gets a chance to reset pm->idx. If the incoming serial
> stream never matches one of the expected packet headers, pm->idx can
> advance past the fixed receive buffer and the next IRQ will write beyond
> PM_MAX_LENGTH.
> 
> Reset stale indices before storing the next byte. Once pm->idx has
> already moved past the valid packet buffer state, the current partial
> packet can no longer be trusted, so the smallest local recovery is to
> drop that stale state and resynchronize from the current byte instead of
> carrying the invalid index into the next interrupt.
> 
> Found by static code analysis.

Missed blank line here. No need to resend until maintainers ask explicitly for
that.

...

The explanation sounds sane, but I'm not familiar enough with how this device
works. In case others consider this good, feel free to add

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko