Skip to contentSkip to conversation

dma-buf: fix UAF in dma_buf_fd() tracepoint

PATCH In review David Carlier 3 participants 3 messages May 23 – May 28

Subsystems (3)
Stable branch Media (V4L/DVB) DMA buffer sharing framework
lore
Share & tools
lore

David Carlier

author
#0 lore raw

PATCH 1/1 dma-buf: fix UAF in dma_buf_fd() tracepoint

Once FD_ADD() returns, the fd is live in the file descriptor table and a thread sharing that table can close() it before DMA_BUF_TRACE() runs. The close drops the last reference, __fput() frees the dma_buf, and the tracepoint then dereferences dmabuf to take dmabuf->name_lock -- slab-use-after-free.

Split FD_ADD() back into get_unused_fd_flags() + fd_install() and emit the tracepoint between them. While the fdtable slot is reserved with a NULL file pointer, a racing close() returns -EBADF without entering __fput(), so the dma_buf stays alive across the trace. Same approach as commit 2d76319c4cbb ("dma-buf: fix UAF in dma_buf_put() tracepoint").

This undoes the FD_ADD() conversion done in commit 34dfce523c90 ("dma: convert dma_buf_fd() to FD_ADD()"); FD_ADD() has no place to hook the tracepoint safely.

Reported-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=7f4987d0afb97dd090cb
Fixes 281a22631423 ("dma-buf: add some tracepoints to debug.")
Cc [email protected] # 7.0.x
Signed-off-by David Carlier <[email protected]>
---
 drivers/dma-buf/dma-buf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/dma-buf/dma-buf.c +5 -1
index 71f37544a5c6..d504c636dc29 100644
@@ -792,9 +792,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags)
 	if (!dmabuf || !dmabuf->file)
 		return -EINVAL;
 
-	fd = FD_ADD(flags, dmabuf->file);
+	fd = get_unused_fd_flags(flags);
+	if (fd < 0)
+		return fd;
+
 	DMA_BUF_TRACE(trace_dma_buf_fd, dmabuf, fd);
 
+	fd_install(fd, dmabuf->file);
 	return fd;
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");
Signature · 1 line
2.53.0
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 17 lines · David Carlier

Once FD_ADD() returns, the fd is live in the file descriptor table and a thread sharing that table can close() it before DMA_BUF_TRACE() runs. The close drops the last reference, __fput() frees the dma_buf, and the tracepoint then dereferences dmabuf to take dmabuf->name_lock -- slab-use-after-free.

Split FD_ADD() back into get_unused_fd_flags() + fd_install() and emit the tracepoint between them. While the fdtable slot is reserved with a NULL file pointer, a racing close() returns -EBADF without entering __fput(), so the dma_buf stays alive across the trace. Same approach as commit 2d76319c4cbb ("dma-buf: fix UAF in dma_buf_put() tracepoint").

This undoes the FD_ADD() conversion done in commit 34dfce523c90 ("dma: convert dma_buf_fd() to FD_ADD()"); FD_ADD() has no place to hook the tracepoint safely.

Quoted 5 lines
Reported-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=7f4987d0afb97dd090cb
Fixes 281a22631423 ("dma-buf: add some tracepoints to debug.")
Cc [email protected] # 7.0.x
Signed-off-by David Carlier <[email protected]>
Reviewed-by Christian König <[email protected]>
Quoted 26 lines
---
 drivers/dma-buf/dma-buf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/dma-buf/dma-buf.c +5 -2
index 71f37544a5c6..d504c636dc29 100644
@@ -792,9 +792,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags)
        if (!dmabuf || !dmabuf->file)
                return -EINVAL;
 
-       fd = FD_ADD(flags, dmabuf->file);
+       fd = get_unused_fd_flags(flags);
+       if (fd < 0)
+               return fd;
+
        DMA_BUF_TRACE(trace_dma_buf_fd, dmabuf, fd);
 
+       fd_install(fd, dmabuf->file);
        return fd;
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");
--

2.53.0

maintainer
#2 lore raw

Hi David,

Quoted 19 lines · Christian König
Quoted 17 lines · David Carlier

Once FD_ADD() returns, the fd is live in the file descriptor table and a thread sharing that table can close() it before DMA_BUF_TRACE() runs. The close drops the last reference, __fput() frees the dma_buf, and the tracepoint then dereferences dmabuf to take dmabuf->name_lock -- slab-use-after-free.

Split FD_ADD() back into get_unused_fd_flags() + fd_install() and emit the tracepoint between them. While the fdtable slot is reserved with a NULL file pointer, a racing close() returns -EBADF without entering __fput(), so the dma_buf stays alive across the trace. Same approach as commit 2d76319c4cbb ("dma-buf: fix UAF in dma_buf_put() tracepoint").

This undoes the FD_ADD() conversion done in commit 34dfce523c90 ("dma: convert dma_buf_fd() to FD_ADD()"); FD_ADD() has no place to hook the tracepoint safely.

Quoted 7 lines
Quoted 5 lines
Reported-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=7f4987d0afb97dd090cb
Fixes 281a22631423 ("dma-buf: add some tracepoints to debug.")
Cc [email protected] # 7.0.x
Signed-off-by David Carlier <[email protected]>
Reviewed-by Christian König <[email protected]>

Thanks very much for the patch; applied to drm-misc-fixes.

Quoted 20 lines
Quoted 20 lines
---
 drivers/dma-buf/dma-buf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/dma-buf/dma-buf.c +4 -1
index 71f37544a5c6..d504c636dc29 100644
@@ -792,9 +792,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags)
        if (!dmabuf || !dmabuf->file)
                return -EINVAL;
 
-       fd = FD_ADD(flags, dmabuf->file);
+       fd = get_unused_fd_flags(flags);
+       if (fd < 0)
+               return fd;
+
        DMA_BUF_TRACE(trace_dma_buf_fd, dmabuf, fd);
Quoted 8 lines
Quoted 7 lines
drivers/dma-buf/dma-buf.c +1 -1
+       fd_install(fd, dmabuf->file);
        return fd;
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");
--

2.53.0

Best,
Sumit.