linux-kernel - Re: [PATCH] usb: cdc_acm: Do not leak URB buffers

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

Date:   Mon, 24 Sep 2018 10:20:50 +0200
From:   Romain Izard <romain.izard.pro@...il.com>
To:     Oliver Neukum <oneukum@...e.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>, linux-usb@...r.kernel.org
Subject: Re: [PATCH] usb: cdc_acm: Do not leak URB buffers

2018-09-21 15:27 +0200, Oliver Neukum <oneukum@...e.com>:
>
> On Do, 2018-09-20 at 16:49 +0200, Romain Izard wrote:
> > When the ACM TTY port is disconnected, the URBs it uses must be killed,
> > and then the buffers must be freed. Unfortunately a previous refactor
> > removed the code freeing the buffers because it looked extremely similar
> > to the code killing the URBs.
> >
> > As a result, there were many new leaks for each plug/unplug cycle of a
> > CDC-ACM device, that were detected by kmemleak.
> >
> > Restore the missing code, and the memory leak is removed.
>
> Try as i may, I don't see the difference. Could you put a comment exactly
> describing the issue into the code itself, lest this problem reappear?
>

The critical point is that on shutdown, the URBs need to be killed with
usb_kill_urb, and then released with usb_free_urb.

As the code for iterating on all allocated URBs and the parameters are the
same for both functions, which also have the same length, the difference is
visually subtle. But conceptually, it is not subtle at all.

I believe that this does not need a comment.

Best regards,
--
Romain Izard