[PATCH] media: gspca: etoms: bound device-provided length in sd_pkt_scan

[PATCH] media: gspca: etoms: bound device-provided length in sd_pkt_scan

[Hyunjung Ko]

etoms sd_pkt_scan() is the gspca pkt_scan callback, invoked from the isoc
URB completion (fill_frame() in gspca.c) with (gspca_dev, data, len) where
data points into the URB coherent transfer buffer at this packet's slot and
len is iso_frame_desc[i].actual_length, i.e. the number of bytes the device
actually sent in that isoc packet.

The subdriver overwrites the real packet length with a value taken straight
from the first two device-supplied header bytes:

	len = (int)(((data[0] & 0xc0) << 2) | data[1]);	/* 0 .. 1023 */

and then hands that count to gspca_frame_add() for a header packet
(data += 30) or an inter packet (data += 8):

	gspca_frame_add(gspca_dev, FIRST_PACKET, data, len);
	...
	gspca_frame_add(gspca_dev, INTER_PACKET, data, len);

gspca_frame_add() only bounds the *destination* frame
(image_len + len > PAGE_ALIGN(sizeimage) -> discard); it applies no bound to
the *source*, doing memcpy(image + image_len, data, len) directly. Nothing
clamps the device-declared length against the bytes actually present in the
packet slot.

The isoc transfer buffer is usb_alloc_coherent(psize * npkt) where psize is
the device's reported wMaxPacketSize and npkt defaults to 32; this driver
sets neither cam.npkt nor pkt_size, so both derive from the device. A
malicious camera can declare a small wMaxPacketSize and send a short packet
in the last slot while setting the header bytes to request len up to 1023.
gspca_frame_add() then reads up to ~1023 bytes starting at data+8 (or
data+30) -- past actual_length, past the packet slot, and past the end of
the transfer-buffer allocation -- and copies that adjacent kernel memory
into the vb2 video frame, which is handed to userspace: an out-of-bounds
read / kernel information leak reachable by plugging in a crafted USB
webcam.

A 1-byte over-read of data[1] also occurs when a 1-byte packet is delivered
(fill_frame() only drops zero-length packets).

Keep the real packet length, reject runt packets, and clamp the
device-declared length to the bytes remaining after the header offset
before passing it to gspca_frame_add().

Found by code review; not reproduced (no device/USB-redir setup on hand).

Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/media/usb/gspca/etoms.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/gspca/etoms.c b/drivers/media/usb/gspca/etoms.c
--- a/drivers/media/usb/gspca/etoms.c
+++ b/drivers/media/usb/gspca/etoms.c
@@ -659,24 +659,37 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
 			u8 *data,			/* isoc packet */
 			int len)			/* iso packet length */
 {
 	int seqframe;
+	int datalen;

+	if (len < 2)			/* need the 2-byte packet header */
+		return;
 	seqframe = data[0] & 0x3f;
-	len = (int) (((data[0] & 0xc0) << 2) | data[1]);
+	datalen = (int) (((data[0] & 0xc0) << 2) | data[1]);
 	if (seqframe == 0x3f) {
 		gspca_dbg(gspca_dev, D_FRAM,
-			  "header packet found datalength %d !!\n", len);
+			  "header packet found datalength %d !!\n", datalen);
+		if (len < 30)
+			return;
 		gspca_dbg(gspca_dev, D_FRAM, "G %d R %d G %d B %d",
 			  data[2], data[3], data[4], data[5]);
 		data += 30;
-		/* don't change datalength as the chips provided it */
+		len -= 30;
+		/* the chip-provided length must not exceed the packet */
+		if (datalen > len)
+			datalen = len;
 		gspca_frame_add(gspca_dev, LAST_PACKET, NULL, 0);
-		gspca_frame_add(gspca_dev, FIRST_PACKET, data, len);
+		gspca_frame_add(gspca_dev, FIRST_PACKET, data, datalen);
 		return;
 	}
