[PATCH 1/2] habanalabs: fix UAF in export_dmabuf()

[PATCH 1/2] habanalabs: fix UAF in export_dmabuf()

[Al Viro]

[dma_buf_fd() fixes; no preferences regarding the tree it goes through -
up to dri folks]

As soon as we'd inserted a file reference into descriptor table, another
thread could close it.  That's fine for the case when all we are doing is
returning that descriptor to userland (it's a race, but it's a userland
race and there's nothing the kernel can do about it).  However, if we
follow fd_install() with any kind of access to objects that would be
destroyed on close (be it the struct file itself or anything destroyed
by its ->release()), we have a UAF.

dma_buf_fd() is a combination of reserving a descriptor and fd_install().
habanalabs export_dmabuf() calls it and then proceeds to access the
objects destroyed on close.  In particular, it grabs an extra reference to
another struct file that will be dropped as part of ->release() for ours;
that "will be" is actually "might have already been".

Fix that by reserving descriptor before anything else and do fd_install()
only when everything had been set up.  As a side benefit, we no longer
have the failure exit with file already created, but reference to
underlying file (as well as ->dmabuf_export_cnt, etc.) not grabbed yet;
unlike dma_buf_fd(), fd_install() can't fail.

Fixes: db1a8dd916aa ("habanalabs: add support for dma-buf exporter")
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/accel/habanalabs/common/memory.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/accel/habanalabs/common/memory.c b/drivers/accel/habanalabs/common/memory.c
index 601fdbe70179..61472a381904 100644
--- a/drivers/accel/habanalabs/common/memory.c
+++ b/drivers/accel/habanalabs/common/memory.c
@@ -1829,9 +1829,6 @@ static void hl_release_dmabuf(struct dma_buf *dmabuf)
 	struct hl_dmabuf_priv *hl_dmabuf = dmabuf->priv;
 	struct hl_ctx *ctx;
 
-	if (!hl_dmabuf)
-		return;
-
 	ctx = hl_dmabuf->ctx;
 
 	if (hl_dmabuf->memhash_hnode)
@@ -1859,7 +1856,12 @@ static int export_dmabuf(struct hl_ctx *ctx,
 {
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
 	struct hl_device *hdev = ctx->hdev;
-	int rc, fd;
+	CLASS(get_unused_fd, fd)(flags);
+
+	if (fd < 0) {
+		dev_err(hdev->dev, "failed to get a file descriptor for a dma-buf, %d\n", fd);
+		return fd;
+	}
 
 	exp_info.ops = &habanalabs_dmabuf_ops;
 	exp_info.size = total_size;
@@ -1872,13 +1874,6 @@ static int export_dmabuf(struct hl_ctx *ctx,
 		return PTR_ERR(hl_dmabuf->dmabuf);
 	}
 
-	fd = dma_buf_fd(hl_dmabuf->dmabuf, flags);
-	if (fd < 0) {
-		dev_err(hdev->dev, "failed to get a file descriptor for a dma-buf, %d\n", fd);
-		rc = fd;
-		goto err_dma_buf_put;
-	}
-
 	hl_dmabuf->ctx = ctx;
 	hl_ctx_get(hl_dmabuf->ctx);
 	atomic_inc(&ctx->hdev->dmabuf_export_cnt);
@@ -1890,13 +1885,9 @@ static int export_dmabuf(struct hl_ctx *ctx,
 	get_file(ctx->hpriv->file_priv->filp);
 
 	*dmabuf_fd = fd;
+	fd_install(take_fd(fd), hl_dmabuf->dmabuf->file);
 
 	return 0;
-
-err_dma_buf_put:
-	hl_dmabuf->dmabuf->priv = NULL;
-	dma_buf_put(hl_dmabuf->dmabuf);
-	return rc;
 }
 
 static int validate_export_params_common(struct hl_device *hdev, u64 addr, u64 size, u64 offset)
-- 
2.39.5

[PATCH 2/2] xen: fix UAF in dmabuf_exp_from_pages()

[Al Viro]

[dma_buf_fd() fixes; no preferences regarding the tree it goes through -
up to xen folks]

As soon as we'd inserted a file reference into descriptor table, another
thread could close it.  That's fine for the case when all we are doing is
returning that descriptor to userland (it's a race, but it's a userland
race and there's nothing the kernel can do about it).  However, if we
follow fd_install() with any kind of access to objects that would be
destroyed on close (be it the struct file itself or anything destroyed
by its ->release()), we have a UAF.

dma_buf_fd() is a combination of reserving a descriptor and fd_install().
gntdev dmabuf_exp_from_pages() calls it and then proceeds to access the
objects destroyed on close - starting with gntdev_dmabuf itself.

Fix that by doing reserving descriptor before anything else and do
fd_install() only when everything had been set up.

Fixes: a240d6e42e28 ("xen/gntdev: Implement dma-buf export functionality")
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/xen/gntdev-dmabuf.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 5453d86324f6..82855105ab85 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -357,8 +357,11 @@ struct gntdev_dmabuf_export_args {
 static int dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args)
 {
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
-	struct gntdev_dmabuf *gntdev_dmabuf;
-	int ret;
+	struct gntdev_dmabuf *gntdev_dmabuf __free(kfree) = NULL;
+	CLASS(get_unused_fd, ret)(O_CLOEXEC);
+
+	if (ret < 0)
+		return ret;
 
 	gntdev_dmabuf = kzalloc(sizeof(*gntdev_dmabuf), GFP_KERNEL);
 	if (!gntdev_dmabuf)
@@ -383,32 +386,21 @@ static int dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args)
 	exp_info.priv = gntdev_dmabuf;
 
 	gntdev_dmabuf->dmabuf = dma_buf_export(&exp_info);
-	if (IS_ERR(gntdev_dmabuf->dmabuf)) {
-		ret = PTR_ERR(gntdev_dmabuf->dmabuf);
-		gntdev_dmabuf->dmabuf = NULL;
-		goto fail;
-	}
-
-	ret = dma_buf_fd(gntdev_dmabuf->dmabuf, O_CLOEXEC);
-	if (ret < 0)
-		goto fail;
+	if (IS_ERR(gntdev_dmabuf->dmabuf))
+		return PTR_ERR(gntdev_dmabuf->dmabuf);
 
 	gntdev_dmabuf->fd = ret;
 	args->fd = ret;
 
 	pr_debug("Exporting DMA buffer with fd %d\n", ret);
 
+	get_file(gntdev_dmabuf->priv->filp);
 	mutex_lock(&args->dmabuf_priv->lock);
 	list_add(&gntdev_dmabuf->next, &args->dmabuf_priv->exp_list);
 	mutex_unlock(&args->dmabuf_priv->lock);
-	get_file(gntdev_dmabuf->priv->filp);
-	return 0;
 
-fail:
-	if (gntdev_dmabuf->dmabuf)
-		dma_buf_put(gntdev_dmabuf->dmabuf);
-	kfree(gntdev_dmabuf);
-	return ret;
+	fd_install(take_fd(ret), no_free_ptr(gntdev_dmabuf)->dmabuf->file);
+	return 0;
 }
 
 static struct gntdev_grant_map *
-- 
2.39.5

Re: [PATCH 1/2] habanalabs: fix UAF in export_dmabuf()

[Lukas Wunner]

On Sat, Jul 12, 2025 at 06:02:31AM +0100, Al Viro wrote:
> [dma_buf_fd() fixes; no preferences regarding the tree it goes through -
> up to dri folks]

A MAINTAINERS change for the habanalabs driver landed in Linus' tree
a few hours ago.  I'm adding the new maintainers Yaron, Koby and
Konstantin to the To: header.  Thanks! -- Lukas

> As soon as we'd inserted a file reference into descriptor table, another
> thread could close it.  That's fine for the case when all we are doing is
> returning that descriptor to userland (it's a race, but it's a userland
> race and there's nothing the kernel can do about it).  However, if we
> follow fd_install() with any kind of access to objects that would be
> destroyed on close (be it the struct file itself or anything destroyed
> by its ->release()), we have a UAF.
> 
> dma_buf_fd() is a combination of reserving a descriptor and fd_install().
> habanalabs export_dmabuf() calls it and then proceeds to access the
> objects destroyed on close.  In particular, it grabs an extra reference to
> another struct file that will be dropped as part of ->release() for ours;
> that "will be" is actually "might have already been".
> 
> Fix that by reserving descriptor before anything else and do fd_install()
> only when everything had been set up.  As a side benefit, we no longer
> have the failure exit with file already created, but reference to
> underlying file (as well as ->dmabuf_export_cnt, etc.) not grabbed yet;
> unlike dma_buf_fd(), fd_install() can't fail.
> 
> Fixes: db1a8dd916aa ("habanalabs: add support for dma-buf exporter")
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/accel/habanalabs/common/memory.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/accel/habanalabs/common/memory.c b/drivers/accel/habanalabs/common/memory.c
> index 601fdbe70179..61472a381904 100644
> --- a/drivers/accel/habanalabs/common/memory.c
> +++ b/drivers/accel/habanalabs/common/memory.c
> @@ -1829,9 +1829,6 @@ static void hl_release_dmabuf(struct dma_buf *dmabuf)
>  	struct hl_dmabuf_priv *hl_dmabuf = dmabuf->priv;
>  	struct hl_ctx *ctx;
>  
> -	if (!hl_dmabuf)
> -		return;
> -
>  	ctx = hl_dmabuf->ctx;
>  
>  	if (hl_dmabuf->memhash_hnode)
> @@ -1859,7 +1856,12 @@ static int export_dmabuf(struct hl_ctx *ctx,
>  {
>  	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>  	struct hl_device *hdev = ctx->hdev;
> -	int rc, fd;
> +	CLASS(get_unused_fd, fd)(flags);
> +
> +	if (fd < 0) {
> +		dev_err(hdev->dev, "failed to get a file descriptor for a dma-buf, %d\n", fd);
> +		return fd;
> +	}
>  
>  	exp_info.ops = &habanalabs_dmabuf_ops;
>  	exp_info.size = total_size;
> @@ -1872,13 +1874,6 @@ static int export_dmabuf(struct hl_ctx *ctx,
>  		return PTR_ERR(hl_dmabuf->dmabuf);
>  	}
>  
> -	fd = dma_buf_fd(hl_dmabuf->dmabuf, flags);
> -	if (fd < 0) {
> -		dev_err(hdev->dev, "failed to get a file descriptor for a dma-buf, %d\n", fd);
> -		rc = fd;
> -		goto err_dma_buf_put;
> -	}
> -
>  	hl_dmabuf->ctx = ctx;
>  	hl_ctx_get(hl_dmabuf->ctx);
>  	atomic_inc(&ctx->hdev->dmabuf_export_cnt);
> @@ -1890,13 +1885,9 @@ static int export_dmabuf(struct hl_ctx *ctx,
>  	get_file(ctx->hpriv->file_priv->filp);
>  
>  	*dmabuf_fd = fd;
> +	fd_install(take_fd(fd), hl_dmabuf->dmabuf->file);
>  
>  	return 0;
> -
> -err_dma_buf_put:
> -	hl_dmabuf->dmabuf->priv = NULL;
> -	dma_buf_put(hl_dmabuf->dmabuf);
> -	return rc;
>  }
>  
>  static int validate_export_params_common(struct hl_device *hdev, u64 addr, u64 size, u64 offset)
> -- 
> 2.39.5

Re: [PATCH 2/2] xen: fix UAF in dmabuf_exp_from_pages()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1197 bytes --]

On 12.07.25 07:09, Al Viro wrote:
> [dma_buf_fd() fixes; no preferences regarding the tree it goes through -
> up to xen folks]

I'll take it via the Xen tree.

> As soon as we'd inserted a file reference into descriptor table, another
> thread could close it.  That's fine for the case when all we are doing is
> returning that descriptor to userland (it's a race, but it's a userland
> race and there's nothing the kernel can do about it).  However, if we
> follow fd_install() with any kind of access to objects that would be
> destroyed on close (be it the struct file itself or anything destroyed
> by its ->release()), we have a UAF.
> 
> dma_buf_fd() is a combination of reserving a descriptor and fd_install().
> gntdev dmabuf_exp_from_pages() calls it and then proceeds to access the
> objects destroyed on close - starting with gntdev_dmabuf itself.
> 
> Fix that by doing reserving descriptor before anything else and do
> fd_install() only when everything had been set up.
> 
> Fixes: a240d6e42e28 ("xen/gntdev: Implement dma-buf export functionality")
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen: fix UAF in dmabuf_exp_from_pages()

[Al Viro]

On Mon, Jul 14, 2025 at 09:57:02AM +0200, Jürgen Groß wrote:
> On 12.07.25 07:09, Al Viro wrote:
> > [dma_buf_fd() fixes; no preferences regarding the tree it goes through -
> > up to xen folks]
> 
> I'll take it via the Xen tree.

Thanks, I'm dropping that one on my side...