Messages in this thread | | | Subject | Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr | From | Hans de Goede <> | Date | Sat, 28 Sep 2019 11:54:20 +0200 |
| |
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
| |