netdev - [PATCH] net: cdc_ncm: fix NULL pointer deref in cdc_ncm_bind

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 Nov 2015 14:32:10 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	netdev@...r.kernel.org
Cc:	linux-usb@...r.kernel.org,
	Bjørn Mork <bjorn@...k.no>,
	Oliver Neukum <oneukum@...e.com>
Subject: [PATCH] net: cdc_ncm: fix NULL pointer deref in cdc_ncm_bind_common

Commit 77b0a099674a ("cdc-ncm: use common parser") added a dangerous
new trust in the CDC functional descriptors presented by the device,
unconditionally assuming that any device handled by the driver has
a CDC Union descriptor.

This descriptor is required by the NCM and MBIM specs, but crashing
on non-compliant devices is still unacceptable. Not only will that
allow malicious devices to crash the kernel, but in this case it is
also well known that there are non-compliant real devices on the
market - as shown by the comment accompanying the IAD workaround
in the same function.

The Sierra Wireless EM7305 is an example of such device, having
a CDC header and a CDC MBIM descriptor but no CDC Union:

    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber       12
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         2 Communications
      bInterfaceSubClass     14
      bInterfaceProtocol      0
      iInterface              0
      CDC Header:
        bcdCDC               1.10
      CDC MBIM:
        bcdMBIMVersion       1.00
        wMaxControlMessage   4096
        bNumberFilters       16
        bMaxFilterSize       128
        wMaxSegmentSize      4064
        bmNetworkCapabilities 0x20
          8-byte ntb input size
      Endpoint Descriptor:
	..

The conversion to a common parser also left the local cdc_union
variable untouched.  This caused the IAD workaround code to be applied
to all devices with an IAD descriptor, which was never intended.  Finish
the conversion by testing for hdr.usb_cdc_union_desc instead.

Cc: Oliver Neukum <oneukum@...e.com>
Fixes: 77b0a099674a ("cdc-ncm: use common parser")
Signed-off-by: Bjørn Mork <bjorn@...k.no>
---
 drivers/net/usb/cdc_ncm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index a187f08113ec..3b1ba8237768 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -691,7 +691,6 @@ static void cdc_ncm_free(struct cdc_ncm_ctx *ctx)
 
 int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting, int drvflags)
 {
-	const struct usb_cdc_union_desc *union_desc = NULL;
 	struct cdc_ncm_ctx *ctx;
 	struct usb_driver *driver;
 	u8 *buf;
@@ -725,15 +724,16 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
 	/* parse through descriptors associated with control interface */
 	cdc_parse_cdc_header(&hdr, intf, buf, len);
 
-	ctx->data = usb_ifnum_to_if(dev->udev,
-				    hdr.usb_cdc_union_desc->bSlaveInterface0);
+	if (hdr.usb_cdc_union_desc)
+		ctx->data = usb_ifnum_to_if(dev->udev,
+					    hdr.usb_cdc_union_desc->bSlaveInterface0);
 	ctx->ether_desc = hdr.usb_cdc_ether_desc;
 	ctx->func_desc = hdr.usb_cdc_ncm_desc;
 	ctx->mbim_desc = hdr.usb_cdc_mbim_desc;
 	ctx->mbim_extended_desc = hdr.usb_cdc_mbim_extended_desc;
 
 	/* some buggy devices have an IAD but no CDC Union */
-	if (!union_desc && intf->intf_assoc && intf->intf_assoc->bInterfaceCount == 2) {
+	if (!hdr.usb_cdc_union_desc && intf->intf_assoc && intf->intf_assoc->bInterfaceCount == 2) {
 		ctx->data = usb_ifnum_to_if(dev->udev, intf->cur_altsetting->desc.bInterfaceNumber + 1);
 		dev_dbg(&intf->dev, "CDC Union missing - got slave from IAD\n");
 	}
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html