Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr
On Mon, Sep 30, 2019 at 3:22 AM Hans de Goede 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 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 > >> > >> 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, _bufs, > >> ); > >> 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.
Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr
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 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 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, _bufs, ); 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
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 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 > > 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, _bufs, ); > 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.
Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr
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 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, _bufs, ); 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
[PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr
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 --- 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); } -- 2.17.1