media: fix some potential buffer overruns

PATCH New Alyssa Milburn 1 participant 5 messages Apr 1, 2017 Media (V4L/DVB)

Share & tools

Alyssa Milburn

author

2017-04-01 17:36 UTC #0 lore raw

cover media: fix some potential buffer overruns

I don't own any of this hardware, so I can't test these patches; I'd appreciate it if someone with the hardware could do so, but in theory they shouldn't break anything.

Most of the patches below fix overruns which can be induced by local users, but only if they can read or write to i2c devices. The zr364xx patch is probably only needed against malicious hardware.

This is against mainline, not linux-media. Sorry. The dw2102 patch will not apply cleanly due to 606142af57dad981b78707234cfbd15f9f7b7125, which changed the relevant code (moving from stack to heap buffers), but backporting it seems silly.

I tried to make these patches as non-invasive as possible, and to stick with any existing error reporting style where present. I have more fixes planned, so any feedback on this approach would be appreciated.

Alyssa Milburn (4):
digitv: limit messages to buffer size
zr364xx: enforce minimum size when reading header
ttusb2: limit messages to buffer size
dw2102: limit messages to buffer size

 drivers/media/usb/dvb-usb/digitv.c  |  3 +++
 drivers/media/usb/dvb-usb/dw2102.c  | 54 +++++++++++++++++++++++++++++++++++++
 drivers/media/usb/dvb-usb/ttusb2.c  | 19 +++++++++++++
 drivers/media/usb/zr364xx/zr364xx.c |  8 ++++++
 4 files changed, 84 insertions(+)

Signature · 1 line

2.11.0

Alyssa Milburn

author

2017-04-01 17:39 UTC #1 lore raw

PATCH 1/4 digitv: limit messages to buffer size

Return an error rather than memcpy()ing beyond the end of the buffer. Internal callers use appropriate sizes, but digitv_i2c_xfer may not.

Signed-off-by Alyssa Milburn <[email protected]>

---
 drivers/media/usb/dvb-usb/digitv.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/media/usb/dvb-usb/digitv.c +3 -0

