[PATCH] Input: wacom_w8001 - validate index before storing data byte

[PATCH] Input: wacom_w8001 - validate index before storing data byte

[Muhammad Bilal]

w8001_interrupt() stores every incoming byte at w8001->data[w8001->idx]
before the following switch on w8001->idx++ has a chance to detect an
invalid packet and reset idx. The switch only resets idx for the
specific packet lengths it recognizes; once idx has advanced past all
of those (W8001_PKTLEN_TOUCH2FG - 1 at most), any further byte falls
into default, where idx is only reset for pen-only devices without a
touch_dev (the ThinkPad X60 workaround). A touch-capable device fed a
malformed or overlong packet therefore has nothing to stop idx from
growing without bound, and w8001->data[w8001->idx] = data runs past
the end of the W8001_MAX_LENGTH-sized array.

Reset idx before it is used as an index whenever it has reached the
end of the buffer, independent of device type, so the array store is
always in range and the existing lead-byte resync in case 0 can take
over on the next byte.

Fixes: 3eb1aa43ef5c ("Input: add support for Wacom W8001 penabled serial touchscreen")
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/input/touchscreen/wacom_w8001.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index d8d1cdc3f09e..bbbe7bc69776 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -285,6 +285,14 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
 	struct w8001_coord coord;
 	unsigned char tmp;
 
+	/*
+	 * Resync if a malformed or overlong packet has pushed idx past
+	 * the end of the data array, so the array store below is always
+	 * in bounds.
+	 */
+	if (w8001->idx >= W8001_MAX_LENGTH)
+		w8001->idx = 0;
+
 	w8001->data[w8001->idx] = data;
 	switch (w8001->idx++) {
 	case 0:
-- 
2.55.0

Re: [PATCH] Input: wacom_w8001 - validate index before storing data byte

[Dmitry Torokhov]

Hi Muhammad,

On Sun, Sep 20, 2026 at 12:21:53AM +0500, Muhammad Bilal wrote:
> w8001_interrupt() stores every incoming byte at w8001->data[w8001->idx]
> before the following switch on w8001->idx++ has a chance to detect an
> invalid packet and reset idx. The switch only resets idx for the
> specific packet lengths it recognizes; once idx has advanced past all
> of those (W8001_PKTLEN_TOUCH2FG - 1 at most), any further byte falls
> into default, where idx is only reset for pen-only devices without a
> touch_dev (the ThinkPad X60 workaround). A touch-capable device fed a
> malformed or overlong packet therefore has nothing to stop idx from
> growing without bound, and w8001->data[w8001->idx] = data runs past
> the end of the W8001_MAX_LENGTH-sized array.

This analysis does not match the code.

w8001->idx starts at 0 and is incremented by 1 on each invocation via
switch (w8001->idx++). To advance past W8001_PKTLEN_TOUCH2FG - 1 (12),
w8001->idx would first have to pass through 12, which matches:

	/* 2 finger touch packet */
	case W8001_PKTLEN_TOUCH2FG - 1:
		w8001->idx = 0;
		parse_multi_touch(w8001);
		break;

Unlike the shorter packet length cases, this case has no conditional
break and unconditionally resets w8001->idx to 0. Since W8001_MAX_LENGTH
is 13 (matching W8001_PKTLEN_TOUCH2FG), w8001->idx is always in the
[0, 12] range when entering w8001_interrupt().

As a result, w8001->idx >= W8001_MAX_LENGTH is unreachable and the array
store cannot go out of bounds.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: wacom_w8001 - validate index before storing data byte

[Muhammad Bilal]

Hi Dmitry,

Thanks for the review. You are right. idx advances by one per call and
the W8001_PKTLEN_TOUCH2FG - 1 case resets it unconditionally, so it
always stays in [0, 12] against a 13 byte buffer. I rechecked against
current master and w8001_interrupt() is the only place that writes
idx, so the store cannot go out of bounds. The commit message was
wrong to say nothing stops idx from growing.

Please drop this patch.

Thanks,
Muhammad

On Sun, Sep 20, 2026 at 9:02 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Muhammad,
>
> On Sun, Sep 20, 2026 at 12:21:53AM +0500, Muhammad Bilal wrote:
> > w8001_interrupt() stores every incoming byte at w8001->data[w8001->idx]
> > before the following switch on w8001->idx++ has a chance to detect an
> > invalid packet and reset idx. The switch only resets idx for the
> > specific packet lengths it recognizes; once idx has advanced past all
> > of those (W8001_PKTLEN_TOUCH2FG - 1 at most), any further byte falls
> > into default, where idx is only reset for pen-only devices without a
> > touch_dev (the ThinkPad X60 workaround). A touch-capable device fed a
> > malformed or overlong packet therefore has nothing to stop idx from
> > growing without bound, and w8001->data[w8001->idx] = data runs past
> > the end of the W8001_MAX_LENGTH-sized array.
>
> This analysis does not match the code.
>
> w8001->idx starts at 0 and is incremented by 1 on each invocation via
> switch (w8001->idx++). To advance past W8001_PKTLEN_TOUCH2FG - 1 (12),
> w8001->idx would first have to pass through 12, which matches:
>
>         /* 2 finger touch packet */
>         case W8001_PKTLEN_TOUCH2FG - 1:
>                 w8001->idx = 0;
>                 parse_multi_touch(w8001);
>                 break;
>
> Unlike the shorter packet length cases, this case has no conditional
> break and unconditionally resets w8001->idx to 0. Since W8001_MAX_LENGTH
> is 13 (matching W8001_PKTLEN_TOUCH2FG), w8001->idx is always in the
> [0, 12] range when entering w8001_interrupt().
>
> As a result, w8001->idx >= W8001_MAX_LENGTH is unreachable and the array
> store cannot go out of bounds.
>
> Thanks.
>
> --
> Dmitry