Skip to contentSkip to conversation

media: fix USB descriptor issues

PATCH In review Johan Hovold 2 participants 12 messages Jan 3, 2020 – Mar 12, 2020 1 message not in the archive

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

cover media: fix USB descriptor issues

This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.

Johan

Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media: xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check

 drivers/media/rc/iguanair.c                   |  2 +-
 drivers/media/usb/b2c2/flexcop-usb.c          |  6 +++---
 drivers/media/usb/dvb-usb/dib0700_core.c      |  4 ++--
 drivers/media/usb/gspca/ov519.c               | 10 ++++++++++
 drivers/media/usb/gspca/stv06xx/stv06xx.c     | 19 ++++++++++++++++++-
 .../media/usb/gspca/stv06xx/stv06xx_pb0100.c  |  4 ++++
 drivers/media/usb/gspca/xirlink_cit.c         | 18 +++++++++++++++++-
 7 files changed, 55 insertions(+), 8 deletions(-)
Signature · 1 line
2.24.1
#1 lore raw

PATCH 1/6 media: flexcop-usb: fix endpoint sanity check

A recent commit added an endpoint sanity check to address a NULL-pointer dereference on probe. Unfortunately the check was done on the current altsetting which was later changed.

Fix this by moving the sanity check to after the altsetting is changed.

Fixes 1b976fc6d684 ("media: b2c2-flexcop-usb: add sanity checking")
Cc Oliver Neukum <[email protected]>
Cc stable <[email protected]>
---
 drivers/media/usb/b2c2/flexcop-usb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/media/usb/b2c2/flexcop-usb.c +3 -3
index 039963a7765b..198ddfb8d2b1 100644
@@ -511,6 +511,9 @@ static int flexcop_usb_init(struct flexcop_usb *fc_usb)
 		return ret;
 	}
 