index 4284f6984dc1..475a3c0cdee7 100644
@@ -33,6 +33,9 @@ static int digitv_ctrl_msg(struct dvb_usb_device *d,
 
 	wo = (rbuf == NULL || rlen == 0); /* write-only */
 
+	if (wlen > 4 || rlen > 4)
+		return -EIO;
+
 	memset(st->sndbuf, 0, 7);
 	memset(st->rcvbuf, 0, 7);

Signature · 1 line

2.11.0

Alyssa Milburn

author

2017-04-01 17:39 UTC #2 lore raw

PATCH 2/4 zr364xx: enforce minimum size when reading header

In reply to Alyssa Milburn

This code copies actual_length-128 bytes from the header, which will underflow if the received buffer is too small.

Signed-off-by Alyssa Milburn <[email protected]>

---
 drivers/media/usb/zr364xx/zr364xx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/media/usb/zr364xx/zr364xx.c +8 -0

index f2d6fc03dda0..efdcd5bd6a4c 100644
@@ -600,6 +600,14 @@ static int zr364xx_read_video_callback(struct zr364xx_camera *cam,
 	ptr = pdest = frm->lpvbits;
 
 	if (frm->ulState == ZR364XX_READ_IDLE) {
+		if (purb->actual_length < 128) {
+			/* header incomplete */
+			dev_info(&cam->udev->dev,
+				 "%s: buffer (%d bytes) too small to hold jpeg header. Discarding.\n",
+				 __func__, purb->actual_length);
+			return -EINVAL;
+		}
+
 		frm->ulState = ZR364XX_READ_FRAME;
 		frm->cur_size = 0;

Signature · 1 line

2.11.0

Alyssa Milburn

author

2017-04-01 17:39 UTC #3 lore raw

PATCH 3/4 ttusb2: limit messages to buffer size

In reply to Alyssa Milburn

Otherwise ttusb2_i2c_xfer can read or write beyond the end of static and heap buffers.

Signed-off-by Alyssa Milburn <[email protected]>

---
 drivers/media/usb/dvb-usb/ttusb2.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/media/usb/dvb-usb/ttusb2.c +19 -0

index ecc207fbaf3c..9e0d6a4166d2 100644
@@ -78,6 +78,9 @@ static int ttusb2_msg(struct dvb_usb_device *d, u8 cmd,
 	u8 *s, *r = NULL;
 	int ret = 0;
 
+	if (4 + rlen > 64)
+		return -EIO;
+
 	s = kzalloc(wlen+4, GFP_KERNEL);
 	if (!s)
 		return -ENOMEM;

@@ -381,6 +384,22 @@ static int ttusb2_i2c_xfer(struct i2c_adapter *adap,struct i2c_msg msg[],int num
 		write_read = i+1 < num && (msg[i+1].flags & I2C_M_RD);
 		read = msg[i].flags & I2C_M_RD;
 
+		if (3 + msg[i].len > sizeof(obuf)) {
+			err("i2c wr len=%d too high", msg[i].len);
+			break;
+		}
+		if (write_read) {
+			if (3 + msg[i+1].len > sizeof(ibuf)) {
+				err("i2c rd len=%d too high", msg[i+1].len);
+				break;
+			}
+		} else if (read) {
+			if (3 + msg[i].len > sizeof(ibuf)) {
+				err("i2c rd len=%d too high", msg[i].len);
+				break;
+			}
+		}
+
 		obuf[0] = (msg[i].addr << 1) | (write_read | read);
 		if (read)
 			obuf[1] = 0;

Signature · 1 line

2.11.0

Alyssa Milburn

author

2017-04-01 17:39 UTC #4 lore raw

PATCH 4/4 dw2102: limit messages to buffer size

In reply to Alyssa Milburn

Otherwise the i2c transfer functions can read or write beyond the end of stack or heap buffers.

Signed-off-by Alyssa Milburn <[email protected]>

---
 drivers/media/usb/dvb-usb/dw2102.c | 54 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

drivers/media/usb/dvb-usb/dw2102.c +54 -0

index 4f42d57f81d9..6e654e5026dd 100644
@@ -204,6 +204,20 @@ static int dw2102_serit_i2c_transfer(struct i2c_adapter *adap,
 
 	switch (num) {
 	case 2:
+		if (msg[0].len != 1) {
+			warn("i2c rd: len=%d is not 1!\n",
+			     msg[0].len);
+			num = -EOPNOTSUPP;
+			break;
+		}
+
+		if (2 + msg[1].len > sizeof(buf6)) {
+			warn("i2c rd: len=%d is too big!\n",
+			     msg[1].len);
+			num = -EOPNOTSUPP;
+			break;
+		}
+
 		/* read si2109 register by number */
 		buf6[0] = msg[0].addr << 1;
 		buf6[1] = msg[0].len;

@@ -219,6 +233,13 @@ static int dw2102_serit_i2c_transfer(struct i2c_adapter *adap,
 	case 1:
 		switch (msg[0].addr) {
 		case 0x68:
+			if (2 + msg[0].len > sizeof(buf6)) {
+				warn("i2c wr: len=%d is too big!\n",
+				     msg[0].len);
+				num = -EOPNOTSUPP;
+				break;
+			}
+
 			/* write to si2109 register */
 			buf6[0] = msg[0].addr << 1;
 			buf6[1] = msg[0].len;

@@ -262,6 +283,13 @@ static int dw2102_earda_i2c_transfer(struct i2c_adapter *adap, struct i2c_msg ms
 		/* first write first register number */
 		u8 ibuf[MAX_XFER_SIZE], obuf[3];
 
+		if (2 + msg[0].len != sizeof(obuf)) {
+			warn("i2c rd: len=%d is not 1!\n",
+			     msg[0].len);
+			ret = -EOPNOTSUPP;
+			goto unlock;
+		}
+
 		if (2 + msg[1].len > sizeof(ibuf)) {
 			warn("i2c rd: len=%d is too big!\n",
 			     msg[1].len);

@@ -462,6 +490,12 @@ static int dw3101_i2c_transfer(struct i2c_adapter *adap, struct i2c_msg msg[],
 		/* first write first register number */
 		u8 ibuf[MAX_XFER_SIZE], obuf[3];
 
+		if (2 + msg[0].len != sizeof(obuf)) {
+			warn("i2c rd: len=%d is not 1!\n",
+			     msg[0].len);
+			ret = -EOPNOTSUPP;
+			goto unlock;
+		}
 		if (2 + msg[1].len > sizeof(ibuf)) {
 			warn("i2c rd: len=%d is too big!\n",
 			     msg[1].len);

@@ -696,6 +730,13 @@ static int su3000_i2c_transfer(struct i2c_adapter *adap, struct i2c_msg msg[],
 			msg[0].buf[0] = state->data[1];
 			break;
 		default:
+			if (3 + msg[0].len > sizeof(state->data)) {
+				warn("i2c wr: len=%d is too big!\n",
+				     msg[0].len);
+				num = -EOPNOTSUPP;
+				break;
+			}
+
 			/* always i2c write*/
 			state->data[0] = 0x08;
 			state->data[1] = msg[0].addr;

@@ -711,6 +752,19 @@ static int su3000_i2c_transfer(struct i2c_adapter *adap, struct i2c_msg msg[],
 		break;
 	case 2:
 		/* always i2c read */
+		if (4 + msg[0].len > sizeof(state->data)) {
+			warn("i2c rd: len=%d is too big!\n",
+			     msg[0].len);
+			num = -EOPNOTSUPP;
+			break;
+		}
+		if (1 + msg[1].len > sizeof(state->data)) {
+			warn("i2c rd: len=%d is too big!\n",
+			     msg[1].len);
+			num = -EOPNOTSUPP;
+			break;
+		}
+
 		state->data[0] = 0x09;
 		state->data[1] = msg[0].len;
 		state->data[2] = msg[1].len;

Signature · 1 line

2.11.0