Input: zforce_ts - fix playload length check

PATCH In review Dmitry Torokhov 4 participants 8 messages Jul 27, 2015 – Jul 28, 2015

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Dmitry Torokhov

author maintainer

2015-07-27 21:06 UTC #0 lore raw

PATCH 1/1 Input: zforce_ts - fix playload length check

Commit 7d01cd261c76f95913c81554a751968a1d282d3a ("Input: zforce - don't overwrite the stack") attempted to add a check for payload size being too large for the supplied buffer. Unfortunately with the currently selected buffer size the comparison is always false as buffer size is larger than the value a single byte can hold, and that results in compiler warnings. Additionally the check was incorrect as it was not accounting for the already read 2 bytes of data stored in the buffer.

Fixes 7d01cd261c76f95913c81554a751968a1d282d3a

Reported-by kbuild test robot <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

This seems to shut up my GCC, I wonder if it is going to work gfor everyone or we better add BUILD_BUG_ON(FRAME_MAXSIZE < 257) and a comment and remove check.

 drivers/input/touchscreen/zforce_ts.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/input/touchscreen/zforce_ts.c +3 -1

index 2554efd..542ff02 100644
@@ -441,7 +441,9 @@ static int zforce_read_packet(struct zforce_ts *ts, u8 *buf)
 		goto unlock;
 	}
 
-	if (buf[PAYLOAD_LENGTH] == 0 || buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE) {
+	if (buf[PAYLOAD_LENGTH] == 0 ||
+	    (FRAME_MAXSIZE - 2 < 255 &&
+	     buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE - 2)) {
 		dev_err(&client->dev, "invalid payload length: %d\n",
 			buf[PAYLOAD_LENGTH]);
 		ret = -EIO;

Signature · 5 lines

2.5.0.rc2.392.g76e840b


-- 
Dmitry

Heiko Stübner

2015-07-27 21:35 UTC #1 lore raw

Hi Dmitry,

Am Montag, 27. Juli 2015, 14:06:19 schrieb Dmitry Torokhov:

Quoted 8 lines

Commit 7d01cd261c76f95913c81554a751968a1d282d3a ("Input: zforce - don't overwrite the stack") attempted to add a check for payload size being too large for the supplied buffer. Unfortunately with the currently selected buffer size the comparison is always false as buffer size is larger than the value a single byte can hold, and that results in compiler warnings. Additionally the check was incorrect as it was not accounting for the already read 2 bytes of data stored in the buffer.

Quoted 8 lines

Fixes 7d01cd261c76f95913c81554a751968a1d282d3a
Reported-by kbuild test robot <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
This seems to shut up my GCC, I wonder if it is going to work gfor everyone or we better add BUILD_BUG_ON(FRAME_MAXSIZE < 257) and a comment and remove check.

needed a bit to get to know my old zforce driver again ;-)

I may be blind, but currently I fail to see what problem the original patch actually tries to fix.

buf[PAYLOAD_LENGTH] is an u8, so the max value it can contain is 255. The i2c_master_recv reads buf[PAYLOAD_LENGTH]-bytes into the buffer starting at buf[PAYLOAD_BODY] (= buf[2]). So it reads at max 255 bytes into a 257 byte big buffer starting at index 2.

zforce_read_packet, also is an internal function used only by the interrupt handler, which always only calls it with a buffer of FRAME_MAXSIZE size.

The original patch said "If we get a corrupted packet with PAYLOAD_LENGTH > FRAME_MAXSIZE, we will silently overwrite the stack." but payload_length can never actually be greater than the buffer size?

Very confused
Heiko

Quoted 18 lines

 drivers/input/touchscreen/zforce_ts.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/input/touchscreen/zforce_ts.c index 2554efd..542ff02 100644

drivers/input/touchscreen/zforce_ts.c +3 -1

@@ -441,7 +441,9 @@ static int zforce_read_packet(struct zforce_ts *ts, u8
 *buf) goto unlock;
 	}
 
