linux-kernel - Re: [PATCH next] drm/syncobj: Fix leak in drm_syncobj_import_sync_file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF6AEGshmGO0AAD3ndz-gN32r+xf2u=7gyf9aXbkZyb97AUXdA@mail.gmail.com>
Date: Thu, 10 Apr 2025 15:06:28 -0700
From: Rob Clark <robdclark@...il.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Rob Clark <robdclark@...omium.org>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Christian König <christian.koenig@....com>, 
	Dmitry Osipenko <dmitry.osipenko@...labora.com>, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH next] drm/syncobj: Fix leak in drm_syncobj_import_sync_file_fence()

On Thu, Apr 10, 2025 at 9:33 AM Dan Carpenter <dan.carpenter@...aro.org> wrote:
>
> We need to cleanup if the chain = dma_fence_chain_alloc() allocation
> fails.  Now that we have multiple error returns in this function, switch
> to using an unwind ladder for cleanup.
>
> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
> Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 636cd83ca29e..c136d0c772dc 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -745,21 +745,24 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>  {
>         struct dma_fence *fence = sync_file_get_fence(fd);
>         struct drm_syncobj *syncobj;
> +       int ret;
>
>         if (!fence)
>                 return -EINVAL;
>
>         syncobj = drm_syncobj_find(file_private, handle);
>         if (!syncobj) {
> -               dma_fence_put(fence);
> -               return -ENOENT;
> +               ret = -ENOENT;
> +               goto err_put_fence;
>         }
>
>         if (point) {
>                 struct dma_fence_chain *chain = dma_fence_chain_alloc();
>
> -               if (!chain)
> -                       return -ENOMEM;
> +               if (!chain) {
> +                       ret = -ENOMEM;
> +                       goto err_put_syncobj;
> +               }
>
>                 drm_syncobj_add_point(syncobj, chain, fence, point);
>         } else {
> @@ -769,6 +772,13 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>         dma_fence_put(fence);
>         drm_syncobj_put(syncobj);
>         return 0;
> +
> +err_put_syncobj:
> +       drm_syncobj_put(syncobj);
> +err_put_fence:
> +       dma_fence_put(fence);
> +
> +       return ret;

I suppose you could just initialize ret to zero and collapse the two
return paths?  Either way,

Reviewed-by: Rob Clark <robdclark@...il.com>

>  }
>
>  static int drm_syncobj_export_sync_file(struct drm_file *file_private,
> --
> 2.47.2
>