Messages in this thread | | | From | Navid Emamdoost <> | Date | Sun, 29 Sep 2019 21:22:56 -0500 | Subject | Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr |
| |
It is a neat fix now, thank you.
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 > > > > >
-- Navid.
| |