-	if (buf[PAYLOAD_LENGTH] == 0 || buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE) {
+	if (buf[PAYLOAD_LENGTH] == 0 ||
+	    (FRAME_MAXSIZE - 2 < 255 &&
+	     buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE - 2)) {
 		dev_err(&client->dev, "invalid payload length: %d\n",
 			buf[PAYLOAD_LENGTH]);
 		ret = -EIO;

Dmitry Torokhov

author maintainer

2015-07-27 21:44 UTC #2 lore raw

Quoted 31 lines · Heiko Stübner

Hi Dmitry,
Am Montag, 27. Juli 2015, 14:06:19 schrieb Dmitry Torokhov:
Quoted 8 lines
Commit 7d01cd261c76f95913c81554a751968a1d282d3a ("Input: zforce - don't overwrite the stack") attempted to add a check for payload size being too large for the supplied buffer. Unfortunately with the currently selected buffer size the comparison is always false as buffer size is larger than the value a single byte can hold, and that results in compiler warnings. Additionally the check was incorrect as it was not accounting for the already read 2 bytes of data stored in the buffer.
Quoted 8 lines
Fixes 7d01cd261c76f95913c81554a751968a1d282d3a
Reported-by kbuild test robot <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
This seems to shut up my GCC, I wonder if it is going to work gfor everyone or we better add BUILD_BUG_ON(FRAME_MAXSIZE < 257) and a comment and remove check.
needed a bit to get to know my old zforce driver again ;-)
I may be blind, but currently I fail to see what problem the original patch actually tries to fix.
buf[PAYLOAD_LENGTH] is an u8, so the max value it can contain is 255. The i2c_master_recv reads buf[PAYLOAD_LENGTH]-bytes into the buffer starting at buf[PAYLOAD_BODY] (= buf[2]). So it reads at max 255 bytes into a 257 byte big buffer starting at index 2.

Quoted 7 lines

zforce_read_packet, also is an internal function used only by the interrupt handler, which always only calls it with a buffer of FRAME_MAXSIZE size.
The original patch said "If we get a corrupted packet with PAYLOAD_LENGTH > FRAME_MAXSIZE, we will silently overwrite the stack." but payload_length can never actually be greater than the buffer size?

Right, not unless we for some reason decide to adjust FRAME_MAXSIZE to make it smaller than 257 and then fail to add the check to make sure we do not go past the buffer.

So everything is fine now, but I guess we'd like to be more safe in the future...

Thanks.

Signature · 5 lines

Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Heiko Stübner

Reviewed-by

2015-07-27 21:53 UTC #3 lore raw

Am Montag, 27. Juli 2015, 14:44:42 schrieb Dmitry Torokhov:

Quoted 45 lines · Heiko Stübner

Quoted 39 lines
Hi Dmitry,
Am Montag, 27. Juli 2015, 14:06:19 schrieb Dmitry Torokhov:
Quoted 9 lines
Commit 7d01cd261c76f95913c81554a751968a1d282d3a ("Input: zforce - don't overwrite the stack") attempted to add a check for payload size being too
large for the supplied buffer. Unfortunately with the currently selected buffer size the comparison is always false as buffer size is larger than the value a single byte can hold, and that results in compiler warnings. Additionally the check was incorrect as it was not accounting for the already read 2 bytes of data stored in the buffer.
Quoted 8 lines
Fixes 7d01cd261c76f95913c81554a751968a1d282d3a
Reported-by kbuild test robot <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
This seems to shut up my GCC, I wonder if it is going to work gfor everyone or we better add BUILD_BUG_ON(FRAME_MAXSIZE < 257) and a comment and remove check.
needed a bit to get to know my old zforce driver again ;-)
I may be blind, but currently I fail to see what problem the original patch
actually tries to fix.
buf[PAYLOAD_LENGTH] is an u8, so the max value it can contain is 255. The i2c_master_recv reads buf[PAYLOAD_LENGTH]-bytes into the buffer starting at
buf[PAYLOAD_BODY] (= buf[2]). So it reads at max 255 bytes into a 257 byte big buffer starting at index 2.
zforce_read_packet, also is an internal function used only by the interrupt
handler, which always only calls it with a buffer of FRAME_MAXSIZE size.
Quoted 4 lines
The original patch said "If we get a corrupted packet with PAYLOAD_LENGTH
Quoted 1 line
FRAME_MAXSIZE, we will silently overwrite the stack." but payload_length can never actually be greater than the buffer size?

