linux-kernel - [PATCH v3] can: mcba_usb: Fix memory leak when cancelling urb

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>] [day] [month] [year] [list]

Message-Id: <20210112042755.21421-1-minhquangbui99@gmail.com>
Date:   Tue, 12 Jan 2021 04:27:55 +0000
From:   Bui Quang Minh <minhquangbui99@...il.com>
To:     linux-usb@...r.kernel.org
Cc:     oneukum@...e.de, a.darwish@...utronix.de, bigeasy@...utronix.de,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        stern@...land.harvard.edu, syzkaller-bugs@...glegroups.com,
        tglx@...utronix.de, minhquangbui99@...il.com
Subject: [PATCH v3] can: mcba_usb: Fix memory leak when cancelling urb

In mcba_usb_read_bulk_callback(), when we don't resubmit or fails to
resubmit the urb, we need to deallocate the transfer buffer that is
allocated in mcba_usb_start().

On some architectures, usb_free_coherent() cannot be called in interrupt
context, create a usb_anchor to keep track of these urbs and free them
later.

Reported-by: syzbot+57281c762a3922e14dfe@...kaller.appspotmail.com
Signed-off-by: Bui Quang Minh <minhquangbui99@...il.com>
---
v1: add memory leak fix when not resubmitting urb
v2: add memory leak fix when failing to resubmit urb
v3: remove usb_free_coherent() calls in interrupt context

 drivers/net/can/usb/mcba_usb.c | 48 +++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index df54eb7d4b36..8025db484a22 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -77,6 +77,7 @@ struct mcba_priv {
 	struct net_device *netdev;
 	struct usb_anchor tx_submitted;
 	struct usb_anchor rx_submitted;
+	struct usb_anchor urbs_cleanup;
 	struct can_berr_counter bec;
 	bool usb_ka_first_pass;
 	bool can_ka_first_pass;
@@ -220,14 +221,17 @@ static void mcba_usb_write_bulk_callback(struct urb *urb)
 {
 	struct mcba_usb_ctx *ctx = urb->context;
 	struct net_device *netdev;
+	struct mcba_priv *priv;
 
 	WARN_ON(!ctx);
 
 	netdev = ctx->priv->netdev;
+	priv = netdev_priv(netdev);
 
-	/* free up our allocated buffer */
-	usb_free_coherent(urb->dev, urb->transfer_buffer_length,
-			  urb->transfer_buffer, urb->transfer_dma);
+	/* On some architectures, usb_free_coherent() cannot be called in
+	 * interrupt context, queue the urb for later cleanup
+	 */
+	usb_anchor_urb(urb, &priv->urbs_cleanup);
 
 	if (ctx->can) {
 		if (!netif_device_present(netdev))
@@ -291,8 +295,11 @@ static netdev_tx_t mcba_usb_xmit(struct mcba_priv *priv,
 
 failed:
 	usb_unanchor_urb(urb);
-	usb_free_coherent(priv->udev, MCBA_USB_TX_BUFF_SIZE, buf,
-			  urb->transfer_dma);
+
+	/* On some architectures, usb_free_coherent() cannot be called in
+	 * interrupt context, queue the urb for later cleanup
+	 */
+	usb_anchor_urb(urb, &priv->urbs_cleanup);
 
 	if (err == -ENODEV)
 		netif_device_detach(priv->netdev);
@@ -584,7 +591,7 @@ static void mcba_usb_read_bulk_callback(struct urb *urb)
 	case -EPIPE:
 	case -EPROTO:
 	case -ESHUTDOWN:
-		return;
+		goto free;
 
 	default:
 		netdev_info(netdev, "Rx URB aborted (%d)\n", urb->status);
@@ -615,11 +622,20 @@ static void mcba_usb_read_bulk_callback(struct urb *urb)
 
 	retval = usb_submit_urb(urb, GFP_ATOMIC);
 
-	if (retval == -ENODEV)
-		netif_device_detach(netdev);
-	else if (retval)
+	if (retval < 0) {
 		netdev_err(netdev, "failed resubmitting read bulk urb: %d\n",
 			   retval);
+		if (retval == -ENODEV)
+			netif_device_detach(netdev);
+		goto free;
+	}
+
+	return;
+free:
+	/* On some architectures, usb_free_coherent() cannot be called in
+	 * interrupt context, queue the urb for later cleanup
+	 */
+	usb_anchor_urb(urb, &priv->urbs_cleanup);
 }
 
 /* Start USB device */
@@ -706,6 +722,17 @@ static int mcba_usb_open(struct net_device *netdev)
 	return 0;
 }
 
+static void mcba_urb_cleanup(struct mcba_priv *priv)
+{
+	struct urb *urb;
+
+	while ((urb = usb_get_from_anchor(&priv->urbs_cleanup))) {
+		usb_free_coherent(urb->dev, urb->transfer_buffer_length,
+				  urb->transfer_buffer, urb->transfer_dma);
+		usb_free_urb(urb);
+	}
+}
+
 static void mcba_urb_unlink(struct mcba_priv *priv)
 {
 	usb_kill_anchored_urbs(&priv->rx_submitted);
@@ -723,6 +750,7 @@ static int mcba_usb_close(struct net_device *netdev)
 
 	/* Stop polling */
 	mcba_urb_unlink(priv);
+	mcba_urb_cleanup(priv);
 
 	close_candev(netdev);
 	can_led_event(netdev, CAN_LED_EVENT_STOP);
@@ -812,6 +840,7 @@ static int mcba_usb_probe(struct usb_interface *intf,
 
 	init_usb_anchor(&priv->rx_submitted);
 	init_usb_anchor(&priv->tx_submitted);
+	init_usb_anchor(&priv->urbs_cleanup);
 
 	usb_set_intfdata(intf, priv);
 
@@ -877,6 +906,7 @@ static void mcba_usb_disconnect(struct usb_interface *intf)
 
 	unregister_candev(priv->netdev);
 	mcba_urb_unlink(priv);
+	mcba_urb_cleanup(priv);
 	free_candev(priv->netdev);
 }
 
-- 
2.17.1