linux-kernel - Re: [PATCH 3/3] udmabuf: fix memory leak on last export

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] [day] [month] [year] [list]

Message-ID: <CAG48ez3pVzBFTb6uX47fps8mZp3TaRWPcLaKvQwREUKVApd02A@mail.gmail.com>
Date: Wed, 4 Dec 2024 17:15:10 +0100
From: Jann Horn <jannh@...gle.com>
To: "Kasireddy, Vivek" <vivek.kasireddy@...el.com>
Cc: Gerd Hoffmann <kraxel@...hat.com>, Sumit Semwal <sumit.semwal@...aro.org>, 
	Christian König <christian.koenig@....com>, 
	Simona Vetter <simona.vetter@...ll.ch>, Andrew Morton <akpm@...ux-foundation.org>, 
	"Joel Fernandes (Google)" <joel@...lfernandes.org>, 
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>, 
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>, 
	"linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] udmabuf: fix memory leak on last export_udmabuf()
 error path

On Wed, Dec 4, 2024 at 10:14 AM Kasireddy, Vivek
<vivek.kasireddy@...el.com> wrote:
> > Subject: [PATCH 3/3] udmabuf: fix memory leak on last export_udmabuf()
> > error path
> >
> > In export_udmabuf(), if dma_buf_fd() fails because the FD table is full, a
> > dma_buf owning the udmabuf has already been created; but the error
> > handling
> > in udmabuf_create() will tear down the udmabuf without doing anything
> > about
> > the containing dma_buf.
> >
> > This leaves a dma_buf in memory that contains a dangling pointer; though
> > that doesn't seem to lead to anything bad except a memory leak.
> >
> > Fix it by moving the dma_buf_fd() call out of export_udmabuf() so that we
> > can give it different error handling.
> >
> > Note that the shape of this code changed a lot in commit 5e72b2b41a21
> > ("udmabuf: convert udmabuf driver to use folios"); but the memory leak
> > seems to have existed since the introduction of udmabuf.
> >
> > Fixes: fbb0de795078 ("Add udmabuf misc device")
> > Signed-off-by: Jann Horn <jannh@...gle.com>
[...]
> > @@ -330,11 +329,7 @@ static int export_udmabuf(struct udmabuf *ubuf,
> >       exp_info.priv = ubuf;
> >       exp_info.flags = O_RDWR;
> >
> > -     buf = dma_buf_export(&exp_info);
> > -     if (IS_ERR(buf))
> > -             return PTR_ERR(buf);
> > -
> > -     return dma_buf_fd(buf, flags);
> flags is now unused in this function.

Ack, will remove in v2.

> > +     return dma_buf_export(&exp_info);
> >  }
> >
> >  static long udmabuf_pin_folios(struct udmabuf *ubuf, struct file *memfd,
> > @@ -391,6 +386,7 @@ static long udmabuf_create(struct miscdevice
> > *device,
> >       struct folio **folios = NULL;
> >       pgoff_t pgcnt = 0, pglimit;
> >       struct udmabuf *ubuf;
> > +     struct dma_buf *dmabuf;
> >       long ret = -EINVAL;
> >       u32 i, flags;
> >
> > @@ -451,9 +447,16 @@ static long udmabuf_create(struct miscdevice
> > *device,
> >       }
> >
> >       flags = head->flags & UDMABUF_FLAGS_CLOEXEC ? O_CLOEXEC : 0;
> > -     ret = export_udmabuf(ubuf, device, flags);
> > -     if (ret < 0)
> > +     dmabuf = export_udmabuf(ubuf, device, flags);
> > +     if (IS_ERR(dmabuf)) {
> > +             ret = PTR_ERR(dmabuf);
> >               goto err;
> > +     }
> > +     /* ownership of ubuf is held by the dmabuf from here */
> Please also add a comment here that says that if dma_buf_fd() fails,
> then calling dma_buf_put() will enable cleanup to be done via release().

Ack, added in v2.

> With that,
>
> Acked-by: Vivek Kasireddy <vivek.kasireddy@...el.com>

Thanks!