Linux Kernel Mentees Archive on lore.kernel.org
 help / color / Atom feed
* [Linux-kernel-mentees] [PATCH v4] bluetooth: hci_h5: fix memory leak in h5_close
@ 2020-10-07  3:48 Anant Thazhemadam
  2020-10-16 11:28 ` Hans de Goede
  0 siblings, 1 reply; 4+ messages in thread
From: Anant Thazhemadam @ 2020-10-07  3:48 UTC (permalink / raw)
  Cc: Anant Thazhemadam, syzbot+6ce141c55b2f7aafd1c4, Johan Hedberg,
	linux-bluetooth, Marcel Holtmann, linux-kernel, Hans de Goede,
	linux-kernel-mentees

If h5_close is called when !hu->serdev, h5 is directly freed.
However, h5->rx_skb is not freed, which causes a memory leak.

Freeing h5->rx_skb fixes this memory leak.

In case hu->serdev exists, h5->rx_skb is then set to NULL,
since we do not want to risk a potential NULL pointer 
dereference.

Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated devices")
Reported-by: syzbot+6ce141c55b2f7aafd1c4@syzkaller.appspotmail.com
Tested-by: syzbot+6ce141c55b2f7aafd1c4@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>h5_close v4
---
Changes in v4:
	* Free h5->rx_skb even when hu->serdev
	(Suggested by Hans de Goede <hdegoede@redhat.com>)
	* If hu->serdev, then assign h5->rx_skb = NULL

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

 drivers/bluetooth/hci_h5.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index e41854e0d79a..39f9553caa5c 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -245,11 +245,15 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
+	kfree_skb(h5->rx_skb);
+
 	if (h5->vnd && h5->vnd->close)
 		h5->vnd->close(h5);
 
 	if (!hu->serdev)
 		kfree(h5);
+	else
+		h5->rx_skb = NULL;
 
 	return 0;
 }
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v4] bluetooth: hci_h5: fix memory leak in h5_close
  2020-10-07  3:48 [Linux-kernel-mentees] [PATCH v4] bluetooth: hci_h5: fix memory leak in h5_close Anant Thazhemadam
@ 2020-10-16 11:28 ` Hans de Goede
  2020-10-16 11:55   ` Anant Thazhemadam
  0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2020-10-16 11:28 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: syzbot+6ce141c55b2f7aafd1c4, Johan Hedberg, Marcel Holtmann,
	linux-kernel, linux-bluetooth, linux-kernel-mentees

Hi,

On 10/7/20 5:48 AM, Anant Thazhemadam wrote:
> If h5_close is called when !hu->serdev, h5 is directly freed.
> However, h5->rx_skb is not freed, which causes a memory leak.
> 
> Freeing h5->rx_skb fixes this memory leak.
> 
> In case hu->serdev exists, h5->rx_skb is then set to NULL,
> since we do not want to risk a potential NULL pointer 
> dereference.
> 
> Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated devices")
> Reported-by: syzbot+6ce141c55b2f7aafd1c4@syzkaller.appspotmail.com
> Tested-by: syzbot+6ce141c55b2f7aafd1c4@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>h5_close v4
> ---
> Changes in v4:
> 	* Free h5->rx_skb even when hu->serdev
> 	(Suggested by Hans de Goede <hdegoede@redhat.com>)
> 	* If hu->serdev, then assign h5->rx_skb = NULL
> 
> 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
> 
>  drivers/bluetooth/hci_h5.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index e41854e0d79a..39f9553caa5c 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -245,11 +245,15 @@ static int h5_close(struct hci_uart *hu)
>  	skb_queue_purge(&h5->rel);
>  	skb_queue_purge(&h5->unrel);
>  
> +	kfree_skb(h5->rx_skb);
> +
>  	if (h5->vnd && h5->vnd->close)
>  		h5->vnd->close(h5);
>  
>  	if (!hu->serdev)
>  		kfree(h5);
> +	else
> +		h5->rx_skb = NULL;

Please just do this unconditionally directly after
the kfree_skb()

So after this comment has been addressed the end result should
look like this:

	skb_queue_purge(&h5->rel);
	skb_queue_purge(&h5->unrel);
	kfree_skb(h5->rx_skb);
	h5->rx_skb = NULL;

	if (h5->vnd && h5->vnd->close)
		h5->vnd->close(h5);

	if (!hu->serdev)
		kfree(h5);
 
 	return 0;

Regards,

Hans

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v4] bluetooth: hci_h5: fix memory leak in h5_close
  2020-10-16 11:28 ` Hans de Goede
