Hi,
On 10/4/20 7:17 AM, Anant Thazhemadam wrote:
> When h5_close() is called and !hu->serdev, h5 is directly freed.
> However, h5->rx_skb is not freed before h5 is freed, which causes
> a memory leak.
> Freeing h5->rx_skb (when !hu->serdev) fixes this memory leak before
> freeing h5.
>
> Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated devices")
> Reported-by: [email protected]
> Tested-by: [email protected]
> Signed-off-by: Anant Thazhemadam <[email protected]>
> ---
> Changes in v3:
> * Free h5->rx_skb when !hu->serdev, and fix the memory leak
> * Do not incorrectly and unnecessarily call serdev_device_close()
>
> Changes in v2:
> * Fixed the Fixes tag
>
> Hans de Goede also suggested calling h5_reset_rx() on close (for both, !hu->serdev
> and hu->serdev cases).
> However, doing so seems to lead to a null-ptr-dereference error,
> https://syzkaller.appspot.com/text?tag=CrashReport&x=136a9a5d900000,
> and for this reason, it has not been implemented.
>
> Instead, directly freeing h5->rx_skb seems to suffice in preventing the memory leak
> reported.
> And since h5 is freed immediately after freeing h5->rx_skb, assigning h5->rx_skb to
> be NULL isn't necessary.
>
> drivers/bluetooth/hci_h5.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index e41854e0d79a..171e55c080ce 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -248,8 +248,10 @@ static int h5_close(struct hci_uart *hu)
> if (h5->vnd && h5->vnd->close)
> h5->vnd->close(h5);
>
> - if (!hu->serdev)
> + if (!hu->serdev){
> + kfree_skb(h5->rx_skb);
> kfree(h5);
> + }
>
> return 0;
> }
>
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).
Regards,
Hans
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
Hi,
On 10/6/20 4:44 AM, Anant Thazhemadam wrote:
> 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?
That will indeed also fix the leak in both cases and is more consistent
wrt when the free_skb happens. So this sounds good to me, go for it.
Regards,
Hans
> 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
>