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: [email protected] # v6.9+
Assisted-by: LLM
Signed-off-by: Michal TOMA <[email protected]>
---
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
