On Mon, Sep 30, 2019 at 3:22 AM Hans de Goede <[email protected]> wrote: > > 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 >
Sure, v2 was sent. > > > > > > > On Sat, Sep 28, 2019 at 4:54 AM Hans de Goede <[email protected]> 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 <[email protected]> > >> > >> 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.
