From patchwork Mon Nov 23 13:32:10 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Bj=C3=B8rn_Mork?= X-Patchwork-Id: 547528 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id C21E514030D for ; Tue, 24 Nov 2015 00:32:28 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; secure) header.d=mork.no header.i=@mork.no header.b=ijbgHUPj; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753071AbbKWNcY (ORCPT ); Mon, 23 Nov 2015 08:32:24 -0500 Received: from canardo.mork.no ([148.122.252.1]:43342 "EHLO canardo.mork.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751700AbbKWNcX (ORCPT ); Mon, 23 Nov 2015 08:32:23 -0500 Received: from nemi.mork.no (nemi.mork.no [IPv6:2001:4641:0:2:e8b:fdff:fe08:971]) (authenticated bits=0) by canardo.mork.no (8.14.4/8.14.4) with ESMTP id tANDWE37006472 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=OK); Mon, 23 Nov 2015 14:32:15 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mork.no; s=a; t=1448285536; bh=CrgqKbK/c+L73NdCy1UlqqFXQt3fS0eGxDzosawT8FE=; h=From:To:Cc:Date:Message-Id:From; b=ijbgHUPjgP6TPqUinAH/KPGElU9XRRfUcmK2HccPx5ShB1PCr2RGLMyDX1vVwNZPk k+3vFdtKBFP9w9D37twGABVumOkRpOLH/cXEQAL28iBlh3X2tsgwfHo8KcVwJGx7+c Qs+mPMECWPBgUNCw/gBWhWHom5u9Lc1UGPKogbxaQZAZRvDynZmaD/SNBUOSey2LmH FYXtRwbXRr/sj1vrG5MeLSO+J9eT/oGhL4NNTNrsKB9WTKaB0E3KLNIQxxmoVTLBx6 bBI3mq8bB4wlmdP8ykqoMjgWzRAnNULf65pCiSPYAguzNzg0G7zitD65U7uPvK3wum NeQeCCTAgzOvQ== Received: from bjorn by nemi.mork.no with local (Exim 4.84) (envelope-from ) id 1a0rE6-0001cy-S6; Mon, 23 Nov 2015 14:32:14 +0100 From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= To: netdev@vger.kernel.org Cc: linux-usb@vger.kernel.org, =?UTF-8?q?Bj=C3=B8rn=20Mork?= , Oliver Neukum Subject: [PATCH] net: cdc_ncm: fix NULL pointer deref in cdc_ncm_bind_common Date: Mon, 23 Nov 2015 14:32:10 +0100 Message-Id: <1448285530-6223-1-git-send-email-bjorn@mork.no> X-Mailer: git-send-email 2.1.4 MIME-Version: 1.0 X-Virus-Scanned: clamav-milter 0.98.7 at canardo X-Virus-Status: Clean Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 Fixes: 77b0a099674a ("cdc-ncm: use common parser") Signed-off-by: Bjørn Mork --- 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"); }