* [PATCH v2 1/3] drm/vmwgfx: Don't map or free the exporter's sg_table for imported BOs
2026-09-10 16:52 [PATCH v2 0/3] drm/vmwgfx: Fix dma-buf import leaks and a crash on imported BOs Michal TOMA
@ 2026-09-10 16:52 ` Michal TOMA
2026-09-10 16:52 ` [PATCH v2 2/3] drm/vmwgfx: Release PRIME import in the BO destroy path Michal TOMA
2026-09-10 16:52 ` [PATCH v2 3/3] drm/vmwgfx: Don't leak a GEM handle when referencing a surface by fd Michal TOMA
2 siblings, 0 replies; 6+ messages in thread
From: Michal TOMA @ 2026-09-10 16:52 UTC (permalink / raw)
To: Zack Rusin
Cc: Michal TOMA, bcm-kernel-feedback-list, ian.forbes,
maarten.lankhorst, mripard, tzimmermann, airlied, simona,
sumit.semwal, christian.koenig, dri-devel, linux-kernel,
linux-media, linaro-mm-sig, stable
For a TTM tt with TTM_TT_FLAG_EXTERNAL, vmw_ttm_map_dma() points
vsgt->sgt at the exporter's sg_table and then calls
vmw_ttm_map_for_dma(), which maps &vmw_tt->sgt: the inline table, not
the one vsgt->sgt points to. For an imported buffer object that inline
table is never populated, since vmw_ttm_tt_create() allocates the
vmw_ttm_tt with kzalloc() and only the non-external branch fills it in.
dma_map_sgtable() is therefore called with orig_nents == 0, which trips
the WARN_ON_ONCE() in __dma_map_sg_attrs() and returns -EIO.
The error path then calls sg_free_table() on vsgt->sgt, which for an
imported object is the exporter's table. Its scatterlist is freed while
the attachment is still live, leaving sgl == NULL and orig_nents
unchanged. vmw_ttm_unmap_dma() would free that table too, if a mapping
ever succeeded.
Any process that can open the render node reaches this with three
DRM_RENDER_ALLOW ioctls: import a dma-buf with
DRM_IOCTL_PRIME_FD_TO_HANDLE, create a guest-backed surface on it with
DRM_VMW_GB_SURFACE_CREATE_EXT, and submit SVGA_3D_CMD_UPDATE_GB_SURFACE
for that surface with DRM_VMW_EXECBUF. Validation moves the buffer to
VMW_BO_DOMAIN_MOB, and binding the tt takes the path above:
WARNING: kernel/dma/mapping.c:266 at __dma_map_sg_attrs+0xdd/0x1d0
dma_map_sgtable+0x1d/0x30
vmw_ttm_map_dma+0xf6/0x140 [vmwgfx]
vmw_move+0x1cd/0x2c0 [vmwgfx]
ttm_bo_handle_move_mem+0xc0/0x180 [ttm]
ttm_bo_validate+0xd2/0x1d0 [ttm]
vmw_validation_bo_validate+0xb5/0x180 [vmwgfx]
vmw_execbuf_process+0x852/0x1330 [vmwgfx]
vmw_execbuf_ioctl+0x10d/0x1d0 [vmwgfx]
drm_ioctl_kernel+0xa6/0x100
drm_ioctl+0x2ad/0x590
__x64_sys_ioctl+0xb9/0x100
vmwgfx 0000:00:02.0: [drm] VSG table map failed!
Today the damage stops there, because vmwgfx never releases a PRIME
import: nothing unmaps the attachment afterwards, so the freed table is
only leaked. Adding the missing drm_prime_gem_destroy() call to
vmw_bo_free(), which is the next patch in this series, makes the
exporter unmap that table on release, and dma_unmap_sgtable() then
walks a NULL scatterlist:
BUG: kernel NULL pointer dereference, address: 000000000000001c
Workqueue: ttm ttm_bo_delayed_delete [ttm]
RIP: 0010:dma_direct_unmap_sg+0x62/0x200
unmap_udmabuf+0x24/0x40
dma_buf_unmap_attachment_unlocked+0x46/0x70
drm_prime_gem_destroy+0x28/0x50
vmw_bo_free+0x15b/0x1f0 [vmwgfx]
The exporter has already mapped the table for this device in
dma_buf_map_attachment(), so there is nothing for vmwgfx to do here:
use the table as it is, and leave mapping, unmapping and freeing to its
owner.
Build tested on drm-misc-fixes (4600b4d1a9ee) with W=1 (no warnings in
drivers/gpu/drm/vmwgfx/) and checkpatch.pl --strict. Runtime tested on
a VirtualBox 7.2 VMSVGA guest running kernel 7.2.3, whose vmwgfx
sources for the files involved are identical to drm-misc-fixes, using a
module built from them.
Without this patch the reproducer above logs the WARN_ON_ONCE() and
"VSG table map failed!", and DRM_VMW_EXECBUF returns -EIO; releasing the
buffer afterwards, with the next patch of this series applied, oopsed in
the TTM delete worker and hung the machine.
With this patch the same reproducer logs nothing: the execbuf is
accepted, so an imported buffer object can now be bound at all, and
releasing it afterwards leaves no entry behind in
/sys/kernel/debug/dma_buf/bufinfo.
Fixes: b32233acceff ("drm/vmwgfx: Fix prime import/export")
Cc: stable@vger.kernel.org # v6.9+
Assisted-by: LLM
Signed-off-by: Michal TOMA <michaltoma@sicoop.com>
---
Reproducer (results and environment are in the cover letter). Run as an
ordinary user with access to the render node and /dev/udmabuf; it needs
no root. Without this patch it logs the WARN and "VSG table map
failed!"; with it, the execbuf is accepted and nothing is logged.
// Can userspace get an imported (external) dma-buf BO bound for the device?
//
// Chain under test:
// udmabuf -> PRIME_FD_TO_HANDLE (external BO, sits in SYSTEM)
// GB_SURFACE_CREATE_EXT buffer_handle=... (surface backed by the import)
// execbuf SVGA_3D_CMD_UPDATE_GB_SURFACE (validation forces the BO to MOB)
// -> vmw_ttm_bind() -> vmw_ttm_map_dma()
// external branch maps the wrong (empty, inline) sg_table, fails, and
// the out_map_fail path calls sg_free_table() on the EXPORTER's table.
//
// Proof of reachability is the kernel message "VSG table map failed!".
//
// vmw-import-execbuf-bind-test hold the buffer, never release it
// vmw-import-execbuf-bind-test --release also close everything at the end
//
// WARNING: --release is the dangerous half. With "drm/vmwgfx: Release PRIME
// import in the BO destroy path" applied, releasing a buffer whose exporter
// sg_table was freed in step 3 makes udmabuf run dma_unmap_sgtable() on a
// table with sgl == NULL and orig_nents != 0 -> NULL pointer dereference.
// Without that patch the release path never touches the table (it leaks).
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <linux/udmabuf.h>
#include <drm/drm.h>
#include <drm/vmwgfx_drm.h>
#define DRM_IOCTL_VMW_GB_SURFACE_CREATE_EXT \
DRM_IOWR(DRM_COMMAND_BASE + DRM_VMW_GB_SURFACE_CREATE_EXT, \
union drm_vmw_gb_surface_create_ext_arg)
#define DRM_IOCTL_VMW_UNREF_SURFACE \
DRM_IOW(DRM_COMMAND_BASE + DRM_VMW_UNREF_SURFACE, struct drm_vmw_surface_arg)
#define DRM_IOCTL_VMW_EXECBUF \
DRM_IOW(DRM_COMMAND_BASE + DRM_VMW_EXECBUF, struct drm_vmw_execbuf_arg)
#define SVGA_3D_CMD_UPDATE_GB_SURFACE 1102
#define SVGA3D_INVALID_ID 0xffffffff
#define SIZE (6UL << 20)
struct update_gb_surface_cmd {
uint32_t id; /* SVGA3dCmdHeader.id */
uint32_t size; /* SVGA3dCmdHeader.size: bytes that follow */
uint32_t sid; /* SVGA3dCmdUpdateGBSurface.sid */
};
int main(int argc, char **argv)
{
int release = (argc > 1 && !strcmp(argv[1], "--release"));
int render = open("/dev/dri/renderD128", O_RDWR | O_CLOEXEC);
int udm = open("/dev/udmabuf", O_RDWR | O_CLOEXEC);
int mfd, dfd;
if (render < 0 || udm < 0) {
perror("open");
return 1;
}
mfd = memfd_create("import-execbuf", MFD_ALLOW_SEALING | MFD_CLOEXEC);
if (mfd < 0 || ftruncate(mfd, SIZE) ||
fcntl(mfd, F_ADD_SEALS, F_SEAL_SHRINK)) {
perror("memfd");
return 1;
}
struct udmabuf_create c = {
.memfd = mfd, .flags = UDMABUF_FLAGS_CLOEXEC, .size = SIZE,
};
dfd = ioctl(udm, UDMABUF_CREATE, &c);
if (dfd < 0) {
perror("UDMABUF_CREATE");
return 1;
}
struct drm_prime_handle ph = { .fd = dfd };
if (ioctl(render, DRM_IOCTL_PRIME_FD_TO_HANDLE, &ph)) {
perror("PRIME_FD_TO_HANDLE");
return 1;
}
printf("1. imported udmabuf as GEM handle %u\n", ph.handle);
union drm_vmw_gb_surface_create_ext_arg a;
memset(&a, 0, sizeof(a));
a.req.version = drm_vmw_gb_surface_v1;
/* HINT_TEXTURE | HINT_RENDERTARGET | BIND_SHADER_RESOURCE | BIND_RENDER_TARGET */
a.req.base.svga3d_flags = (1U << 5) | (1U << 6) | (1U << 23) | (1U << 24);
a.req.base.format = 142; /* SVGA3D_B8G8R8X8_UNORM */
a.req.base.mip_levels = 1;
a.req.base.drm_surface_flags = drm_vmw_surface_flag_shareable;
a.req.base.buffer_handle = ph.handle;
a.req.base.base_size.width = 1536;
a.req.base.base_size.height = 1024;
a.req.base.base_size.depth = 1;
if (ioctl(render, DRM_IOCTL_VMW_GB_SURFACE_CREATE_EXT, &a)) {
printf("2. GB_SURFACE_CREATE_EXT on the imported buffer: FAILED, errno %d (%s)\n",
errno, strerror(errno));
return 1;
}
printf("2. surface backed by the imported buffer: sid %u\n", a.rep.handle);
struct update_gb_surface_cmd cmd = {
.id = SVGA_3D_CMD_UPDATE_GB_SURFACE,
.size = sizeof(uint32_t),
.sid = a.rep.handle,
};
struct drm_vmw_execbuf_arg e;
memset(&e, 0, sizeof(e));
e.commands = (uint64_t)(uintptr_t)&cmd;
e.command_size = sizeof(cmd);
e.version = DRM_VMW_EXECBUF_VERSION;
e.context_handle = SVGA3D_INVALID_ID;
errno = 0;
if (ioctl(render, DRM_IOCTL_VMW_EXECBUF, &e))
printf("3. execbuf UPDATE_GB_SURFACE: returned errno %d (%s)\n",
errno, strerror(errno));
else
printf("3. execbuf UPDATE_GB_SURFACE: accepted\n");
printf(" -> now check the kernel log for \"VSG table map failed!\"\n");
if (!release) {
printf("4. holding the buffer open (no release). Ctrl-C or kill to end;\n"
" note that ending this process frees the BO, which is the\n"
" step that can oops with the BO-destroy fix applied.\n");
fflush(stdout);
pause();
return 0;
}
printf("4. --release: dropping the surface, the handle and every fd\n");
fflush(stdout);
struct drm_vmw_surface_arg u = { .sid = a.rep.handle, .handle_type = DRM_VMW_HANDLE_LEGACY };
ioctl(render, DRM_IOCTL_VMW_UNREF_SURFACE, &u);
struct drm_gem_close gc = { .handle = ph.handle };
ioctl(render, DRM_IOCTL_GEM_CLOSE, &gc);
close(dfd);
close(mfd);
close(udm);
close(render);
sleep(1);
printf("5. released, still alive\n");
return 0;
}
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
index dfd08ee19..3e8bdf246 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
@@ -189,6 +189,11 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt)
case vmw_dma_map_bind:
case vmw_dma_map_populate:
if (vmw_tt->dma_ttm.page_flags & TTM_TT_FLAG_EXTERNAL) {
+ /*
+ * The exporter has already mapped its sg_table for
+ * this device in dma_buf_map_attachment(). Use it as
+ * it is: it is not ours to map, unmap or free.
+ */
vsgt->sgt = vmw_tt->dma_ttm.sg;
} else {
vsgt->sgt = &vmw_tt->sgt;
@@ -199,11 +204,11 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt)
GFP_KERNEL);
if (ret)
goto out_sg_alloc_fail;
- }
- ret = vmw_ttm_map_for_dma(vmw_tt);
- if (unlikely(ret != 0))
- goto out_map_fail;
+ ret = vmw_ttm_map_for_dma(vmw_tt);
+ if (unlikely(ret != 0))
+ goto out_map_fail;
+ }
break;
default:
@@ -237,6 +242,13 @@ static void vmw_ttm_unmap_dma(struct vmw_ttm_tt *vmw_tt)
if (!vmw_tt->vsgt.sgt)
return;
+ if (vmw_tt->dma_ttm.page_flags & TTM_TT_FLAG_EXTERNAL) {
+ /* The mapping and the table belong to the exporter. */
+ vmw_tt->vsgt.sgt = NULL;
+ vmw_tt->mapped = false;
+ return;
+ }
+
switch (dev_priv->map_mode) {
case vmw_dma_map_bind:
case vmw_dma_map_populate:
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/3] drm/vmwgfx: Release PRIME import in the BO destroy path
2026-09-10 16:52 [PATCH v2 0/3] drm/vmwgfx: Fix dma-buf import leaks and a crash on imported BOs Michal TOMA
2026-09-10 16:52 ` [PATCH v2 1/3] drm/vmwgfx: Don't map or free the exporter's sg_table for " Michal TOMA
@ 2026-09-10 16:52 ` Michal TOMA
2026-09-10 17:44 ` sashiko-bot
2026-09-10 16:52 ` [PATCH v2 3/3] drm/vmwgfx: Don't leak a GEM handle when referencing a surface by fd Michal TOMA
2 siblings, 1 reply; 6+ messages in thread
From: Michal TOMA @ 2026-09-10 16:52 UTC (permalink / raw)
To: Zack Rusin
Cc: Michal TOMA, bcm-kernel-feedback-list, ian.forbes,
maarten.lankhorst, mripard, tzimmermann, airlied, simona,
sumit.semwal, christian.koenig, dri-devel, linux-kernel,
linux-media, linaro-mm-sig, stable
drm_gem_prime_import_dev() attaches to a foreign dma-buf, takes a
reference with get_dma_buf(), maps the attachment and, once
vmw_prime_import_sg_table() has created the TTM buffer object, stores
the attachment in obj->import_attach. Drivers that import this way have
to undo it by calling drm_prime_gem_destroy() when the object is freed.
vmwgfx never does. The TTM destroy callback, vmw_bo_free(), only calls
drm_gem_object_release() and kfree(). For every imported dma-buf, the
sg mapping, the attachment and the dma-buf reference are leaked when
the last GEM reference goes away, and the exporter's backing pages stay
pinned until reboot.
Any process that can open the vmwgfx render node can hit this by
importing a dma-buf from another exporter, for example with
DRM_IOCTL_PRIME_FD_TO_HANDLE. It showed up as a memory drain on a
VirtualBox VMSVGA guest running a Plasma 6.7 Wayland session with the
host's 3D acceleration off. KWin 6.7 wraps wl_shm client buffers in
udmabufs and imports them through EGL on llvmpipe, and it keeps many of
those imports referenced while it runs, which is a separate userspace
problem. Restarting the compositor did not give the memory back,
though: with about 1.5 GiB of client buffers imported, killing KWin
left 211 udmabufs in /sys/kernel/debug/dma_buf/bufinfo with a refcount
of 1, still attached to the vmwgfx device, after every userspace
reference was gone. Their pages are no longer mapped or in any page
cache, so reclaim and swap cannot free them.
Call drm_prime_gem_destroy() for imported objects before
drm_gem_object_release(), as amdgpu, radeon and nouveau do in their TTM
destroy callbacks, and include <drm/drm_prime.h> for it. The check is
safe on the import error path: drm_gem_prime_import_dev() only sets
obj->import_attach after gem_prime_import_sg_table() succeeded, so a
buffer object destroyed before that point is not unmapped, detached or
put a second time. ttm_bo_release() tears down the TTM backing and
drops the reservation lock before calling the destroy callback, which
fits the _unlocked unmap done by drm_prime_gem_destroy().
The leak was found by walking /proc/kpageflags, which attributed the
missing memory to orphaned shmem pages. Those matched the udmabuf
objects in dma_buf/bufinfo, and the remaining reference was traced to
the missing PRIME teardown. It was confirmed without a compositor using
a small reproducer: create a memfd, wrap it with UDMABUF_CREATE, import
it with DRM_IOCTL_PRIME_FD_TO_HANDLE on the vmwgfx render node, close
the GEM handle and every file descriptor, then check bufinfo.
Build tested on drm-misc-fixes (4600b4d1a9ee) with the openSUSE 7.2.3
config and W=1: drivers/gpu/drm/vmwgfx/ builds without warnings before
and after the change, and checkpatch.pl --strict is clean. Runtime
tested on a VirtualBox 7.2 VMSVGA guest with kernel 7.2.3, whose vmwgfx
sources for the files involved are identical to drm-misc-fixes, using a
vmwgfx.ko built from them with this change. With the reproducer, all 8
imported udmabufs stayed pinned without the change and none with it,
with 3D acceleration both on and off. With KWin on llvmpipe, killing
the compositor now released 84 of 92 udmabufs (610 MiB) within 10
seconds, where the same test on the unpatched driver released none.
Fixes: b32233acceff ("drm/vmwgfx: Fix prime import/export")
Cc: stable@vger.kernel.org # v6.6+
Assisted-by: LLM
Signed-off-by: Michal TOMA <michaltoma@sicoop.com>
---
Reproducer (results and environment are in the cover letter). Compare
the 4 MiB udmabuf entries in /sys/kernel/debug/dma_buf/bufinfo before
and after: without this patch all 8 remain, with count 1 and still
attached to the vmwgfx device.
#!/usr/bin/env python3
import fcntl, os, struct
N, SIZE = 8, 4 * 1024 * 1024
UDMABUF_CREATE = 0x40187542 # _IOW('u', 0x42, struct udmabuf_create)
PRIME_FD_TO_HANDLE = 0xC00C642E # DRM_IOWR(0x2e, struct drm_prime_handle)
GEM_CLOSE = 0x40086409 # DRM_IOW(0x09, struct drm_gem_close)
render = os.open('/dev/dri/renderD128', os.O_RDWR | os.O_CLOEXEC)
udm = os.open('/dev/udmabuf', os.O_RDWR | os.O_CLOEXEC)
for i in range(N):
mfd = os.memfd_create(f'prime-leak-{i}', os.MFD_ALLOW_SEALING)
os.ftruncate(mfd, SIZE)
fcntl.fcntl(mfd, fcntl.F_ADD_SEALS, fcntl.F_SEAL_SHRINK)
create = bytearray(struct.pack('IIQQ', mfd, 1, 0, SIZE))
dfd = fcntl.ioctl(udm, UDMABUF_CREATE, create)
ph = bytearray(struct.pack('IIi', 0, 0, dfd))
fcntl.ioctl(render, PRIME_FD_TO_HANDLE, ph)
handle = struct.unpack('IIi', ph)[0]
fcntl.ioctl(render, GEM_CLOSE, bytearray(struct.pack('II', handle, 0)))
os.close(dfd)
os.close(mfd)
os.close(udm)
os.close(render)
The same loop without the PRIME_FD_TO_HANDLE/GEM_CLOSE step frees
every udmabuf, so the leak is specific to the vmwgfx import.
drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 9c7a73c0b..56bc94edc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -30,6 +30,7 @@
#include "vmwgfx_drv.h"
#include "vmwgfx_resource_priv.h"
+#include <drm/drm_prime.h>
#include <drm/ttm/ttm_placement.h>
/**
@@ -69,6 +70,8 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
vmw_surface_unreference(&vbo->dumb_surface);
}
WARN_ON(!RB_EMPTY_ROOT(&vbo->res_tree));
+ if (drm_gem_is_imported(&vbo->tbo.base))
+ drm_prime_gem_destroy(&vbo->tbo.base, vbo->tbo.sg);
drm_gem_object_release(&vbo->tbo.base);
WARN_ON(vbo->dirty);
kfree(vbo);
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/3] drm/vmwgfx: Release PRIME import in the BO destroy path
2026-09-10 16:52 ` [PATCH v2 2/3] drm/vmwgfx: Release PRIME import in the BO destroy path Michal TOMA
@ 2026-09-10 17:44 ` sashiko-bot
0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-09-10 17:44 UTC (permalink / raw)
To: Michal TOMA; +Cc: media-ci, dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] NULL pointer dereference when dma_buf import fails.
--
commit 200f36633d8d14d0ae43912ea8f089cba49bace3
Author: Michal TOMA <michaltoma@sicoop.com>
drm/vmwgfx: Release PRIME import in the BO destroy path
This patch fixes a resource leak in the vmwgfx driver where PRIME imported
dma-bufs are not properly torn down when the buffer object is destroyed. It
adds a call to drm_prime_gem_destroy() in vmw_bo_free() to ensure the
attachment and sg mapping are released.
> drm_gem_prime_import_dev() attaches to a foreign dma-buf, takes a
> reference with get_dma_buf(), maps the attachment and, once
> vmw_prime_import_sg_table() has created the TTM buffer object, stores
> the attachment in obj->import_attach. Drivers that import this way have
> to undo it by calling drm_prime_gem_destroy() when the object is freed.
[Severity: High]
This is a pre-existing issue, but since we are looking at the dma_buf import
path, could a failed import trigger a NULL pointer dereference here?
When a process imports a dma_buf via DRM_IOCTL_PRIME_FD_TO_HANDLE on the
vmwgfx device, the DRM core calls vmw_prime_import_sg_table(). If BO creation
fails, the function returns NULL instead of an ERR_PTR:
drivers/gpu/drm/vmwgfx/vmwgfx_gem.c:vmw_prime_import_sg_table() {
...
out_no_bo:
dma_resv_unlock(params.resv);
return gem;
}
In the DRM core, drm_gem_prime_import_dev() only checks for IS_ERR(),
which is false for NULL:
drivers/gpu/drm/drm_prime.c:drm_gem_prime_import_dev() {
...
obj = dev->driver->gem_prime_import_sg_table(dev, attach, sgt);
if (IS_ERR(obj)) {
ret = PTR_ERR(obj);
goto fail_unmap;
}
obj->import_attach = attach;
...
}
Does this mean an unprivileged user could crash the kernel by forcing an
import failure (e.g. by exhausting memory or providing an excessive size)?
Should vmw_prime_import_sg_table() return ERR_PTR(-ENOMEM) instead of NULL
on failure?
[ ... ]
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index 9c7a73c0b0dc3..56bc94edc30b7 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
[ ... ]
> @@ -69,6 +70,8 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
> vmw_surface_unreference(&vbo->dumb_surface);
> }
> WARN_ON(!RB_EMPTY_ROOT(&vbo->res_tree));
> + if (drm_gem_is_imported(&vbo->tbo.base))
> + drm_prime_gem_destroy(&vbo->tbo.base, vbo->tbo.sg);
> drm_gem_object_release(&vbo->tbo.base);
> WARN_ON(vbo->dirty);
> kfree(vbo);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910165221.7558-1-michaltoma@sicoop.com?part=2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] drm/vmwgfx: Don't leak a GEM handle when referencing a surface by fd
2026-09-10 16:52 [PATCH v2 0/3] drm/vmwgfx: Fix dma-buf import leaks and a crash on imported BOs Michal TOMA
2026-09-10 16:52 ` [PATCH v2 1/3] drm/vmwgfx: Don't map or free the exporter's sg_table for " Michal TOMA
2026-09-10 16:52 ` [PATCH v2 2/3] drm/vmwgfx: Release PRIME import in the BO destroy path Michal TOMA
@ 2026-09-10 16:52 ` Michal TOMA
2026-09-10 17:49 ` sashiko-bot
2 siblings, 1 reply; 6+ messages in thread
From: Michal TOMA @ 2026-09-10 16:52 UTC (permalink / raw)
To: Zack Rusin
Cc: Michal TOMA, bcm-kernel-feedback-list, ian.forbes,
maarten.lankhorst, mripard, tzimmermann, airlied, simona,
sumit.semwal, christian.koenig, dri-devel, linux-kernel,
linux-media, linaro-mm-sig, stable
DRM_VMW_GB_SURFACE_REF_EXT with DRM_VMW_HANDLE_PRIME first tries
ttm_prime_fd_to_handle(). If the fd is not a vmwgfx surface export,
vmw_surface_handle_reference() falls back to
vmw_buffer_prime_to_surface_base(). That function calls
drm_gem_prime_fd_to_handle() to turn the fd into a GEM handle in the
caller's file, then looks for a surface attached to that buffer.
That handle is never returned to userspace, since the ioctl reports the
surface handle and a separate backup buffer handle, and it is never
deleted. Every call that gets this far therefore leaves a GEM handle
behind in the file, whether it fails or succeeds, and whatever the
handle references stays alive until the file is closed.
For a dma-buf from another exporter, such as udmabuf, this is an
actual import: drm_gem_prime_fd_to_handle() attaches to the dma-buf,
maps it and creates a new GEM object. The surface lookup then always
fails, because such a buffer never has a surface. The WARN_ON() fires
and the ioctl returns -EINVAL, but the import stays referenced by the
stray handle.
Mesa's svga winsys passes dma-buf fds to DRM_VMW_GB_SURFACE_REF_EXT as
DRM_VMW_HANDLE_PRIME, and KWin 6.7 imports every wl_shm client buffer
as a udmabuf through EGL. On a Plasma Wayland session this pins one
window-sized buffer inside the compositor's DRM file for every
attempted import, for as long as the compositor runs, and logs a full
WARN backtrace each time. On a VirtualBox VMSVGA guest with 3D
acceleration on and the previous patch applied, 103 udmabufs (750 MiB)
had accumulated this way 40 minutes after boot. 99 of them had a
dma-buf refcount of 3, were still attached to the vmwgfx device and
were held by no process fd or mapping.
Look the buffer up through the dma-buf instead of importing it. Only
GEM buffers exported by this device can have a surface, so reject
anything else with drm_gem_is_prime_exported_dma_buf() before touching
it. For our own buffers, use dma_buf->priv directly: the dma-buf holds
a reference to the GEM object for as long as we hold the dma-buf, so
neither a handle nor an extra object reference is needed. The surface
lookup and the ttm_ref_object_add() reference that the callers rely on
are unchanged.
While restructuring the function, also:
- replace the WARN_ON() on a buffer without a surface with a debug
message. Userspace can trigger that case at will.
- drop the base object reference taken by
vmw_lookup_user_surface_for_buffer() when ttm_ref_object_add() fails.
It was leaked before.
Build tested on drm-misc-fixes (4600b4d1a9ee) with W=1 (no warnings in
drivers/gpu/drm/vmwgfx/) and checkpatch.pl --strict. Runtime tested on
the same guest and 7.2.3 kernel as the previous patch, comparing a
vmwgfx.ko with only the previous patch against one with both. Calls
from a second render file:
- udmabuf fd: -EINVAL both times. Before, each call left the import
held by the file (refcount 3, attached) and logged a WARN. Now
nothing is imported and nothing is logged.
- fd of a dumb buffer exported from the primary node: -EINVAL both
times. Before, a stray GEM handle and a WARN per call; now neither.
- fd of a buffer exported before a GB surface was created on it, which
reaches this function and succeeds: the exporter's surface is
returned both times. Before, each call left a GEM handle in the
file; now it does not.
- fd of a buffer created together with its surface, which is exported
as a TTM prime surface and does not reach this function: succeeds
both times.
During a 5-minute KWin 6.7 test replaying a terminal workload, the
udmabuf count went from 4 to 20 (140 MiB) with only the previous patch,
with WARNs logged. With both patches it stayed at 6 during the run,
returned to 4 afterwards, and no WARN was logged.
Fixes: d6667f0ddf46 ("drm/vmwgfx: Fix handling of dumb buffers")
Cc: stable@vger.kernel.org # v6.11+
Assisted-by: LLM
Signed-off-by: Michal TOMA <michaltoma@sicoop.com>
---
Reproducer (results and environment are in the cover letter). Run as
root, since it counts udmabufs in debugfs:
cc -o vmwgfx-surfref-fd-leak vmwgfx-surfref-fd-leak.c
./vmwgfx-surfref-fd-leak
// SPDX-License-Identifier: MIT
/*
* Reproducer for "drm/vmwgfx: Don't leak a GEM handle when referencing a
* surface by fd": DRM_VMW_GB_SURFACE_REF_EXT(DRM_VMW_HANDLE_PRIME) with a
* udmabuf fd. Run as root, since it counts udmabufs in debugfs.
*
* cc -o vmwgfx-surfref-fd-leak vmwgfx-surfref-fd-leak.c && ./vmwgfx-surfref-fd-leak
*
* The render node stays open after the calls. Expected lines 2 and 3
* (udmabufs left while the node is open / after it is closed):
* with this fix: 0 / 0
* without it: 8 / 0 if "drm/vmwgfx: Release PRIME import in
* the BO destroy path" is applied, otherwise
* 8 / 8 (pinned until reboot)
*/
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <linux/udmabuf.h>
#include <drm/drm.h>
#include <drm/vmwgfx_drm.h>
#define DRM_IOCTL_VMW_GB_SURFACE_REF_EXT \
DRM_IOWR(DRM_COMMAND_BASE + DRM_VMW_GB_SURFACE_REF_EXT, \
union drm_vmw_gb_surface_reference_ext_arg)
#define N 8
#define SIZE (6UL << 20)
/* udmabufs of @size listed in dma_buf/bufinfo */
static int count_udmabufs(unsigned long size)
{
FILE *f = fopen("/sys/kernel/debug/dma_buf/bufinfo", "r");
char line[512];
int n = 0;
if (!f) {
perror("/sys/kernel/debug/dma_buf/bufinfo");
return -1;
}
while (fgets(line, sizeof(line), f)) {
unsigned long sz;
if (sscanf(line, "%lu", &sz) == 1 && sz == size &&
strstr(line, "udmabuf"))
n++;
}
fclose(f);
return n;
}
int main(void)
{
int render = open("/dev/dri/renderD128", O_RDWR | O_CLOEXEC);
int udm = open("/dev/udmabuf", O_RDWR | O_CLOEXEC);
int before, einval = 0;
if (render < 0 || udm < 0) {
perror("open");
return 1;
}
before = count_udmabufs(SIZE);
for (int i = 0; i < N; i++) {
int memfd = memfd_create("surfref", MFD_ALLOW_SEALING | MFD_CLOEXEC);
struct udmabuf_create create = {
.memfd = memfd, .flags = UDMABUF_FLAGS_CLOEXEC, .size = SIZE,
};
union drm_vmw_gb_surface_reference_ext_arg arg;
int dmabuf;
if (memfd < 0 || ftruncate(memfd, SIZE) ||
fcntl(memfd, F_ADD_SEALS, F_SEAL_SHRINK)) {
perror("memfd");
return 1;
}
dmabuf = ioctl(udm, UDMABUF_CREATE, &create);
if (dmabuf < 0) {
perror("UDMABUF_CREATE");
return 1;
}
memset(&arg, 0, sizeof(arg));
arg.req.sid = dmabuf;
arg.req.handle_type = DRM_VMW_HANDLE_PRIME;
if (ioctl(render, DRM_IOCTL_VMW_GB_SURFACE_REF_EXT, &arg) &&
errno == EINVAL)
einval++;
close(dmabuf);
close(memfd);
}
close(udm);
usleep(500000);
printf("GB_SURFACE_REF_EXT with a udmabuf fd: %d/%d calls failed with EINVAL\n",
einval, N);
printf("6 MiB udmabufs left, render node open: %d (before: %d)\n",
count_udmabufs(SIZE), before);
close(render);
usleep(500000);
printf("6 MiB udmabufs left, render node closed: %d\n",
count_udmabufs(SIZE));
return 0;
}
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 48 ++++++++++++++-----------
1 file changed, 28 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index bd0563741..27f68fd9c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -16,8 +16,11 @@
#include "device_include/svga3d_surfacedefs.h"
#include <drm/drm_dumb_buffers.h>
+#include <drm/drm_prime.h>
#include <drm/ttm/ttm_placement.h>
+#include <linux/dma-buf.h>
+
#define SVGA3D_FLAGS_64(upper32, lower32) (((uint64_t)upper32 << 32) | lower32)
/**
@@ -931,33 +934,37 @@ u32 vmw_lookup_surface_handle_for_buffer(struct vmw_private *vmw,
static int vmw_buffer_prime_to_surface_base(struct vmw_private *dev_priv,
struct drm_file *file_priv,
- u32 fd, u32 *handle,
+ u32 fd,
struct ttm_base_object **base_p)
{
struct ttm_base_object *base;
- struct vmw_bo *bo;
+ struct dma_buf *dma_buf;
struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
struct vmw_user_surface *user_srf;
int ret;
- ret = drm_gem_prime_fd_to_handle(&dev_priv->drm, file_priv, fd, handle);
- if (ret) {
- drm_warn(&dev_priv->drm,
- "Wasn't able to find user buffer for fd = %u.\n", fd);
- return ret;
- }
+ dma_buf = dma_buf_get(fd);
+ if (IS_ERR(dma_buf))
+ return PTR_ERR(dma_buf);
- ret = vmw_user_bo_lookup(file_priv, *handle, &bo);
- if (ret) {
- drm_warn(&dev_priv->drm,
- "Wasn't able to lookup user buffer for handle = %u.\n", *handle);
- return ret;
+ /*
+ * Only buffers exported by this device can have a user surface.
+ * Look the buffer up through the dma-buf, which holds a reference
+ * to it, instead of importing the fd into file_priv: the GEM handle
+ * that would create is never returned to userspace, so nothing
+ * would release it (or a foreign dma-buf) until the file is closed.
+ */
+ if (!drm_gem_is_prime_exported_dma_buf(&dev_priv->drm, dma_buf)) {
+ ret = -EINVAL;
+ goto out;
}
- user_srf = vmw_lookup_user_surface_for_buffer(dev_priv, bo, *handle);
- if (WARN_ON(!user_srf)) {
- drm_warn(&dev_priv->drm,
- "User surface fd %d (handle %d) is null.\n", fd, *handle);
+ user_srf = vmw_lookup_user_surface_for_buffer(dev_priv,
+ to_vmw_bo(dma_buf->priv),
+ fd);
+ if (!user_srf) {
+ drm_dbg_driver(&dev_priv->drm,
+ "No user surface for buffer fd %d.\n", fd);
ret = -EINVAL;
goto out;
}
@@ -966,13 +973,15 @@ static int vmw_buffer_prime_to_surface_base(struct vmw_private *dev_priv,
ret = ttm_ref_object_add(tfile, base, NULL, false);
if (ret) {
drm_warn(&dev_priv->drm,
- "Couldn't add an object ref for the buffer (%d).\n", *handle);
+ "Couldn't add an object ref for buffer fd %d (%d).\n",
+ fd, ret);
+ ttm_base_object_unref(&base);
goto out;
}
*base_p = base;
out:
- vmw_user_bo_unref(&bo);
+ dma_buf_put(dma_buf);
return ret;
}
@@ -996,7 +1005,6 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv,
return vmw_buffer_prime_to_surface_base(dev_priv,
file_priv,
u_handle,
- &handle,
base_p);
} else {
handle = u_handle;
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 3/3] drm/vmwgfx: Don't leak a GEM handle when referencing a surface by fd
2026-09-10 16:52 ` [PATCH v2 3/3] drm/vmwgfx: Don't leak a GEM handle when referencing a surface by fd Michal TOMA
@ 2026-09-10 17:49 ` sashiko-bot
0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-09-10 17:49 UTC (permalink / raw)
To: Michal TOMA; +Cc: media-ci, dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Unsynchronized rbtree and xarray traversal in vmw_bo_surface() can lead to crashes or UAF.
- [High] Kernel stack memory leak in `vmw_gb_surface_reference_internal` error path.
--
commit 6ca6707ae183fa071e8ebb217050fe9e1947a0b2
Author: Michal TOMA <michaltoma@sicoop.com>
drm/vmwgfx: Don't leak a GEM handle when referencing a surface by fd
This commit prevents a GEM handle leak by looking up a surface buffer
through its dma-buf directly instead of importing the fd into the DRM file.
It rejects buffers not exported by the device and simplifies object
reference cleanup.
Cc: stable@vger.kernel.org # v6.11+
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index bd0563741e89d..27f68fd9c22bd 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
[ ... ]
> @@ -931,33 +934,37 @@ u32 vmw_lookup_surface_handle_for_buffer(struct vmw_private *vmw,
>
> static int vmw_buffer_prime_to_surface_base(struct vmw_private *dev_priv,
> struct drm_file *file_priv,
> - u32 fd, u32 *handle,
> + u32 fd,
> struct ttm_base_object **base_p)
> {
[ ... ]
> - user_srf = vmw_lookup_user_surface_for_buffer(dev_priv, bo, *handle);
> - if (WARN_ON(!user_srf)) {
> - drm_warn(&dev_priv->drm,
> - "User surface fd %d (handle %d) is null.\n", fd, *handle);
> + user_srf = vmw_lookup_user_surface_for_buffer(dev_priv,
> + to_vmw_bo(dma_buf->priv),
> + fd);
[Severity: Critical]
This is a pre-existing issue, but does this lockless lookup risk a
use-after-free or kernel crash?
When vmw_lookup_user_surface_for_buffer() is called here, it calls
vmw_bo_surface() which iterates over the detached_resources xarray and
res_tree rbtree without holding the dma_resv lock or an RCU read lock:
drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:vmw_bo_surface() {
...
xa_for_each(&vbo->detached_resources, index, res) {
if (res->func->res_type == vmw_res_surface)
goto out;
}
...
for (rb_itr = rb_first(&vbo->res_tree); rb_itr;
rb_itr = rb_next(rb_itr)) {
...
}
If another thread concurrently modifies these structures (for example, by
adding or removing resources), this lockless traversal could access freed
nodes. Should this lookup take the buffer object's dma_resv lock?
> + if (!user_srf) {
> + drm_dbg_driver(&dev_priv->drm,
> + "No user surface for buffer fd %d.\n", fd);
> ret = -EINVAL;
> goto out;
> }
[ ... ]
[Severity: High]
As an additional observation in this file, this is a pre-existing issue,
but does vmw_gb_surface_reference_internal() leak kernel stack memory to
userspace in its error path?
If srf->res.guest_memory_bo is NULL, the error path skips setting a
negative error code and jumps directly to out_bad_resource:
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_internal() {
...
if (!srf->res.guest_memory_bo) {
DRM_ERROR("Shared GB surface is missing a backup buffer.\n");
goto out_bad_resource;
}
...
out_bad_resource:
...
return ret;
}
Because ret is still 0 at this point, the caller
vmw_gb_surface_reference_ioctl() assumes success and copies the
uninitialized stack variable rep_ext into the user-visible rep pointer:
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_ioctl() {
struct drm_vmw_gb_surface_ref_ext_rep rep_ext;
int ret;
ret = vmw_gb_surface_reference_internal(dev, req, &rep_ext, file_priv);
if (unlikely(ret != 0))
return ret;
rep->creq = rep_ext.creq.base;
rep->crep = rep_ext.crep;
}
Could this be fixed by explicitly setting ret = -EINVAL before jumping to
out_bad_resource in vmw_gb_surface_reference_internal()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910165221.7558-1-michaltoma@sicoop.com?part=3
^ permalink raw reply [flat|nested] 6+ messages in thread