[PATCH 0/6] media: fix USB descriptor issues

[PATCH 0/6] media: fix USB descriptor issues

[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


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(-)

-- 
2.24.1

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

[Johan Hovold]

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 <oneukum@suse.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/usb/b2c2/flexcop-usb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
index 039963a7765b..198ddfb8d2b1 100644
--- a/drivers/media/usb/b2c2/flexcop-usb.c
+++ b/drivers/media/usb/b2c2/flexcop-usb.c
@@ -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;
-- 
2.24.1

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

[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 <stable@vger.kernel.org>     # 2.6.31
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/usb/gspca/ov519.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/usb/gspca/ov519.c b/drivers/media/usb/gspca/ov519.c
index f417dfc0b872..0afe70a3f9a2 100644
--- a/drivers/media/usb/gspca/ov519.c
+++ b/drivers/media/usb/gspca/ov519.c
@@ -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);
 
-- 
2.24.1

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

[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 <stable@vger.kernel.org>     # 2.6.31
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/usb/gspca/stv06xx/stv06xx.c     | 19 ++++++++++++++++++-
 .../media/usb/gspca/stv06xx/stv06xx_pb0100.c  |  4 ++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx.c b/drivers/media/usb/gspca/stv06xx/stv06xx.c
index 79653d409951..95673fc0a99c 100644
--- a/drivers/media/usb/gspca/stv06xx/stv06xx.c
+++ b/drivers/media/usb/gspca/stv06xx/stv06xx.c
@@ -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];
diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c b/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c
index 6d1007715ff7..ae382b3b5f7f 100644
--- a/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c
+++ b/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c
@@ -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 */
-- 
2.24.1

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

[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 <stable@vger.kernel.org>     # 2.6.37
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/usb/gspca/xirlink_cit.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/gspca/xirlink_cit.c b/drivers/media/usb/gspca/xirlink_cit.c
index 934a90bd78c2..c579b100f066 100644
--- a/drivers/media/usb/gspca/xirlink_cit.c
+++ b/drivers/media/usb/gspca/xirlink_cit.c
@@ -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)
-- 
2.24.1

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

[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 <stable@vger.kernel.org>     # 3.16
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/usb/dvb-usb/dib0700_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index e53c58ab6488..ef62dd6c5ae4 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -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);
-- 
2.24.1

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

[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 <stable@vger.kernel.org>     # 3.6
Cc: Sean Young <sean@mess.org>
Cc: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/rc/iguanair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/iguanair.c b/drivers/media/rc/iguanair.c
index 872d6441e512..a7deca1fefb7 100644
--- a/drivers/media/rc/iguanair.c
+++ b/drivers/media/rc/iguanair.c
@@ -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;
 
-- 
2.24.1

Re: [PATCH 0/6] media: fix USB descriptor issues

[Johan Hovold]

On Fri, Jan 03, 2020 at 05:35:07PM +0100, Johan Hovold wrote:
> 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

Re: [PATCH 0/6] media: fix USB descriptor issues

[Johan Hovold]

On Fri, Feb 14, 2020 at 09:02:54AM +0100, Johan Hovold wrote:
> On Fri, Jan 03, 2020 at 05:35:07PM +0100, Johan Hovold wrote:
> > 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.

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

Johan

Re: [PATCH 0/6] media: fix USB descriptor issues

[Hans Verkuil]

On 3/12/20 12:23 PM, Johan Hovold wrote:
> On Fri, Feb 14, 2020 at 09:02:54AM +0100, Johan Hovold wrote:
>> On Fri, Jan 03, 2020 at 05:35:07PM +0100, Johan Hovold wrote:
>>> 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.
> 
> 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

Re: [PATCH 0/6] media: fix USB descriptor issues

[Johan Hovold]

On Thu, Mar 12, 2020 at 12:26:35PM +0100, Hans Verkuil wrote:
> On 3/12/20 12:23 PM, Johan Hovold wrote:
> > On Fri, Feb 14, 2020 at 09:02:54AM +0100, Johan Hovold wrote:
> >> On Fri, Jan 03, 2020 at 05:35:07PM +0100, Johan Hovold wrote:
> >>> 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.
> > 
> > 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