Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr

[Hans de Goede <hdegoede@redhat.com>]

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
>>
>>
>>
>>
>>
> 
>