* [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr
@ 2019-09-27 23:04 Navid Emamdoost
2019-09-28 9:54 ` Hans de Goede
0 siblings, 1 reply; 7+ messages in thread
From: Navid Emamdoost @ 2019-09-27 23:04 UTC (permalink / raw)
Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Hans de Goede,
Arnd Bergmann, Greg Kroah-Hartman, linux-kernel
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>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr 2019-09-27 23:04 [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr Navid Emamdoost @ 2019-09-28 9:54 ` Hans de Goede 2019-09-30 2:22 ` Navid Emamdoost 0 siblings, 1 reply; 7+ messages in thread From: Hans de Goede @ 2019-09-28 9:54 UTC (permalink / raw) To: Navid Emamdoost Cc: emamd001, kjlu, smccaman, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr 2019-09-28 9:54 ` Hans de Goede @ 2019-09-30 2:22 ` Navid Emamdoost 2019-09-30 8:22 ` Hans de Goede 0 siblings, 1 reply; 7+ messages in thread From: Navid Emamdoost @ 2019-09-30 2:22 UTC (permalink / raw) To: Hans de Goede Cc: Navid Emamdoost, kjlu, Stephen McCamant, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr 2019-09-30 2:22 ` Navid Emamdoost @ 2019-09-30 8:22 ` Hans de Goede 2019-09-30 20:42 ` [PATCH v2] " Navid Emamdoost 2019-09-30 20:43 ` [PATCH] " Navid Emamdoost 0 siblings, 2 replies; 7+ messages in thread From: Hans de Goede @ 2019-09-30 8:22 UTC (permalink / raw) To: Navid Emamdoost Cc: Navid Emamdoost, kjlu, Stephen McCamant, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel 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 >> >> >> >> >> > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr 2019-09-30 8:22 ` Hans de Goede @ 2019-09-30 20:42 ` Navid Emamdoost 2019-10-01 6:15 ` Hans de Goede 2019-09-30 20:43 ` [PATCH] " Navid Emamdoost 1 sibling, 1 reply; 7+ messages in thread From: Navid Emamdoost @ 2019-09-30 20:42 UTC (permalink / raw) To: hdegoede Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel In hgcm_call_preprocess_linaddr memory is allocated for bounce_buf but is not released if copy_form_user fails. In order to prevent memory leak in case of failure, the assignment to bounce_buf_ret is moved before the error check. This way the allocated bounce_buf will be released by the caller. Fixes: 579db9d45cb4 ("virt: Add vboxguest VMMDEV communication code") Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- drivers/virt/vboxguest/vboxguest_utils.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/virt/vboxguest/vboxguest_utils.c b/drivers/virt/vboxguest/vboxguest_utils.c index 75fd140b02ff..43c391626a00 100644 --- a/drivers/virt/vboxguest/vboxguest_utils.c +++ b/drivers/virt/vboxguest/vboxguest_utils.c @@ -220,6 +220,8 @@ static int hgcm_call_preprocess_linaddr( 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) @@ -228,7 +230,6 @@ static int hgcm_call_preprocess_linaddr( memset(bounce_buf, 0, len); } - *bounce_buf_ret = bounce_buf; hgcm_call_add_pagelist_size(bounce_buf, len, extra); return 0; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr 2019-09-30 20:42 ` [PATCH v2] " Navid Emamdoost @ 2019-10-01 6:15 ` Hans de Goede 0 siblings, 0 replies; 7+ messages in thread From: Hans de Goede @ 2019-10-01 6:15 UTC (permalink / raw) To: Navid Emamdoost Cc: emamd001, smccaman, kjlu, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel Hi, On 30-09-2019 22:42, Navid Emamdoost wrote: > In hgcm_call_preprocess_linaddr memory is allocated for bounce_buf but > is not released if copy_form_user fails. In order to prevent memory leak > in case of failure, the assignment to bounce_buf_ret is moved before the > error check. This way the allocated bounce_buf will be released by the > caller. > > Fixes: 579db9d45cb4 ("virt: Add vboxguest VMMDEV communication code") > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> Thank you. Patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/virt/vboxguest/vboxguest_utils.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/virt/vboxguest/vboxguest_utils.c b/drivers/virt/vboxguest/vboxguest_utils.c > index 75fd140b02ff..43c391626a00 100644 > --- a/drivers/virt/vboxguest/vboxguest_utils.c > +++ b/drivers/virt/vboxguest/vboxguest_utils.c > @@ -220,6 +220,8 @@ static int hgcm_call_preprocess_linaddr( > 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) > @@ -228,7 +230,6 @@ static int hgcm_call_preprocess_linaddr( > memset(bounce_buf, 0, len); > } > > - *bounce_buf_ret = bounce_buf; > hgcm_call_add_pagelist_size(bounce_buf, len, extra); > return 0; > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr 2019-09-30 8:22 ` Hans de Goede 2019-09-30 20:42 ` [PATCH v2] " Navid Emamdoost @ 2019-09-30 20:43 ` Navid Emamdoost 1 sibling, 0 replies; 7+ messages in thread From: Navid Emamdoost @ 2019-09-30 20:43 UTC (permalink / raw) To: Hans de Goede Cc: Navid Emamdoost, kjlu, Stephen McCamant, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel On Mon, Sep 30, 2019 at 3:22 AM Hans de Goede <hdegoede@redhat.com> 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 <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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-01 6:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-27 23:04 [PATCH] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr Navid Emamdoost 2019-09-28 9:54 ` Hans de Goede 2019-09-30 2:22 ` Navid Emamdoost 2019-09-30 8:22 ` Hans de Goede 2019-09-30 20:42 ` [PATCH v2] " Navid Emamdoost 2019-10-01 6:15 ` Hans de Goede 2019-09-30 20:43 ` [PATCH] " Navid Emamdoost
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).