Skip to content

Commit

Permalink
can: mcba_usb: fix memory leak in mcba_usb
Browse files Browse the repository at this point in the history
commit 91c0255 upstream.

Syzbot reported memory leak in SocketCAN driver for Microchip CAN BUS
Analyzer Tool. The problem was in unfreed usb_coherent.

In mcba_usb_start() 20 coherent buffers are allocated and there is
nothing, that frees them:

1) In callback function the urb is resubmitted and that's all
2) In disconnect function urbs are simply killed, but URB_FREE_BUFFER
   is not set (see mcba_usb_start) and this flag cannot be used with
   coherent buffers.

Fail log:
| [ 1354.053291][ T8413] mcba_usb 1-1:0.0 can0: device disconnected
| [ 1367.059384][ T8420] kmemleak: 20 new suspected memory leaks (see /sys/kernel/debug/kmem)

So, all allocated buffers should be freed with usb_free_coherent()
explicitly

NOTE:
The same pattern for allocating and freeing coherent buffers
is used in drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c

Fixes: 51f3baa ("can: mcba_usb: Add support for Microchip CAN BUS Analyzer")
Link: https://lore.kernel.org/r/20210609215833.30393-1-paskripkin@gmail.com
Cc: linux-stable <stable@vger.kernel.org>
Reported-and-tested-by: syzbot+57281c762a3922e14dfe@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
pskrgag authored and gregkh committed Jun 23, 2021
1 parent 509ab6b commit 6bd3d80
Showing 1 changed file with 15 additions and 2 deletions.
17 changes: 15 additions & 2 deletions drivers/net/can/usb/mcba_usb.c
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ struct mcba_priv {
bool can_ka_first_pass;
bool can_speed_check;
atomic_t free_ctx_cnt;
void *rxbuf[MCBA_MAX_RX_URBS];
dma_addr_t rxbuf_dma[MCBA_MAX_RX_URBS];
};

/* CAN frame */
Expand Down Expand Up @@ -633,6 +635,7 @@ static int mcba_usb_start(struct mcba_priv *priv)
for (i = 0; i < MCBA_MAX_RX_URBS; i++) {
struct urb *urb = NULL;
u8 *buf;
dma_addr_t buf_dma;

/* create a URB, and a buffer for it */
urb = usb_alloc_urb(0, GFP_KERNEL);
Expand All @@ -642,7 +645,7 @@ static int mcba_usb_start(struct mcba_priv *priv)
}

buf = usb_alloc_coherent(priv->udev, MCBA_USB_RX_BUFF_SIZE,
GFP_KERNEL, &urb->transfer_dma);
GFP_KERNEL, &buf_dma);
if (!buf) {
netdev_err(netdev, "No memory left for USB buffer\n");
usb_free_urb(urb);
Expand All @@ -661,11 +664,14 @@ static int mcba_usb_start(struct mcba_priv *priv)
if (err) {
usb_unanchor_urb(urb);
usb_free_coherent(priv->udev, MCBA_USB_RX_BUFF_SIZE,
buf, urb->transfer_dma);
buf, buf_dma);
usb_free_urb(urb);
break;
}

priv->rxbuf[i] = buf;
priv->rxbuf_dma[i] = buf_dma;

/* Drop reference, USB core will take care of freeing it */
usb_free_urb(urb);
}
Expand Down Expand Up @@ -708,7 +714,14 @@ static int mcba_usb_open(struct net_device *netdev)

static void mcba_urb_unlink(struct mcba_priv *priv)
{
int i;

usb_kill_anchored_urbs(&priv->rx_submitted);

for (i = 0; i < MCBA_MAX_RX_URBS; ++i)
usb_free_coherent(priv->udev, MCBA_USB_RX_BUFF_SIZE,
priv->rxbuf[i], priv->rxbuf_dma[i]);

usb_kill_anchored_urbs(&priv->tx_submitted);
}

Expand Down

0 comments on commit 6bd3d80

Please sign in to comment.