[PATCH] drm/gem: Fix a leak in drm_gem_objects_lookup()

[Emil Velikov]

Hi Dan,

On Fri, 20 Mar 2020 at 13:23, Dan Carpenter <dan.carpenter at 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 at 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