Quoted 6 lines

Right, not unless we for some reason decide to adjust FRAME_MAXSIZE to make it smaller than 257 and then fail to add the check to make sure we do not go past the buffer.
So everything is fine now, but I guess we'd like to be more safe in the future...

I would argue that FRAME_MAXSIZE already indicates that it should not be changed. It's the maximum size a single frame can be. And this size is a property of the hardware itself, because of the format, 257 bytes is always the maximum you could get (2 bytes header + at max 255 bytes payload).

So this second check (while only taking up a minimal amount of time) actually only checks against kernel-developer making errors in the future and not something the hardware can cause.

But your change itself looks correct, so if you prefer to keep that check you can also add my

Reviewed-by Heiko Stuebner <[email protected]>

Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov

author maintainer

2015-07-27 22:16 UTC #4 lore raw

Quoted 53 lines · Heiko Stübner

Am Montag, 27. Juli 2015, 14:44:42 schrieb Dmitry Torokhov:
Quoted 45 lines · Heiko Stübner
Quoted 39 lines
Hi Dmitry,
Am Montag, 27. Juli 2015, 14:06:19 schrieb Dmitry Torokhov:
Quoted 9 lines
Commit 7d01cd261c76f95913c81554a751968a1d282d3a ("Input: zforce - don't overwrite the stack") attempted to add a check for payload size being too
large for the supplied buffer. Unfortunately with the currently selected buffer size the comparison is always false as buffer size is larger than the value a single byte can hold, and that results in compiler warnings. Additionally the check was incorrect as it was not accounting for the already read 2 bytes of data stored in the buffer.
Quoted 8 lines
Fixes 7d01cd261c76f95913c81554a751968a1d282d3a
Reported-by kbuild test robot <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
This seems to shut up my GCC, I wonder if it is going to work gfor everyone or we better add BUILD_BUG_ON(FRAME_MAXSIZE < 257) and a comment and remove check.
needed a bit to get to know my old zforce driver again ;-)
I may be blind, but currently I fail to see what problem the original patch
actually tries to fix.
buf[PAYLOAD_LENGTH] is an u8, so the max value it can contain is 255. The i2c_master_recv reads buf[PAYLOAD_LENGTH]-bytes into the buffer starting at
buf[PAYLOAD_BODY] (= buf[2]). So it reads at max 255 bytes into a 257 byte big buffer starting at index 2.
zforce_read_packet, also is an internal function used only by the interrupt
handler, which always only calls it with a buffer of FRAME_MAXSIZE size.
Quoted 4 lines
The original patch said "If we get a corrupted packet with PAYLOAD_LENGTH
Quoted 1 line
FRAME_MAXSIZE, we will silently overwrite the stack." but payload_length can never actually be greater than the buffer size?
Quoted 6 lines
Right, not unless we for some reason decide to adjust FRAME_MAXSIZE to make it smaller than 257 and then fail to add the check to make sure we do not go past the buffer.
So everything is fine now, but I guess we'd like to be more safe in the future...

Quoted 6 lines

I would argue that FRAME_MAXSIZE already indicates that it should not be changed. It's the maximum size a single frame can be. And this size is a property of the hardware itself, because of the format, 257 bytes is always the maximum you could get (2 bytes header + at max 255 bytes payload).
So this second check (while only taking up a minimal amount of time)

It does not take any time as it gets optimized out completely (with current FRAME_MAXSIZE value).

Quoted 3 lines