+	if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	switch (fc_usb->udev->speed) {
 	case USB_SPEED_LOW:
 		err("cannot handle USB speed because it is too slow.");
@@ -544,9 +547,6 @@ static int flexcop_usb_probe(struct usb_interface *intf,
 	struct flexcop_device *fc = NULL;
 	int ret;
 
-	if (intf->cur_altsetting->desc.bNumEndpoints < 1)
-		return -ENODEV;
-
 	if ((fc = flexcop_device_kmalloc(sizeof(struct flexcop_usb))) == NULL) {
 		err("out of memory\n");
 		return -ENOMEM;
Signature · 1 line
2.24.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw

Re: [PATCH 1/6] media: flexcop-usb: fix endpoint sanity check

Quoted 18 lines · Sasha Levin

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag, fixing commit: 1b976fc6d684 ("media: b2c2-flexcop-usb: add sanity checking").

The bot has tested the following trees: v5.4.7, v5.3.18, v4.19.92, v4.14.161, v4.9.207, v4.4.207.

v5.4.7: Build OK!
v5.3.18: Failed to apply! Possible dependencies:
649cd16c438f ("media: flexcop-usb: fix NULL-ptr deref in flexcop_usb_transfer_init()")

v4.19.92: Build OK!
v4.14.161: Build OK!
v4.9.207: Failed to apply! Possible dependencies:
649cd16c438f ("media: flexcop-usb: fix NULL-ptr deref in flexcop_usb_transfer_init()")

Quoted 7 lines

v4.4.207: Failed to apply! Possible dependencies:
649cd16c438f ("media: flexcop-usb: fix NULL-ptr deref in flexcop_usb_transfer_init()")

NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

Just fix the trivial context change when backporting.

Johan

#4 lore raw

PATCH 2/6 media: ov519: add missing endpoint sanity checks

In reply to Johan Hovold

Make sure to check that we have at least one endpoint before accessing the endpoint array to avoid dereferencing a NULL-pointer on stream start.

Note that these sanity checks are not redundant as the driver is mixing looking up altsettings by index and by number, which need not coincide.

Fixes 1876bb923c98 ("V4L/DVB (12079): gspca_ov519: add support for the ov511 bridge")
Fixes b282d87332f5 ("V4L/DVB (12080): gspca_ov519: Fix ov518+ with OV7620AE (Trust spacecam 320)")
Cc stable <[email protected]> # 2.6.31
Cc Hans de Goede <[email protected]>
---
 drivers/media/usb/gspca/ov519.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/media/usb/gspca/ov519.c +10 -0
index f417dfc0b872..0afe70a3f9a2 100644
@@ -3477,6 +3477,11 @@ static void ov511_mode_init_regs(struct sd *sd)
 		return;
 	}
 
+	if (alt->desc.bNumEndpoints < 1) {
+		sd->gspca_dev.usb_err = -ENODEV;
+		return;
+	}
+
 	packet_size = le16_to_cpu(alt->endpoint[0].desc.wMaxPacketSize);
 	reg_w(sd, R51x_FIFO_PSIZE, packet_size >> 5);
 
@@ -3603,6 +3608,11 @@ static void ov518_mode_init_regs(struct sd *sd)
 		return;
 	}
 
+	if (alt->desc.bNumEndpoints < 1) {
+		sd->gspca_dev.usb_err = -ENODEV;
+		return;
+	}
+
 	packet_size = le16_to_cpu(alt->endpoint[0].desc.wMaxPacketSize);
 	ov518_reg_w32(sd, R51x_FIFO_PSIZE, packet_size & ~7, 2);
 
Signature · 1 line
2.24.1
#5 lore raw

PATCH 3/6 media: stv06xx: add missing descriptor sanity checks

In reply to Johan Hovold

Make sure to check that we have two alternate settings and at least one endpoint before accessing the second altsetting structure and dereferencing the endpoint arrays.

This specifically avoids dereferencing NULL-pointers or corrupting memory when a device does not have the expected descriptors.

Note that the sanity checks in stv06xx_start() and pb0100_start() are not redundant as the driver is mixing looking up altsettings by index and by number, which may not coincide.

Fixes 8668d504d72c ("V4L/DVB (12082): gspca_stv06xx: Add support for st6422 bridge and sensor")
Fixes c0b33bdc5b8d ("[media] gspca-stv06xx: support bandwidth changing")
Cc stable <[email protected]> # 2.6.31
Cc Hans de Goede <[email protected]>
---
 drivers/media/usb/gspca/stv06xx/stv06xx.c     | 19 ++++++++++++++++++-
 .../media/usb/gspca/stv06xx/stv06xx_pb0100.c  |  4 ++++
 2 files changed, 22 insertions(+), 1 deletion(-)
drivers/media/usb/gspca/stv06xx/stv06xx.c +18 -1
index 79653d409951..95673fc0a99c 100644
@@ -282,6 +282,9 @@ static int stv06xx_start(struct gspca_dev *gspca_dev)
 		return -EIO;
 	}
 