@ 2020-10-16 11:55   ` Anant Thazhemadam
  2020-10-16 12:45     ` Hans de Goede
  0 siblings, 1 reply; 4+ messages in thread
From: Anant Thazhemadam @ 2020-10-16 11:55 UTC (permalink / raw)
  To: Hans de Goede
  Cc: syzbot+6ce141c55b2f7aafd1c4, Johan Hedberg, Marcel Holtmann,
	linux-kernel, linux-bluetooth, linux-kernel-mentees


Hi,

On 16/10/20 4:58 pm, Hans de Goede wrote:
> Hi,
>
> On 10/7/20 5:48 AM, Anant Thazhemadam wrote:
>> If h5_close is called when !hu->serdev, h5 is directly freed.
>> However, h5->rx_skb is not freed, which causes a memory leak.
>>
>> Freeing h5->rx_skb fixes this memory leak.
>>
>> In case hu->serdev exists, h5->rx_skb is then set to NULL,
>> since we do not want to risk a potential NULL pointer 
>> dereference.
>>
>> Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated devices")
>> Reported-by: syzbot+6ce141c55b2f7aafd1c4@syzkaller.appspotmail.com
>> Tested-by: syzbot+6ce141c55b2f7aafd1c4@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>h5_close v4
>> ---
>> Changes in v4:
>> 	* Free h5->rx_skb even when hu->serdev
>> 	(Suggested by Hans de Goede <hdegoede@redhat.com>)
>> 	* If hu->serdev, then assign h5->rx_skb = NULL
>>
>> 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
>>
>>  drivers/bluetooth/hci_h5.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
>> index e41854e0d79a..39f9553caa5c 100644
>> --- a/drivers/bluetooth/hci_h5.c
>> +++ b/drivers/bluetooth/hci_h5.c
>> @@ -245,11 +245,15 @@ static int h5_close(struct hci_uart *hu)
>>  	skb_queue_purge(&h5->rel);
>>  	skb_queue_purge(&h5->unrel);
>>  
>> +	kfree_skb(h5->rx_skb);
>> +
>>  	if (h5->vnd && h5->vnd->close)
>>  		h5->vnd->close(h5);
>>  
>>  	if (!hu->serdev)
>>  		kfree(h5);
>> +	else
>> +		h5->rx_skb = NULL;
> Please just do this unconditionally directly after
> the kfree_skb()

Could you also please tell me why this might be necessary?
The pointer value stored at h5->rx_skb would be freed anyways when we free h5 (since rx_skb is
essentially a member of the structure that h5 points to).
Also since we're performing the *if* check, the *else* condition wouldn't exactly be taxing either,
right?
Is there some performance metric that I'm missing where unconditionally setting it to NULL
in this manner would be better? (I couldn't find any resources that had any similar analysis
performed :/ )
Or is this in interest of code readability?

Also, how about we introduce a h5 = NULL, after freeing h5 when !hu->serdev?

Thank you for your time.

Thanks,
Anant
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v4] bluetooth: hci_h5: fix memory leak in h5_close
  2020-10-16 11:55   ` Anant Thazhemadam
@ 2020-10-16 12:45     ` Hans de Goede
  0 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2020-10-16 12:45 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: syzbot+6ce141c55b2f7aafd1c4, Johan Hedberg, Marcel Holtmann,
	linux-kernel, linux-bluetooth, linux-kernel-mentees