-	if (len) {
+	if (datalen) {
+		if (len < 8)
+			return;
 		data += 8;
-		gspca_frame_add(gspca_dev, INTER_PACKET, data, len);
+		len -= 8;
+		if (datalen > len)
+			datalen = len;
+		gspca_frame_add(gspca_dev, INTER_PACKET, data, datalen);
 	} else {			/* Drop Packet */
 		gspca_dev->last_packet_type = DISCARD_PACKET;
 	}

[PATCH v2] media: gspca: etoms: bound device-provided length in sd_pkt_scan

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

etoms sd_pkt_scan() is the gspca pkt_scan callback, invoked from the isoc
URB completion (fill_frame() in gspca.c) with (gspca_dev, data, len) where
data points into the URB coherent transfer buffer at this packet's slot and
len is iso_frame_desc[i].actual_length, i.e. the number of bytes the device
actually sent in that isoc packet.

The subdriver overwrites the real packet length with a value taken straight
from the first two device-supplied header bytes:

	len = (int)(((data[0] & 0xc0) << 2) | data[1]);	/* 0 .. 1023 */

and then hands that count to gspca_frame_add() for a header packet
(data += 30) or an inter packet (data += 8):

	gspca_frame_add(gspca_dev, FIRST_PACKET, data, len);
	...
	gspca_frame_add(gspca_dev, INTER_PACKET, data, len);

gspca_frame_add() only bounds the *destination* frame (image_len + len >
PAGE_ALIGN(sizeimage) -> discard); it applies no bound to the *source*,
doing memcpy(image + image_len, data, len) directly. Nothing clamps the
device-declared length against the bytes actually present in the packet
slot.

The isoc transfer buffer is usb_alloc_coherent(psize * npkt) where psize is
the device's reported wMaxPacketSize and npkt defaults to 32; this driver
sets neither cam.npkt nor pkt_size, so both derive from the device. A
malicious camera can declare a small wMaxPacketSize and send a short packet
in the last slot while setting the header bytes to request len up to 1023.
gspca_frame_add() then reads up to ~1023 bytes starting at data+8 (or
data+30) -- past actual_length, past the packet slot, and past the end of
the transfer-buffer allocation -- and copies that adjacent kernel memory
into the vb2 video frame, which is handed to userspace: an out-of-bounds
read / kernel information leak reachable by plugging in a crafted USB
webcam.

A 1-byte over-read of data[1] also occurs when a 1-byte packet is delivered
(fill_frame() only drops zero-length packets).

Keep the real packet length, reject runt packets, and clamp the
device-declared length to the bytes remaining after the header offset
before passing it to gspca_frame_add().

Found by code review; not reproduced (no device/USB-redir setup on hand).

Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
v2: wrap a commit-log line to <=75 cols and drop the space after the
    (int) cast, per the media-ci checkpatch report. No functional change.

 drivers/media/usb/gspca/etoms.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/gspca/etoms.c b/drivers/media/usb/gspca/etoms.c
--- a/drivers/media/usb/gspca/etoms.c
+++ b/drivers/media/usb/gspca/etoms.c
@@ -659,24 +659,37 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
 			u8 *data,			/* isoc packet */
 			int len)			/* iso packet length */
 {
 	int seqframe;
+	int datalen;

+	if (len < 2)			/* need the 2-byte packet header */
+		return;
 	seqframe = data[0] & 0x3f;
-	len = (int) (((data[0] & 0xc0) << 2) | data[1]);
+	datalen = (int)(((data[0] & 0xc0) << 2) | data[1]);
 	if (seqframe == 0x3f) {
 		gspca_dbg(gspca_dev, D_FRAM,
-			  "header packet found datalength %d !!\n", len);
+			  "header packet found datalength %d !!\n", datalen);
+		if (len < 30)
+			return;
 		gspca_dbg(gspca_dev, D_FRAM, "G %d R %d G %d B %d",
 			  data[2], data[3], data[4], data[5]);
 		data += 30;
-		/* don't change datalength as the chips provided it */
+		len -= 30;
+		/* the chip-provided length must not exceed the packet */
+		if (datalen > len)
+			datalen = len;
 		gspca_frame_add(gspca_dev, LAST_PACKET, NULL, 0);
-		gspca_frame_add(gspca_dev, FIRST_PACKET, data, len);
+		gspca_frame_add(gspca_dev, FIRST_PACKET, data, datalen);
 		return;
 	}
-	if (len) {
+	if (datalen) {
+		if (len < 8)
+			return;
 		data += 8;
-		gspca_frame_add(gspca_dev, INTER_PACKET, data, len);
+		len -= 8;
+		if (datalen > len)
+			datalen = len;
+		gspca_frame_add(gspca_dev, INTER_PACKET, data, datalen);
 	} else {			/* Drop Packet */
 		gspca_dev->last_packet_type = DISCARD_PACKET;
 	}