[Linux-kernel-mentees] [PATCH v4] bluetooth: hci_h5: fix memory leak in h5_close

Anant Thazhemadam anant.thazhemadam at gmail.com
Fri Oct 16 11:55:54 UTC 2020


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 at syzkaller.appspotmail.com
>> Tested-by: syzbot+6ce141c55b2f7aafd1c4 at syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam at gmail.com>h5_close v4
>> ---
>> Changes in v4:
>> 	* Free h5->rx_skb even when hu->serdev
>> 	(Suggested by Hans de Goede <hdegoede at 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


More information about the Linux-kernel-mentees mailing list