Hi,

On 10/16/20 1:55 PM, Anant Thazhemadam wrote:
> 
> Hi,
> 
> On 16/10/20 4:58 pm, Hans de Goede wrote:
>> Hi,
>>
>> On 10/7/20 5:48 AM, Anant Thazhemadam wrote:
>>> If h5_close is called when !hu->serdev, h5 is directly freed.
>>> However, h5->rx_skb is not freed, which causes a memory leak.
>>>
>>> Freeing h5->rx_skb fixes this memory leak.
>>>
>>> In case hu->serdev exists, h5->rx_skb is then set to NULL,
>>> since we do not want to risk a potential NULL pointer 
>>> dereference.
>>>
>>> Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated devices")
>>> Reported-by: syzbot+6ce141c55b2f7aafd1c4@syzkaller.appspotmail.com
>>> Tested-by: syzbot+6ce141c55b2f7aafd1c4@syzkaller.appspotmail.com
>>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>h5_close v4
>>> ---
>>> Changes in v4:
>>> 	* Free h5->rx_skb even when hu->serdev
>>> 	(Suggested by Hans de Goede <hdegoede@redhat.com>)
>>> 	* If hu->serdev, then assign h5->rx_skb = NULL
>>>
>>> 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
>>>
>>>  drivers/bluetooth/hci_h5.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
>>> index e41854e0d79a..39f9553caa5c 100644
>>> --- a/drivers/bluetooth/hci_h5.c
>>> +++ b/drivers/bluetooth/hci_h5.c
>>> @@ -245,11 +245,15 @@ static int h5_close(struct hci_uart *hu)
>>>  	skb_queue_purge(&h5->rel);
>>>  	skb_queue_purge(&h5->unrel);
>>>  
>>> +	kfree_skb(h5->rx_skb);
>>> +
>>>  	if (h5->vnd && h5->vnd->close)
>>>  		h5->vnd->close(h5);
>>>  
>>>  	if (!hu->serdev)
>>>  		kfree(h5);
>>> +	else
>>> +		h5->rx_skb = NULL;
>> Please just do this unconditionally directly after
>> the kfree_skb()
> 
> Could you also please tell me why this might be necessary?
> The pointer value stored at h5->rx_skb would be freed anyways when we free h5 (since rx_skb is
> essentially a member of the structure that h5 points to).

It is necessary in the path where the struct h5 points to is not
free-ed and it is cleaner to just always do it then, as you
indicate yourself 

> Also since we're performing the *if* check, the *else* condition wouldn't exactly be taxing either,
> right?

For the computer it is not taxing, but for a human reading the code
and trying to understand the flow it makes things extra complicated
unnecessarily.

> Is there some performance metric that I'm missing where unconditionally setting it to NULL
> in this manner would be better? (I couldn't find any resources that had any similar analysis
> performed :/ )
> Or is this in interest of code readability?

Yes, it is in interest of code readability?

> Also, how about we introduce a h5 = NULL, after freeing h5 when !hu->serdev?

That is not necessary, there is no reason to have that in either code path.

Regards,

Hans

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07  3:48 [Linux-kernel-mentees] [PATCH v4] bluetooth: hci_h5: fix memory leak in h5_close Anant Thazhemadam
2020-10-16 11:28 ` Hans de Goede
2020-10-16 11:55   ` Anant Thazhemadam
2020-10-16 12:45     ` Hans de Goede

Linux Kernel Mentees Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kernel-mentees/0 linux-kernel-mentees/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kernel-mentees linux-kernel-mentees/ https://lore.kernel.org/linux-kernel-mentees \
		linux-kernel-mentees@lists.linuxfoundation.org linux-kernel-mentees@lists.linux-foundation.org
	public-inbox-index linux-kernel-mentees

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxfoundation.lists.linux-kernel-mentees


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git