USB: fix NULL-deref and alternate settings bugs

PATCH New Johan Hovold 1 participant 5 messages Dec 10, 2019

Subsystems (5)

Stable branch USB USB serial ATM +1 more subsystems

lore

Share & tools

lore

Johan Hovold

author maintainer

2019-12-10 11:26 UTC #0 lore raw

cover USB: fix NULL-deref and alternate settings bugs

I stumbled over a couple potential NULL-pointer dereferences due to drivers using the first altsetting instead of the current one when doing descriptor sanity checks.

Turns out we have a quite a few drivers getting this wrong even if this would mostly be an issue on kernels with panic_on_warn set due to the WARN() in usb_submit_urb().

Since we've started backporting fixes for such warnings (e.g. as reported by syzbot), I've marked these for stable as well.

Johan

Johan Hovold (4):
USB: atm: ueagle-atm: add missing endpoint check
USB: adutux: fix interface sanity check
USB: idmouse: fix interface sanity checks
USB: serial: io_edgeport: fix epic endpoint lookup

 drivers/usb/atm/ueagle-atm.c     | 18 ++++++++++++------
 drivers/usb/misc/adutux.c        |  2 +-
 drivers/usb/misc/idmouse.c       |  2 +-
 drivers/usb/serial/io_edgeport.c | 10 ++++++----
 4 files changed, 20 insertions(+), 12 deletions(-)

Signature · 1 line

2.24.0

Johan Hovold

author maintainer

2019-12-10 11:26 UTC #1 lore raw

PATCH 1/4 USB: atm: ueagle-atm: add missing endpoint check

Make sure that the interrupt interface has an endpoint before trying to access its endpoint descriptors to avoid dereferencing a NULL pointer.

The driver binds to the interrupt interface with interface number 0, but must not assume that this interface or its current alternate setting are the first entries in the corresponding configuration arrays.

Fixes b72458a80c75 ("[PATCH] USB: Eagle and ADI 930 usb adsl modem driver")

Cc stable <[email protected]> # 2.6.16

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/atm/ueagle-atm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

drivers/usb/atm/ueagle-atm.c +12 -6

index 8b0ea8c70d73..635cf0466b59 100644
@@ -2124,10 +2124,11 @@ static void uea_intr(struct urb *urb)
 /*
  * Start the modem : init the data and start kernel thread
  */
-static int uea_boot(struct uea_softc *sc)
+static int uea_boot(struct uea_softc *sc, struct usb_interface *intf)
 {
-	int ret, size;
 	struct intr_pkt *intr;
+	int ret = -ENOMEM;
+	int size;
 
 	uea_enters(INS_TO_USBDEV(sc));

@@ -2152,6 +2153,11 @@ static int uea_boot(struct uea_softc *sc)
 	if (UEA_CHIP_VERSION(sc) == ADI930)
 		load_XILINX_firmware(sc);
 
+	if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+		ret = -ENODEV;
+		goto err0;
+	}
+
 	intr = kmalloc(size, GFP_KERNEL);
 	if (!intr)
 		goto err0;

