linux-kernel - Re: [PATCH v3] bluetooth: hci_h5: fix memory leak in h5

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]

Message-ID: <93ed51be-97b9-c5b4-8448-d06528a1d1af@gmail.com>
Date:   Tue, 6 Oct 2020 08:14:45 +0530
From:   Anant Thazhemadam <anant.thazhemadam@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     linux-kernel-mentees@...ts.linuxfoundation.org,
        syzbot+6ce141c55b2f7aafd1c4@...kaller.appspotmail.com,
        Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] bluetooth: hci_h5: fix memory leak in h5_close

On 05-10-2020 14:48, Hans de Goede wrote:
> To fully fix the memleak you also need to add a kfree_skb(h5->rx_skb);
> call to the end of h5_serdev_remove(), because in the hu->serdev case
> that is where the h5 struct will be free-ed (it is free-ed after that
> function exits).

Hi Hans,

I'm not entirely convinced that it might be entirely the best idea to do
that.

* The bug detected by syzbot only provides us with reproducer and
information about this bug (which gets triggered when !hu->serdev).
Even if like you said, there might be a memory leak unattended to when
hu->serdev exists, then this might not necessarily be the right place to fix
it.

* From what I can see, all the drivers that have modified to provide serdev
support have different close() mechanisms.
However, one thing they do have in common (in this context) is that their
respective serdev_remove() function simply calls hci_uart_unregister_device()
to unregister the device.
It is primarily for this reason that I feel adding a kfree_skb() call at the end
of h5_serdev_remove() might not exactly be the best way we could solve this
(and since this hasn't been picked up by syzbot yet, there's no way to know if
this just fixes things or ends up causing unforeseen complications).

Alternatively, wouldn't freeing h5->rx_skb and assigning it to NULL, for both
hu->serdev and !hu->serdev cases within h5_close() itself be a better
approach? I've also taken the liberty of testing a patch that does this, and it
seems to work correctly too. :)

But then again, I'm not exactly an authority on how this works.

Thanks,
Anant