Messages in this thread | | | Subject | Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr | From | Hans de Goede <> | Date | Mon, 30 Sep 2019 10:22:30 +0200 |
| |
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 >> >> >> >> >> > >
| |