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

Message-ID: <IA0PR11MB7185D54AD6BB8067EAA6AE8EF8372@IA0PR11MB7185.namprd11.prod.outlook.com>
Date: Wed, 4 Dec 2024 09:13:21 +0000
From: "Kasireddy, Vivek" <vivek.kasireddy@...el.com>
To: Jann Horn <jannh@...gle.com>, Gerd Hoffmann <kraxel@...hat.com>, "Sumit
 Semwal" <sumit.semwal@...aro.org>, Christian König
	<christian.koenig@....com>, Simona Vetter <simona.vetter@...ll.ch>, "John
 Stultz" <john.stultz@...aro.org>, Andrew Morton <akpm@...ux-foundation.org>,
	"Joel Fernandes (Google)" <joel@...lfernandes.org>
CC: "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

Hi Jann,

> 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>
> ---
>  drivers/dma-buf/udmabuf.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index
> 8ce77f5837d71a73be677cad014e05f29706057d..aae0071be14a2c83a428b59
> ea9e905c7173232be 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -317,12 +317,11 @@ static int check_memfd_seals(struct file *memfd)
>  	return 0;
>  }
> 
> -static int export_udmabuf(struct udmabuf *ubuf,
> -			  struct miscdevice *device,
> -			  u32 flags)
> +static struct dma_buf *export_udmabuf(struct udmabuf *ubuf,
> +				      struct miscdevice *device,
> +				      u32 flags)
>  {
>  	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> -	struct dma_buf *buf;
> 
>  	ubuf->device = device;
>  	exp_info.ops  = &udmabuf_ops;
> @@ -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.

> +	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().
With that,

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

> +
> +	ret = dma_buf_fd(dmabuf, flags);
> +	if (ret < 0)
> +		dma_buf_put(dmabuf);
> 
>  	kvfree(folios);
>  	return ret;
> 
> --
> 2.47.0.338.g60cca15819-goog