lkml.org 
[lkml]   [2019]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr
From
Date
Hi,

On 30-09-2019 04:22, Navid Emamdoost wrote:
> It is a neat fix now, thank you.

Can you submit a new version of your patch with the fix I proposed please ?

Regards,

Hans


>
>
> On Sat, Sep 28, 2019 at 4:54 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 28-09-2019 01:04, Navid Emamdoost wrote:
>>> In hgcm_call_preprocess_linaddr memory is allocated for bounce_buf but
>>> is not released if copy_form_user fails. The release is added.
>>>
>>> Fixes: 579db9d45cb4 ("virt: Add vboxguest VMMDEV communication code")
>>>
>>> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
>>
>> Thank you for catching this, I agree this is a bug, but I think we
>> can fix it in a cleaner way (see below).
>>
>>> ---
>>> drivers/virt/vboxguest/vboxguest_utils.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/virt/vboxguest/vboxguest_utils.c b/drivers/virt/vboxguest/vboxguest_utils.c
>>> index 75fd140b02ff..7965885a50fa 100644
>>> --- a/drivers/virt/vboxguest/vboxguest_utils.c
>>> +++ b/drivers/virt/vboxguest/vboxguest_utils.c
>>> @@ -222,8 +222,10 @@ static int hgcm_call_preprocess_linaddr(
>>>
>>> if (copy_in) {
>>> ret = copy_from_user(bounce_buf, (void __user *)buf, len);
>>> - if (ret)
>>> + if (ret) {
>>> + kvfree(bounce_buf);
>>> return -EFAULT;
>>> + }
>>> } else {
>>> memset(bounce_buf, 0, len);
>>> }
>>>
>>
>> First let me quote some more of the function, pre leak fix, for context:
>>
>> bounce_buf = kvmalloc(len, GFP_KERNEL);
>> if (!bounce_buf)
>> return -ENOMEM;
>>
>> if (copy_in) {
>> ret = copy_from_user(bounce_buf, (void __user *)buf, len);
>> if (ret)
>> return -EFAULT;
>> } else {
>> memset(bounce_buf, 0, len);
>> }
>>
>> *bounce_buf_ret = bounce_buf;
>>
>> This function gets called repeatedly by hgcm_call_preprocess(), and the
>> caller of hgcm_call_preprocess() already takes care of freeing the
>> bounce bufs both on a (later) error and on success:
>>
>> ret = hgcm_call_preprocess(parms, parm_count, &bounce_bufs, &size);
>> if (ret) {
>> /* Even on error bounce bufs may still have been allocated */
>> goto free_bounce_bufs;
>> }
>>
>> ...
>>
>> free_bounce_bufs:
>> if (bounce_bufs) {
>> for (i = 0; i < parm_count; i++)
>> kvfree(bounce_bufs[i]);
>> kfree(bounce_bufs);
>> }
>>
>> So we are already taking care of freeing bounce-bufs allocated for previous
>> parameters to the call (which me must do anyways), so a cleaner fix would
>> be to store the allocated bounce_buf in the bounce_bufs array before
>> doing the copy_from_user, then if copy_from_user fails it will be cleaned
>> up by the code at the free_bounce_bufs label.
>>
>> IOW I believe it is better to fix this by changing the part of
>> hgcm_call_preprocess_linaddr I quoted to:
>>
>> bounce_buf = kvmalloc(len, GFP_KERNEL);
>> if (!bounce_buf)
>> return -ENOMEM;
>>
>> *bounce_buf_ret = bounce_buf;
>>
>> if (copy_in) {
>> ret = copy_from_user(bounce_buf, (void __user *)buf, len);
>> if (ret)
>> return -EFAULT;
>> } else {
>> memset(bounce_buf, 0, len);
>> }
>>
>> This should also fix the leak in IMHO is a clear way of doing so.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
>
>

\
 
 \ /
  Last update: 2019-09-30 10:23    [W:5.199 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site