actually
only checks against kernel-developer making errors in the future and not something the hardware can cause.

Right.

Quoted 5 lines

But your change itself looks correct, so if you prefer to keep that check you can also add my
Reviewed-by Heiko Stuebner <[email protected]>

I guess I'll sit on it. Another option is to revert the original change and be done with it.

Thanks.

Signature · 1 line

Dmitry

Geert Uytterhoeven

2015-07-28 10:23 UTC #5 lore raw

In reply to Dmitry Torokhov

Quoted 7 lines · Dmitry Torokhov

Commit 7d01cd261c76f95913c81554a751968a1d282d3a ("Input: zforce - don't overwrite the stack") attempted to add a check for payload size being too large for the supplied buffer. Unfortunately with the currently selected buffer size the comparison is always false as buffer size is larger than the value a single byte can hold, and that results in compiler warnings. Additionally the check was incorrect as it was not accounting for the already read 2 bytes of data stored in the buffer.

The check was indeed incorrect.

Quoted 17 lines

Fixes 7d01cd261c76f95913c81554a751968a1d282d3a
Reported-by kbuild test robot <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
This seems to shut up my GCC, I wonder if it is going to work gfor everyone or we better add BUILD_BUG_ON(FRAME_MAXSIZE < 257) and a comment and remove check.
 drivers/input/touchscreen/zforce_ts.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/input/touchscreen/zforce_ts.c +0 -0
index 2554efd..542ff02 100644

drivers/input/touchscreen/zforce_ts.c static int zforce_read_packet(struct zforce_ts *ts, u8 *buf) · line 446

Quoted 7 lines

drivers/input/touchscreen/zforce_ts.c +3 -1

@@ -441,7 +441,9 @@ static int zforce_read_packet(struct zforce_ts *ts, u8 *buf)
                goto unlock;
        }
 
