* [PATCH] drm/gem: Fix a leak in drm_gem_objects_lookup()
@ 2020-03-20 13:23 Dan Carpenter
2020-03-23 11:13 ` Emil Velikov
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2020-03-20 13:23 UTC (permalink / raw)
To: Maarten Lankhorst, Rob Herring
Cc: Thomas Zimmermann, David Airlie, kernel-janitors, dri-devel
If the "handles" allocation or the copy_from_user() fails then we leak
"objs". It's supposed to be freed in panfrost_job_cleanup().
Fixes: c117aa4d8701 ("drm: Add a drm_gem_objects_lookup helper")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/gpu/drm/drm_gem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index a9e4a610445a..f28724f2eb69 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -710,6 +710,8 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
if (!objs)
return -ENOMEM;
+ *objs_out = objs;
+
handles = kvmalloc_array(count, sizeof(u32), GFP_KERNEL);
if (!handles) {
ret = -ENOMEM;
@@ -723,8 +725,6 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
}
ret = objects_lookup(filp, handles, count, objs);
- *objs_out = objs;
-
out:
kvfree(handles);
return ret;
--
2.25.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/gem: Fix a leak in drm_gem_objects_lookup()
2020-03-20 13:23 [PATCH] drm/gem: Fix a leak in drm_gem_objects_lookup() Dan Carpenter
@ 2020-03-23 11:13 ` Emil Velikov
2020-03-23 12:13 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Emil Velikov @ 2020-03-23 11:13 UTC (permalink / raw)
To: Dan Carpenter
Cc: David Airlie, kernel-janitors, ML dri-devel, Thomas Zimmermann
Hi Dan,
On Fri, 20 Mar 2020 at 13:23, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> If the "handles" allocation or the copy_from_user() fails then we leak
> "objs". It's supposed to be freed in panfrost_job_cleanup().
>
> Fixes: c117aa4d8701 ("drm: Add a drm_gem_objects_lookup helper")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/gpu/drm/drm_gem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index a9e4a610445a..f28724f2eb69 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -710,6 +710,8 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
> if (!objs)
> return -ENOMEM;
>
> + *objs_out = objs;
> +
> handles = kvmalloc_array(count, sizeof(u32), GFP_KERNEL);
> if (!handles) {
> ret = -ENOMEM;
> @@ -723,8 +725,6 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
> }
>
> ret = objects_lookup(filp, handles, count, objs);
> - *objs_out = objs;
> -
> out:
> kvfree(handles);
> return ret;
It seems that this will return error to the caller, mangle the output
pointer and effectively still leak the objs.
Better option IMHO is to:
- move the __user/copy_from_user into the caller
Removes a silly kvmalloc_array(1,...) in ~90+ users and drops the "out" label.
Extra bonus, this is the only instance in drm_gem with __user -
consistency is nice.
- add "err" or similar label, where the objs is freed before returning an error.
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/gem: Fix a leak in drm_gem_objects_lookup()
2020-03-23 11:13 ` Emil Velikov
@ 2020-03-23 12:13 ` Dan Carpenter
2020-05-17 20:48 ` Emil Velikov
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2020-03-23 12:13 UTC (permalink / raw)
To: Emil Velikov
Cc: David Airlie, kernel-janitors, ML dri-devel, Thomas Zimmermann
On Mon, Mar 23, 2020 at 11:13:22AM +0000, Emil Velikov wrote:
> Hi Dan,
>
> On Fri, 20 Mar 2020 at 13:23, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > If the "handles" allocation or the copy_from_user() fails then we leak
> > "objs". It's supposed to be freed in panfrost_job_cleanup().
> >
> > Fixes: c117aa4d8701 ("drm: Add a drm_gem_objects_lookup helper")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > drivers/gpu/drm/drm_gem.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index a9e4a610445a..f28724f2eb69 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -710,6 +710,8 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
> > if (!objs)
> > return -ENOMEM;
> >
> > + *objs_out = objs;
> > +
> > handles = kvmalloc_array(count, sizeof(u32), GFP_KERNEL);
> > if (!handles) {
> > ret = -ENOMEM;
> > @@ -723,8 +725,6 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
> > }
> >
> > ret = objects_lookup(filp, handles, count, objs);
> > - *objs_out = objs;
> > -
> > out:
> > kvfree(handles);
> > return ret;
>
> It seems that this will return error to the caller, mangle the output
> pointer and effectively still leak the objs.
The patch works.
This is "one function frees everything" style error handling. It gets
passed back to panfrost_ioctl_submit() which calls panfrost_job_put()
which calls panfrost_job_cleanup() which frees it.
It's a horrible way to do error handling but this was the only actual
bug I could see with the approach.
> Better option IMHO is to:
> - move the __user/copy_from_user into the caller
> Removes a silly kvmalloc_array(1,...) in ~90+ users and drops the "out" label.
> Extra bonus, this is the only instance in drm_gem with __user -
> consistency is nice.
> - add "err" or similar label, where the objs is freed before returning an error.
Those sound like good ideas. Also we could use kvcalloc() instead of
kvmalloc_array() with __GFP_ZERO. But it's too much for me to do...
I'm mostly focused on static analysis warnings.
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/gem: Fix a leak in drm_gem_objects_lookup()
2020-03-23 12:13 ` Dan Carpenter
@ 2020-05-17 20:48 ` Emil Velikov
0 siblings, 0 replies; 4+ messages in thread
From: Emil Velikov @ 2020-05-17 20:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: David Airlie, kernel-janitors, ML dri-devel, Thomas Zimmermann
On Mon, 23 Mar 2020 at 12:13, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Mar 23, 2020 at 11:13:22AM +0000, Emil Velikov wrote:
> > Hi Dan,
> >
> > On Fri, 20 Mar 2020 at 13:23, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > If the "handles" allocation or the copy_from_user() fails then we leak
> > > "objs". It's supposed to be freed in panfrost_job_cleanup().
> > >
> > > Fixes: c117aa4d8701 ("drm: Add a drm_gem_objects_lookup helper")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > > drivers/gpu/drm/drm_gem.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index a9e4a610445a..f28724f2eb69 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -710,6 +710,8 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
> > > if (!objs)
> > > return -ENOMEM;
> > >
> > > + *objs_out = objs;
> > > +
> > > handles = kvmalloc_array(count, sizeof(u32), GFP_KERNEL);
> > > if (!handles) {
> > > ret = -ENOMEM;
> > > @@ -723,8 +725,6 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
> > > }
> > >
> > > ret = objects_lookup(filp, handles, count, objs);
> > > - *objs_out = objs;
> > > -
> > > out:
> > > kvfree(handles);
> > > return ret;
> >
> > It seems that this will return error to the caller, mangle the output
> > pointer and effectively still leak the objs.
>
> The patch works.
>
> This is "one function frees everything" style error handling. It gets
> passed back to panfrost_ioctl_submit() which calls panfrost_job_put()
> which calls panfrost_job_cleanup() which frees it.
>
> It's a horrible way to do error handling but this was the only actual
> bug I could see with the approach.
>
> > Better option IMHO is to:
> > - move the __user/copy_from_user into the caller
> > Removes a silly kvmalloc_array(1,...) in ~90+ users and drops the "out" label.
> > Extra bonus, this is the only instance in drm_gem with __user -
> > consistency is nice.
> > - add "err" or similar label, where the objs is freed before returning an error.
>
> Those sound like good ideas. Also we could use kvcalloc() instead of
> kvmalloc_array() with __GFP_ZERO. But it's too much for me to do...
> I'm mostly focused on static analysis warnings.
>
Your patch addresses the issue with the smallest diffstat, so I've
pushed it to drm-misc-next.
Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-17 20:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 13:23 [PATCH] drm/gem: Fix a leak in drm_gem_objects_lookup() Dan Carpenter
2020-03-23 11:13 ` Emil Velikov
2020-03-23 12:13 ` Dan Carpenter
2020-05-17 20:48 ` Emil Velikov
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).