Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak
@ 2020-07-28 12:10 Johan Hovold
  2020-07-28 12:10 ` [PATCH net 1/3] net: lan78xx: add missing endpoint sanity check Johan Hovold
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Johan Hovold @ 2020-07-28 12:10 UTC (permalink / raw)
  To: Woojung Huh
  Cc: Microchip Linux Driver Support, David S. Miller, Jakub Kicinski,
	netdev, linux-usb, linux-kernel, Johan Hovold

The first two patches fix a NULL-pointer dereference at probe that can
be triggered by a malicious device and a small transfer-buffer memory
leak, respectively.

For another subsystem I would have marked them:

	Cc: stable@vger.kernel.org	# 4.3

The third one replaces the driver's current broken endpoint lookup
helper, which could end up accepting incomplete interfaces and whose
results weren't even useeren
Johan


Johan Hovold (3):
  net: lan78xx: add missing endpoint sanity check
  net: lan78xx: fix transfer-buffer memory leak
  net: lan78xx: replace bogus endpoint lookup

 drivers/net/usb/lan78xx.c | 113 +++++++++++---------------------------
 1 file changed, 31 insertions(+), 82 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH net 1/3] net: lan78xx: add missing endpoint sanity check
  2020-07-28 12:10 [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak Johan Hovold
@ 2020-07-28 12:10 ` Johan Hovold
  2020-07-28 12:10 ` [PATCH net 2/3] net: lan78xx: fix transfer-buffer memory leak Johan Hovold
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Johan Hovold @ 2020-07-28 12:10 UTC (permalink / raw)
  To: Woojung Huh
  Cc: Microchip Linux Driver Support, David S. Miller, Jakub Kicinski,
	netdev, linux-usb, linux-kernel, Johan Hovold,
	Woojung . Huh @ microchip . com

Add the missing endpoint sanity check to prevent a NULL-pointer
dereference should a malicious device lack the expected endpoints.

Note that the driver has a broken endpoint-lookup helper,
lan78xx_get_endpoints(), which can end up accepting interfaces in an
altsetting without endpoints as long as *some* altsetting has a bulk-in
and a bulk-out endpoint.

Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
Cc: Woojung.Huh@microchip.com <Woojung.Huh@microchip.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/usb/lan78xx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index eccbf4cd7149..d7162690e3f3 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3759,6 +3759,11 @@ static int lan78xx_probe(struct usb_interface *intf,
 	netdev->max_mtu = MAX_SINGLE_PACKET_SIZE;
 	netif_set_gso_max_size(netdev, MAX_SINGLE_PACKET_SIZE - MAX_HEADER);
 
+	if (intf->cur_altsetting->desc.bNumEndpoints < 3) {
+		ret = -ENODEV;
+		goto out3;
+	}
+
 	dev->ep_blkin = (intf->cur_altsetting)->endpoint + 0;
 	dev->ep_blkout = (intf->cur_altsetting)->endpoint + 1;
 	dev->ep_intr = (intf->cur_altsetting)->endpoint + 2;
-- 
2.26.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH net 2/3] net: lan78xx: fix transfer-buffer memory leak
  2020-07-28 12:10 [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak Johan Hovold
  2020-07-28 12:10 ` [PATCH net 1/3] net: lan78xx: add missing endpoint sanity check Johan Hovold
