* [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 related [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 related [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 related [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, other threads:[~2020-07-28 20:36 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).