[PATCH v3] USB: serial: garmin_gps: fix signed integer underflow and OOB read on packet length

[Hui Peng <benquike@gmail.com>]

getDataLength() returns a signed int from __le32_to_cpup((__le32
*)(garmin_data_p + 8)). When bit 31 is set (e.g. 0x80000004), len is
negative (-2147483644), bypassing the upper bound check (GSP_INITIAL_OFFSET
+ len > GSP_MAX_BUFSIZ) in gsp_send() and triggering a KASAN slab-out-of-
bounds read in garmin_write_bulk() when GARMIN_PKTHDR_LENGTH + len wraps
around to 16.

Kernel stack trace:
BUG: KASAN: slab-out-of-bounds in garmin_write_bulk+0x164/0x3b0
Read of size 16 at addr ffff88800791400c by task poc_verify/188
Call Trace:
 <TASK>
 dump_stack_lvl+0x4d/0x70
 print_report+0xc4/0x610
 kasan_report+0xb8/0xf0
 kasan_check_range+0x118/0x190
 memcpy+0x24/0x60
 garmin_write_bulk+0x164/0x3b0
 gsp_send+0x218/0x490
 garmin_write+0x142/0x2c0
 tty_write+0x294/0x540
 vfs_write+0x412/0x640
 </TASK>


Note that garmin_write_bulk() and garmin_write_bulk_callback() are
triggered from local userspace calling write(fd, ...) on /dev/ttyUSB0
(even with a normal USB device attached) by writing a 12-byte packet
with a negative 32-bit length field (e.g. 0x80ffffff) at byte offset 4.

Kernel stack trace (Linux 7.3.0-rc3):
 ==================================================================
 BUG: KASAN: slab-out-of-bounds in garmin_write_bulk+0x922/0xbf0
 Read of size 12 at addr ffff88800f8ef958 by task usb_poc_verify/162
 Call Trace:
  <TASK>
  dump_stack_lvl+0x70/0xa0
  print_report+0x153/0x4c6
  kasan_report+0xf1/0x120
  kasan_check_range+0x11c/0x200
  __asan_memcpy+0x29/0x70
  garmin_write_bulk+0x922/0xbf0
  garmin_write+0x3e6/0x770
  serial_write+0x167/0x2b0
  n_tty_write+0x5f4/0x1020
  file_tty_write.isra.0+0x411/0x760
  vfs_write+0x671/0xd20
  ksys_write+0x1bb/0x210
  do_syscall_64+0xda/0x4b0
  </TASK>
 ==================================================================
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
v3: Add a Fixes: tag and use the conventional subject prefix.

The length arithmetic in gsp_send() and nat_receive() is unchanged
since the initial git import - garmin_gps.c is already present in
1da177e4c3f4 with the same signed getDataLength() and the same missing
bound - so that is the tag. The later commits git blame surfaces are
not introducers: af6d780b5787 ("garmin_gps: Coding style") and
fb571101af63 ("USB: serial: fix compare_const_fl.cocci warnings") are
cosmetic, and b4072f46e57f only re-nested an existing condition.

Greg asked on an earlier posting whether this is an untrusted-device
issue or something a local user can trigger. To be explicit: hunks 1
and 2 are reached from userspace. gsp_send() consumes the buffer that
write() fills via garmin_write(), so a local user with access to
/dev/ttyUSBn can drive the length arithmetic directly without any
malicious hardware. Hunk 3 is the device-input side and is plain
hardening in the sense of
Documentation/process/threat-model.rst - we trust the device, and I am
not claiming otherwise.

On scope, since it is fair to ask why hunk 3 is here at all: the
urb->transfer_buffer_length >= 5 guard in
garmin_write_bulk_callback() covers a read added later, by
468d13623b6c ("USB: serial: garmin_gps: fixes package loss if used
from gpsbabel"). I kept it in the same patch because the short buffer
it reads past is produced by the same unvalidated length arithmetic,
but I am happy to split it out if you would rather have it separate.

The datalen < 0 test does still do something despite the accessors
becoming __u32: gsp_send() assigns into an int datalen. If you would
rather I made datalen a u32 and kept only the upper bound, say so and
I will respin.

v2: Send to linux-usb@vger.kernel.org via git send-email with Assisted-by: LLM tag and clarify local userspace write() trigger.
 drivers/usb/serial/garmin_gps.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/garmin_gps.c b/drivers/usb/serial/garmin_gps.c
index 8020149f5..ea10af159 100644
--- a/drivers/usb/serial/garmin_gps.c
+++ b/drivers/usb/serial/garmin_gps.c
@@ -208,12 +208,12 @@ static inline int getLayerId(const __u8 *usbPacket)
 	return __le32_to_cpup((__le32 *)(usbPacket));
 }
 
-static inline int getPacketId(const __u8 *usbPacket)
+static inline __u32 getPacketId(const __u8 *usbPacket)
 {
 	return __le32_to_cpup((__le32 *)(usbPacket+4));
 }
 
-static inline int getDataLength(const __u8 *usbPacket)
+static inline __u32 getDataLength(const __u8 *usbPacket)
 {
 	return __le32_to_cpup((__le32 *)(usbPacket+8));
 }
@@ -607,6 +607,10 @@ static int gsp_send(struct garmin_data *garmin_data_p,
 	if (k >= GARMIN_PKTHDR_LENGTH) {
 		pktid  = getPacketId(garmin_data_p->outbuffer);
 		datalen = getDataLength(garmin_data_p->outbuffer);
+		if (datalen < 0 || datalen > GPS_OUT_BUFSIZ - GARMIN_PKTHDR_LENGTH) {
+			garmin_data_p->outsize = 0;
+			return -3;
+		}
 		i = GARMIN_PKTHDR_LENGTH + datalen;
 		if (k < i)
 			return 0;
@@ -769,8 +773,13 @@ static int nat_receive(struct garmin_data *garmin_data_p,
 
 		/* do we have a complete packet ? */
 		if (garmin_data_p->insize >= GARMIN_PKTHDR_LENGTH) {
-			len = GARMIN_PKTHDR_LENGTH+
-			   getDataLength(garmin_data_p->inbuffer);
+			__u32 dlen = getDataLength(garmin_data_p->inbuffer);
+
+			if (dlen > GPS_IN_BUFSIZ - GARMIN_PKTHDR_LENGTH) {
+				garmin_data_p->insize = 0;
+				break;
+			}
+			len = GARMIN_PKTHDR_LENGTH + dlen;
 			if (garmin_data_p->insize >= len) {
 				garmin_write_bulk(garmin_data_p->port,
 						   garmin_data_p->inbuffer,
@@ -951,7 +960,8 @@ static void garmin_write_bulk_callback(struct urb *urb)
 		struct garmin_data *garmin_data_p =
 					usb_get_serial_port_data(port);
 
-		if (getLayerId(urb->transfer_buffer) == GARMIN_LAYERID_APPL) {
+		if (urb->transfer_buffer_length >= 5 &&
+		    getLayerId(urb->transfer_buffer) == GARMIN_LAYERID_APPL) {
 
 			if (garmin_data_p->mode == MODE_GARMIN_SERIAL) {
 				gsp_send_ack(garmin_data_p,
-- 
2.55.0.1082.g2b9226bbc0-goog