@@ -2163,8 +2169,7 @@ static int uea_boot(struct uea_softc *sc)
 	usb_fill_int_urb(sc->urb_int, sc->usb_dev,
 			 usb_rcvintpipe(sc->usb_dev, UEA_INTR_PIPE),
 			 intr, size, uea_intr, sc,
-			 sc->usb_dev->actconfig->interface[0]->altsetting[0].
-			 endpoint[0].desc.bInterval);
+			 intf->cur_altsetting->endpoint[0].desc.bInterval);
 
 	ret = usb_submit_urb(sc->urb_int, GFP_KERNEL);
 	if (ret < 0) {

@@ -2179,6 +2184,7 @@ static int uea_boot(struct uea_softc *sc)
 	sc->kthread = kthread_create(uea_kthread, sc, "ueagle-atm");
 	if (IS_ERR(sc->kthread)) {
 		uea_err(INS_TO_USBDEV(sc), "failed to create thread\n");
+		ret = PTR_ERR(sc->kthread);
 		goto err2;
 	}

@@ -2193,7 +2199,7 @@ static int uea_boot(struct uea_softc *sc)
 	kfree(intr);
 err0:
 	uea_leaves(INS_TO_USBDEV(sc));
-	return -ENOMEM;
+	return ret;
 }
 
 /*

@@ -2548,7 +2554,7 @@ static int uea_bind(struct usbatm_data *usbatm, struct usb_interface *intf,
 		}
 	}
 
-	ret = uea_boot(sc);
+	ret = uea_boot(sc, intf);
 	if (ret < 0)
 		goto error;

Signature · 1 line

2.24.0

Johan Hovold

author maintainer

2019-12-10 11:26 UTC #2 lore raw

PATCH 2/4 USB: adutux: fix interface sanity check

In reply to Johan Hovold

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

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 03270634e242 ("USB: Add ADU support for Ontrak ADU devices")

Cc stable <[email protected]> # 2.6.19

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/misc/adutux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/misc/adutux.c +1 -1

index 6f5edb9fc61e..d8d157c4c271 100644
@@ -669,7 +669,7 @@ static int adu_probe(struct usb_interface *interface,
 	init_waitqueue_head(&dev->read_wait);
 	init_waitqueue_head(&dev->write_wait);
 
-	res = usb_find_common_endpoints_reverse(&interface->altsetting[0],
+	res = usb_find_common_endpoints_reverse(interface->cur_altsetting,
 			NULL, NULL,
 			&dev->interrupt_in_endpoint,
 			&dev->interrupt_out_endpoint);

Signature · 1 line

2.24.0

Johan Hovold

author maintainer

2019-12-10 11:26 UTC #3 lore raw

PATCH 3/4 USB: idmouse: fix interface sanity checks

In reply to Johan Hovold

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

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 1da177e4c3f4 ("Linux-2.6.12-rc2")

Cc stable <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/misc/idmouse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/misc/idmouse.c +1 -1

index 4afb5ddfd361..e9437a176518 100644
@@ -322,7 +322,7 @@ static int idmouse_probe(struct usb_interface *interface,
 	int result;
 
 	/* check if we have gotten the data or the hid interface */
-	iface_desc = &interface->altsetting[0];
+	iface_desc = interface->cur_altsetting;
 	if (iface_desc->desc.bInterfaceClass != 0x0A)
 		return -ENODEV;

Signature · 1 line

2.24.0

Johan Hovold

author maintainer

2019-12-10 11:26 UTC #4 lore raw

PATCH 4/4 USB: serial: io_edgeport: fix epic endpoint lookup

In reply to Johan Hovold

Make sure to use the current alternate setting when looking up the endpoints on epic devices to avoid binding to an invalid interface.

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 6e8cf7751f9f ("USB: add EPIC support to the io_edgeport driver")

Cc stable <[email protected]> # 2.6.21

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/io_edgeport.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/usb/serial/io_edgeport.c +6 -4

index 48a439298a68..9690a5f4b9d6 100644
@@ -2901,16 +2901,18 @@ static int edge_startup(struct usb_serial *serial)
 	response = 0;
 
 	if (edge_serial->is_epic) {
+		struct usb_host_interface *alt;
+
+		alt = serial->interface->cur_altsetting;
+
 		/* EPIC thing, set up our interrupt polling now and our read
 		 * urb, so that the device knows it really is connected. */
 		interrupt_in_found = bulk_in_found = bulk_out_found = false;
-		for (i = 0; i < serial->interface->altsetting[0]
-						.desc.bNumEndpoints; ++i) {
+		for (i = 0; i < alt->desc.bNumEndpoints; ++i) {
 			struct usb_endpoint_descriptor *endpoint;
 			int buffer_size;
 
-			endpoint = &serial->interface->altsetting[0].
-							endpoint[i].desc;
+			endpoint = &alt->endpoint[i].desc;
 			buffer_size = usb_endpoint_maxp(endpoint);
 			if (!interrupt_in_found &&
 			    (usb_endpoint_is_int_in(endpoint))) {

Signature · 1 line

2.24.0