@ 2020-07-28 12:10 ` Johan Hovold
  2020-07-28 12:10 ` [PATCH net 3/3] net: lan78xx: replace bogus endpoint lookup Johan Hovold
  2020-07-28 20:36 ` [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak David Miller
  3 siblings, 0 replies; 5+ messages in thread
From: Johan Hovold @ 2020-07-28 12:10 UTC (permalink / raw)
  To: Woojung Huh
  Cc: Microchip Linux Driver Support, David S. Miller, Jakub Kicinski,
	netdev, linux-usb, linux-kernel, Johan Hovold,
	Woojung . Huh @ microchip . com

The interrupt URB transfer-buffer was never freed on disconnect or after
probe errors.

Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
Cc: Woojung.Huh@microchip.com <Woojung.Huh@microchip.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/usb/lan78xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d7162690e3f3..ee062b27cfa7 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3788,6 +3788,7 @@ static int lan78xx_probe(struct usb_interface *intf,
 			usb_fill_int_urb(dev->urb_intr, dev->udev,
 					 dev->pipe_intr, buf, maxp,
 					 intr_complete, dev, period);
+			dev->urb_intr->transfer_flags |= URB_FREE_BUFFER;
 		}
 	}
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH net 3/3] net: lan78xx: replace bogus endpoint lookup
  2020-07-28 12:10 [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak Johan Hovold
  2020-07-28 12:10 ` [PATCH net 1/3] net: lan78xx: add missing endpoint sanity check Johan Hovold
  2020-07-28 12:10 ` [PATCH net 2/3] net: lan78xx: fix transfer-buffer memory leak Johan Hovold
@ 2020-07-28 12:10 ` Johan Hovold
  2020-07-28 20:36 ` [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak David Miller
  3 siblings, 0 replies; 5+ messages in thread
From: Johan Hovold @ 2020-07-28 12:10 UTC (permalink / raw)
  To: Woojung Huh
  Cc: Microchip Linux Driver Support, David S. Miller, Jakub Kicinski,
	netdev, linux-usb, linux-kernel, Johan Hovold

Drop the bogus endpoint-lookup helper which could end up accepting
interfaces based on endpoints belonging to unrelated altsettings.

Note that the returned bulk pipes and interrupt endpoint descriptor
were never actually used. Instead the bulk-endpoint numbers are
hardcoded to 1 and 2 (matching the specification), while the interrupt-
endpoint descriptor was assumed to be the third descriptor created by
USB core.

Try to bring some order to this by dropping the bogus lookup helper and
adding the missing endpoint sanity checks while keeping the interrupt-
descriptor assumption for now.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/usb/lan78xx.c | 117 ++++++++++----------------------------
 1 file changed, 30 insertions(+), 87 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index ee062b27cfa7..442507f25aad 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -377,10 +377,6 @@ struct lan78xx_net {
 	struct tasklet_struct	bh;
 	struct delayed_work	wq;
 
-	struct usb_host_endpoint *ep_blkin;
-	struct usb_host_endpoint *ep_blkout;
-	struct usb_host_endpoint *ep_intr;
-
 	int			msg_enable;
 
 	struct urb		*urb_intr;
@@ -2860,78 +2856,12 @@ lan78xx_start_xmit(struct sk_buff *skb, struct net_device *net)
 	return NETDEV_TX_OK;
 }
 
-static int
-lan78xx_get_endpoints(struct lan78xx_net *dev, struct usb_interface *intf)
-{
-	int tmp;
-	struct usb_host_interface *alt = NULL;
-	struct usb_host_endpoint *in = NULL, *out = NULL;
-	struct usb_host_endpoint *status = NULL;
-
-	for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
-		unsigned ep;
-
-		in = NULL;
-		out = NULL;
-		status = NULL;
-		alt = intf->altsetting + tmp;
-
-		for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
-			struct usb_host_endpoint *e;
-			int intr = 0;
-
-			e = alt->endpoint + ep;
-			switch (e->desc.bmAttributes) {
-			case USB_ENDPOINT_XFER_INT:
-				if (!usb_endpoint_dir_in(&e->desc))
-					continue;
-				intr = 1;
-				/* FALLTHROUGH */
-			case USB_ENDPOINT_XFER_BULK:
-				break;
-			default:
-				continue;
-			}
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!intr && !in)
-					in = e;
-				else if (intr && !status)
-					status = e;
-			} else {
-				if (!out)
-					out = e;
-			}
-		}
-		if (in && out)
-			break;
-	}
-	if (!alt || !in || !out)
-		return -EINVAL;
-
-	dev->pipe_in = usb_rcvbulkpipe(dev->udev,
-				       in->desc.bEndpointAddress &
-				       USB_ENDPOINT_NUMBER_MASK);
-	dev->pipe_out = usb_sndbulkpipe(dev->udev,
-					out->desc.bEndpointAddress &
-					USB_ENDPOINT_NUMBER_MASK);
-	dev->ep_intr = status;
-
-	return 0;
-}
-
 static int lan78xx_bind(struct lan78xx_net *dev, struct usb_interface *intf)
 {
 	struct lan78xx_priv *pdata = NULL;
 	int ret;
 	int i;
 
-	ret = lan78xx_get_endpoints(dev, intf);
-	if (ret) {
-		netdev_warn(dev->net, "lan78xx_get_endpoints failed: %d\n",
-			    ret);
-		return ret;
-	}
-
 	dev->data[0] = (unsigned long)kzalloc(sizeof(*pdata), GFP_KERNEL);
 
 	pdata = (struct lan78xx_priv *)(dev->data[0]);
@@ -3700,6 +3630,7 @@ static void lan78xx_stat_monitor(struct timer_list *t)
 static int lan78xx_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
+	struct usb_host_endpoint *ep_blkin, *ep_blkout, *ep_intr;
 	struct lan78xx_net *dev;
 	struct net_device *netdev;
 	struct usb_device *udev;
@@ -3748,6 +3679,34 @@ static int lan78xx_probe(struct usb_interface *intf,
 
 	mutex_init(&dev->stats.access_lock);
 
+	if (intf->cur_altsetting->desc.bNumEndpoints < 3) {
+		ret = -ENODEV;
+		goto out2;
+	}
+
+	dev->pipe_in = usb_rcvbulkpipe(udev, BULK_IN_PIPE);
+	ep_blkin = usb_pipe_endpoint(udev, dev->pipe_in);
+	if (!ep_blkin || !usb_endpoint_is_bulk_in(&ep_blkin->desc)) {
+		ret = -ENODEV;
+		goto out2;
+	}
+
+	dev->pipe_out = usb_sndbulkpipe(udev, BULK_OUT_PIPE);
+	ep_blkout = usb_pipe_endpoint(udev, dev->pipe_out);
+	if (!ep_blkout || !usb_endpoint_is_bulk_out(&ep_blkout->desc)) {
+		ret = -ENODEV;
+		goto out2;
+	}
+
+	ep_intr = &intf->cur_altsetting->endpoint[2];
+	if (!usb_endpoint_is_int_in(&ep_intr->desc)) {
+		ret = -ENODEV;
+		goto out2;
+	}
+
+	dev->pipe_intr = usb_rcvintpipe(dev->udev,
+					usb_endpoint_num(&ep_intr->desc));
+
 	ret = lan78xx_bind(dev, intf);
 	if (ret < 0)
 		goto out2;
@@ -3759,23 +3718,7 @@ static int lan78xx_probe(struct usb_interface *intf,
 	netdev->max_mtu = MAX_SINGLE_PACKET_SIZE;
 	netif_set_gso_max_size(netdev, MAX_SINGLE_PACKET_SIZE - MAX_HEADER);
 
-	if (intf->cur_altsetting->desc.bNumEndpoints < 3) {
-		ret = -ENODEV;
-		goto out3;
-	}
-
-	dev->ep_blkin = (intf->cur_altsetting)->endpoint + 0;
-	dev->ep_blkout = (intf->cur_altsetting)->endpoint + 1;
-	dev->ep_intr = (intf->cur_altsetting)->endpoint + 2;
-
-	dev->pipe_in = usb_rcvbulkpipe(udev, BULK_IN_PIPE);
-	dev->pipe_out = usb_sndbulkpipe(udev, BULK_OUT_PIPE);
-
-	dev->pipe_intr = usb_rcvintpipe(dev->udev,
-					dev->ep_intr->desc.bEndpointAddress &
-					USB_ENDPOINT_NUMBER_MASK);
-	period = dev->ep_intr->desc.bInterval;
-
+	period = ep_intr->desc.bInterval;
 	maxp = usb_maxpacket(dev->udev, dev->pipe_intr, 0);
 	buf = kmalloc(maxp, GFP_KERNEL);
 	if (buf) {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak
  2020-07-28 12:10 [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak Johan Hovold
                   ` (2 preceding siblings ...)
  2020-07-28 12:10 ` [PATCH net 3/3] net: lan78xx: replace bogus endpoint lookup Johan Hovold
@ 2020-07-28 20:36 ` David Miller
  3 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2020-07-28 20:36 UTC (permalink / raw)
  To: johan; +Cc: woojung.huh, UNGLinuxDriver, kuba, netdev, linux-usb, linux-kernel

From: Johan Hovold <johan@kernel.org>
Date: Tue, 28 Jul 2020 14:10:28 +0200

> The first two patches fix a NULL-pointer dereference at probe that can
> be triggered by a malicious device and a small transfer-buffer memory
> leak, respectively.
> 
> For another subsystem I would have marked them:
> 
> 	Cc: stable@vger.kernel.org	# 4.3
> 
> The third one replaces the driver's current broken endpoint lookup
> helper, which could end up accepting incomplete interfaces and whose
> results weren't even useeren

Series applied and queued up for -stable.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 12:10 [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak Johan Hovold
2020-07-28 12:10 ` [PATCH net 1/3] net: lan78xx: add missing endpoint sanity check Johan Hovold
2020-07-28 12:10 ` [PATCH net 2/3] net: lan78xx: fix transfer-buffer memory leak Johan Hovold
2020-07-28 12:10 ` [PATCH net 3/3] net: lan78xx: replace bogus endpoint lookup Johan Hovold
2020-07-28 20:36 ` [PATCH net 0/3] net: lan78xx: fix NULL deref and memory leak David Miller

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git