+	if (alt->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	packet_size = le16_to_cpu(alt->endpoint[0].desc.wMaxPacketSize);
 	err = stv06xx_write_bridge(sd, STV_ISO_SIZE_L, packet_size);
 	if (err < 0)
@@ -306,11 +309,21 @@ static int stv06xx_start(struct gspca_dev *gspca_dev)
 
 static int stv06xx_isoc_init(struct gspca_dev *gspca_dev)
 {
+	struct usb_interface_cache *intfc;
 	struct usb_host_interface *alt;
 	struct sd *sd = (struct sd *) gspca_dev;
 
+	intfc = gspca_dev->dev->actconfig->intf_cache[0];
+
+	if (intfc->num_altsetting < 2)
+		return -ENODEV;
+
+	alt = &intfc->altsetting[1];
+
+	if (alt->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	/* Start isoc bandwidth "negotiation" at max isoc bandwidth */
-	alt = &gspca_dev->dev->actconfig->intf_cache[0]->altsetting[1];
 	alt->endpoint[0].desc.wMaxPacketSize =
 		cpu_to_le16(sd->sensor->max_packet_size[gspca_dev->curr_mode]);
 
@@ -323,6 +336,10 @@ static int stv06xx_isoc_nego(struct gspca_dev *gspca_dev)
 	struct usb_host_interface *alt;
 	struct sd *sd = (struct sd *) gspca_dev;
 
+	/*
+	 * Existence of altsetting and endpoint was verified in
+	 * stv06xx_isoc_init()
+	 */
 	alt = &gspca_dev->dev->actconfig->intf_cache[0]->altsetting[1];
 	packet_size = le16_to_cpu(alt->endpoint[0].desc.wMaxPacketSize);
 	min_packet_size = sd->sensor->min_packet_size[gspca_dev->curr_mode];
drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c +4 -0
index 6d1007715ff7..ae382b3b5f7f 100644
@@ -185,6 +185,10 @@ static int pb0100_start(struct sd *sd)
 	alt = usb_altnum_to_altsetting(intf, sd->gspca_dev.alt);
 	if (!alt)
 		return -ENODEV;
+
+	if (alt->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	packet_size = le16_to_cpu(alt->endpoint[0].desc.wMaxPacketSize);
 
 	/* If we don't have enough bandwidth use a lower framerate */
Signature · 1 line
2.24.1
#6 lore raw

PATCH 4/6 media: xirlink_cit: add missing descriptor sanity checks

In reply to Johan Hovold

Make sure to check that we have two alternate settings and at least one endpoint before accessing the second altsetting structure and dereferencing the endpoint arrays.

This specifically avoids dereferencing NULL-pointers or corrupting memory when a device does not have the expected descriptors.

Note that the sanity check in cit_get_packet_size() is not redundant as the driver is mixing looking up altsettings by index and by number, which may not coincide.

Fixes 659fefa0eb17 ("V4L/DVB: gspca_xirlink_cit: Add support for camera with a bcd version of 0.01")
Fixes 59f8b0bf3c12 ("V4L/DVB: gspca_xirlink_cit: support bandwidth changing for devices with 1 alt setting")
Cc stable <[email protected]> # 2.6.37
Cc Hans de Goede <[email protected]>
---
 drivers/media/usb/gspca/xirlink_cit.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)
drivers/media/usb/gspca/xirlink_cit.c +17 -1
index 934a90bd78c2..c579b100f066 100644
@@ -1442,6 +1442,9 @@ static int cit_get_packet_size(struct gspca_dev *gspca_dev)
 		return -EIO;
 	}
 
+	if (alt->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	return le16_to_cpu(alt->endpoint[0].desc.wMaxPacketSize);
 }
 
@@ -2626,6 +2629,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 
 static int sd_isoc_init(struct gspca_dev *gspca_dev)
 {
+	struct usb_interface_cache *intfc;
 	struct usb_host_interface *alt;
 	int max_packet_size;
 
@@ -2641,8 +2645,17 @@ static int sd_isoc_init(struct gspca_dev *gspca_dev)
 		break;
 	}
 
+	intfc = gspca_dev->dev->actconfig->intf_cache[0];
+
+	if (intfc->num_altsetting < 2)
+		return -ENODEV;
+
+	alt = &intfc->altsetting[1];
+
+	if (alt->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	/* Start isoc bandwidth "negotiation" at max isoc bandwidth */
-	alt = &gspca_dev->dev->actconfig->intf_cache[0]->altsetting[1];
 	alt->endpoint[0].desc.wMaxPacketSize = cpu_to_le16(max_packet_size);
 
 	return 0;
@@ -2665,6 +2678,9 @@ static int sd_isoc_nego(struct gspca_dev *gspca_dev)
 		break;
 	}
 
+	/*
+	 * Existence of altsetting and endpoint was verified in sd_isoc_init()
+	 */
 	alt = &gspca_dev->dev->actconfig->intf_cache[0]->altsetting[1];
 	packet_size = le16_to_cpu(alt->endpoint[0].desc.wMaxPacketSize);
 	if (packet_size <= min_packet_size)
Signature · 1 line
2.24.1
#7 lore raw

PATCH 5/6 media: dib0700: fix rc endpoint lookup

In reply to Johan Hovold

Make sure to use the current alternate setting when verifying the interface descriptors to avoid submitting an URB to an invalid endpoint.

Failing to do so could cause the driver to misbehave or trigger a WARN() in usb_submit_urb() that kernels with panic_on_warn set would choke on.

Fixes c4018fa2e4c0 ("[media] dib0700: fix RC support on Hauppauge Nova-TD")
Cc stable <[email protected]> # 3.16
---
 drivers/media/usb/dvb-usb/dib0700_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/usb/dvb-usb/dib0700_core.c +2 -2
index e53c58ab6488..ef62dd6c5ae4 100644
@@ -818,7 +818,7 @@ int dib0700_rc_setup(struct dvb_usb_device *d, struct usb_interface *intf)
 
 	/* Starting in firmware 1.20, the RC info is provided on a bulk pipe */
 
-	if (intf->altsetting[0].desc.bNumEndpoints < rc_ep + 1)
+	if (intf->cur_altsetting->desc.bNumEndpoints < rc_ep + 1)
 		return -ENODEV;
 
 	purb = usb_alloc_urb(0, GFP_KERNEL);
@@ -838,7 +838,7 @@ int dib0700_rc_setup(struct dvb_usb_device *d, struct usb_interface *intf)
 	 * Some devices like the Hauppauge NovaTD model 52009 use an interrupt
 	 * endpoint, while others use a bulk one.
 	 */
-	e = &intf->altsetting[0].endpoint[rc_ep].desc;
+	e = &intf->cur_altsetting->endpoint[rc_ep].desc;
 	if (usb_endpoint_dir_in(e)) {
 		if (usb_endpoint_xfer_bulk(e)) {
 			pipe = usb_rcvbulkpipe(d->udev, rc_ep);
Signature · 1 line
2.24.1
#8 lore raw

PATCH 6/6 media: iguanair: fix endpoint sanity check

In reply to Johan Hovold

Make sure to use the current alternate setting, which need not be the first one by index, when verifying the endpoint descriptors and initialising the URBs.

Failing to do so could cause the driver to misbehave or trigger a WARN() in usb_submit_urb() that kernels with panic_on_warn set would choke on.

Fixes 26ff63137c45 ("[media] Add support for the IguanaWorks USB IR Transceiver")
Fixes ab1cbdf159be ("media: iguanair: add sanity checks")
Cc stable <[email protected]> # 3.6
Cc Sean Young <[email protected]>
Cc Oliver Neukum <[email protected]>
---
 drivers/media/rc/iguanair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/rc/iguanair.c +1 -1
index 872d6441e512..a7deca1fefb7 100644
@@ -413,7 +413,7 @@ static int iguanair_probe(struct usb_interface *intf,
 	int ret, pipein, pipeout;
 	struct usb_host_interface *idesc;
 
-	idesc = intf->altsetting;
+	idesc = intf->cur_altsetting;
 	if (idesc->desc.bNumEndpoints < 2)
 		return -ENODEV;
 
Signature · 1 line
2.24.1
#9 lore raw
In reply to Johan Hovold
Quoted 11 lines · Johan Hovold

This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.

Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media: xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check

Just sending a reminder about these as it seems only the last one has made into mainline (and stable) yet.

Johan

#10 lore raw
Quoted 13 lines · Johan Hovold
Quoted 11 lines · Johan Hovold

This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.

Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media: xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check

Quoted 2 lines

Just sending a reminder about these as it seems only the last one has made into mainline (and stable) yet.

Another month, another reminder. Three of the above patches still hasn't been applied.

Johan

#11 lore raw
Quoted 14 lines · Johan Hovold
Quoted 13 lines · Johan Hovold
Quoted 11 lines · Johan Hovold

This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.

Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media: xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check

Quoted 5 lines
Quoted 2 lines

Just sending a reminder about these as it seems only the last one has made into mainline (and stable) yet.

Another month, another reminder. Three of the above patches still hasn't been applied.

I've delegated these to me and will make a PR today/tomorrow.

Regards,

Hans

#12 lore raw
Quoted 15 lines · Hans Verkuil
Quoted 14 lines · Johan Hovold
Quoted 13 lines · Johan Hovold
Quoted 11 lines · Johan Hovold

This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.

Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media: xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Just sending a reminder about these as it seems only the last one has made into mainline (and stable) yet.

Another month, another reminder. Three of the above patches still hasn't been applied.

I've delegated these to me and will make a PR today/tomorrow.

Thanks, Hans!

Johan