The IRQ handler 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 writing the next byte so malformed packet
streams cannot walk past the end of the local packet buffer.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 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 Andy,

This was found during static code analysis of the packet receive path.

About the fix: my reasoning was that once pm->idx has already moved past
the valid packet buffer state, the current partial packet is no longer
usable, so the safest local recovery is to drop that stale state and
resynchronize from the current byte. That is why I reset the index before
storing the next byte.

I did not choose to ignore the IRQ entirely because the interrupt has
already delivered a byte, and simply returning without resetting the stale
state would leave the parser in the same invalid condition for the next
interrupt. Resetting the index seemed like the smallest change that both
prevents the out-of-bounds write and lets the parser recover cleanly.

Thanks,
Pengpeng

On Tue, Mar 24, 2026 at 10:29:50AM +0800, Pengpeng Hou wrote:
> 
> This was found during static code analysis of the packet receive path.
> 
> About the fix: my reasoning was that once pm->idx has already moved past
> the valid packet buffer state, the current partial packet is no longer
> usable, so the safest local recovery is to drop that stale state and
> resynchronize from the current byte. That is why I reset the index before
> storing the next byte.
> 
> I did not choose to ignore the IRQ entirely because the interrupt has
> already delivered a byte, and simply returning without resetting the stale
> state would leave the parser in the same invalid condition for the next
> interrupt. Resetting the index seemed like the smallest change that both
> prevents the out-of-bounds write and lets the parser recover cleanly.

Good, (summary of) this should be in the commit message in the first place.

-- 
With Best Regards,
Andy Shevchenko

On Mon, Mar 23, 2026 at 08:17:15PM +0800, Pengpeng Hou wrote:
> The IRQ handler 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 did you find the issue? Any assistance?

> Reset stale indices before writing the next byte so malformed packet
> streams cannot walk past the end of the local packet buffer.

Why do you think this is the best possible approach? Maybe we should
simply ignore IRQ or handle it without any further actions?

-- 
With Best Regards,
Andy Shevchenko

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