-       if (buf[PAYLOAD_LENGTH] == 0 || buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE) {
+       if (buf[PAYLOAD_LENGTH] == 0 ||
+           (FRAME_MAXSIZE - 2 < 255 &&
+            buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE - 2)) {

Doesn't help with gcc 4.1.2 :-(

Before:

drivers/input/touchscreen/zforce_ts.c: In function ‘zforce_read_packet’:
drivers/input/touchscreen/zforce_ts.c:432: warning: comparison is always false due to limited range of data type

After:

drivers/input/touchscreen/zforce_ts.c: In function ‘zforce_read_packet’:
drivers/input/touchscreen/zforce_ts.c:434: warning: comparison is always false due to limited range of data type

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Dirk Behme

2015-07-28 11:28 UTC #6 lore raw

Quoted 45 lines · Geert Uytterhoeven

Quoted 7 lines · Dmitry Torokhov
Commit 7d01cd261c76f95913c81554a751968a1d282d3a ("Input: zforce - don't overwrite the stack") attempted to add a check for payload size being too large for the supplied buffer. Unfortunately with the currently selected buffer size the comparison is always false as buffer size is larger than the value a single byte can hold, and that results in compiler warnings. Additionally the check was incorrect as it was not accounting for the already read 2 bytes of data stored in the buffer.
The check was indeed incorrect.
Quoted 17 lines
Fixes 7d01cd261c76f95913c81554a751968a1d282d3a
Reported-by kbuild test robot <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
This seems to shut up my GCC, I wonder if it is going to work gfor everyone or we better add BUILD_BUG_ON(FRAME_MAXSIZE < 257) and a comment and remove check.
  drivers/input/touchscreen/zforce_ts.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
drivers/input/touchscreen/zforce_ts.c +0 -0
index 2554efd..542ff02 100644
drivers/input/touchscreen/zforce_ts.c static int zforce_read_packet(struct zforce_ts *ts, u8 *buf) · line 446
Quoted 7 lines
drivers/input/touchscreen/zforce_ts.c +3 -1
@@ -441,7 +441,9 @@ static int zforce_read_packet(struct zforce_ts *ts, u8 *buf)
                 goto unlock;
         }
 
-       if (buf[PAYLOAD_LENGTH] == 0 || buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE) {
+       if (buf[PAYLOAD_LENGTH] == 0 ||
+           (FRAME_MAXSIZE - 2 < 255 &&
+            buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE - 2)) {
Doesn't help with gcc 4.1.2 :-(
Before:
drivers/input/touchscreen/zforce_ts.c: In function ‘zforce_read_packet’:
drivers/input/touchscreen/zforce_ts.c:432: warning: comparison is always false due to limited range of data type

Quoted 5 lines

After:
drivers/input/touchscreen/zforce_ts.c: In function ‘zforce_read_packet’:
drivers/input/touchscreen/zforce_ts.c:434: warning: comparison is always false due to limited range of data type

If it's easier, then just revert 7d01cd261c76f95913c81.

Sorry! It seems that at least 4 people have overlooked this issue :(

Best regards

Dirk

Btw: Could anybody give me a hint how to get this warning? My GCC 4.8.1 with kernel default ARM Cortex A9 kernel options doesn't give me anything about this.

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov

author maintainer

2015-07-28 16:30 UTC #7 lore raw

Quoted 48 lines · Dirk Behme

Quoted 47 lines · Geert Uytterhoeven
Quoted 7 lines · Dmitry Torokhov
Commit 7d01cd261c76f95913c81554a751968a1d282d3a ("Input: zforce - don't overwrite the stack") attempted to add a check for payload size being too large for the supplied buffer. Unfortunately with the currently selected buffer size the comparison is always false as buffer size is larger than the value a single byte can hold, and that results in compiler warnings. Additionally the check was incorrect as it was not accounting for the already read 2 bytes of data stored in the buffer.
The check was indeed incorrect.
Quoted 17 lines
Fixes 7d01cd261c76f95913c81554a751968a1d282d3a
Reported-by kbuild test robot <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
This seems to shut up my GCC, I wonder if it is going to work gfor everyone or we better add BUILD_BUG_ON(FRAME_MAXSIZE < 257) and a comment and remove check.
 drivers/input/touchscreen/zforce_ts.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/input/touchscreen/zforce_ts.c +0 -0
index 2554efd..542ff02 100644
drivers/input/touchscreen/zforce_ts.c static int zforce_read_packet(struct zforce_ts *ts, u8 *buf) · line 446
Quoted 7 lines
drivers/input/touchscreen/zforce_ts.c +3 -1
@@ -441,7 +441,9 @@ static int zforce_read_packet(struct zforce_ts *ts, u8 *buf)
                goto unlock;
        }
 
-       if (buf[PAYLOAD_LENGTH] == 0 || buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE) {
+       if (buf[PAYLOAD_LENGTH] == 0 ||
+           (FRAME_MAXSIZE - 2 < 255 &&
+            buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE - 2)) {
Doesn't help with gcc 4.1.2 :-(
Before:
drivers/input/touchscreen/zforce_ts.c: In function ‘zforce_read_packet’:
drivers/input/touchscreen/zforce_ts.c:432: warning: comparison is always false due to limited range of data type
After:

Quoted 8 lines

Quoted 3 lines
drivers/input/touchscreen/zforce_ts.c: In function ‘zforce_read_packet’:
drivers/input/touchscreen/zforce_ts.c:434: warning: comparison is always false due to limited range of data type
If it's easier, then just revert 7d01cd261c76f95913c81.
Sorry! It seems that at least 4 people have overlooked this issue :(

Yes, I guess that is an example where unified diff provides too little of a context...

Quoted 8 lines

Best regards
Dirk
Btw: Could anybody give me a hint how to get this warning? My GCC 4.8.1 with kernel default ARM Cortex A9 kernel options doesn't give me anything about this.

make KBUILD_CFLAGS="-Wtype-limits" drivers/input/touchscreen/zforce_ts.o

may trigger it.

Signature · 5 lines

Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html