drm/vmwgfx: do not hand the embedded sg_table to the PRIME core

[PATCH] drm/vmwgfx: do not hand the embedded sg_table to the PRIME core

Posted by Aldo Ariel Panzardo 23 hours ago

vmw_gem_object_get_sg_table() returns vmw_tt->vsgt.sgt when the buffer
object has already been DMA mapped.  vmw_ttm_map_dma() sets that field
to &vmw_tt->sgt, which is a member embedded inside the struct
vmw_ttm_tt allocation and not a table of its own.

The core owns whatever .get_sg_table returns.  drm_gem_map_dma_buf()
takes the table and drm_gem_unmap_dma_buf() destroys it in full:

	dma_unmap_sgtable(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC);
	sg_free_table(sgt);
	kfree(sgt);

Both are ops of drm_gem_prime_dmabuf_ops, so the core ends up calling
kfree() on &vmw_tt->sgt, which is not the start of an allocation.  The
preceding sg_free_table() also destroys a scatterlist that vmwgfx still
considers its own and frees again from vmw_ttm_unmap_dma().

drm_gem_unmap_dma_buf() runs from dma_buf_detach(), including the
importer's error path, so a failed import is enough to reach it:
exporting a bound buffer with DRM_IOCTL_PRIME_HANDLE_TO_FD and
importing it on a second DRM device with DRM_IOCTL_PRIME_FD_TO_HANDLE
is sufficient.  Both ioctls are DRM_RENDER_ALLOW.

Observed on 6.12.101 with KASAN, as uid 65534:

  BUG: KASAN: invalid-free in drm_gem_unmap_dma_buf+0xb3/0xf0
  Free of addr ffff888008290450 by task poc_mm04_sgtabl/321
  CPU: 1 UID: 65534 PID: 321 Comm: poc_mm04_sgtabl Not tainted 6.12.101 #4
   kasan_report_invalid_free+0x94/0xc0
   check_slab_allocation+0x116/0x120
   kfree+0x103/0x360
   drm_gem_unmap_dma_buf+0xb3/0xf0
   dma_buf_detach+0x165/0x510
   drm_gem_prime_import_dev+0x33e/0x430
  which belongs to the cache kmalloc-192 of size 192
  The buggy address is located 80 bytes inside of
   144-byte region [ffff888008290400, ffff888008290490)

80 is offsetof(struct vmw_ttm_tt, sgt) and 144 is sizeof(struct
vmw_ttm_tt), which identifies the freed pointer as the embedded member.

Always return a table the core can own, as the other drivers do.
Reusing the cached representation would require copying it into a
freshly allocated sg_table, never returning the alias.

Fixes: 8afa13a0583f ("drm/vmwgfx: Implement DRIVER_GEM")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
index 39f8c4655..a0233729b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
@@ -73,10 +73,13 @@ static struct sg_table *vmw_gem_object_get_sg_table(struct drm_gem_object *obj)
 	struct vmw_ttm_tt *vmw_tt =
 		container_of(bo->ttm, struct vmw_ttm_tt, dma_ttm);
 
-	if (vmw_tt->vsgt.sgt)
-		return vmw_tt->vsgt.sgt;
-
-	return drm_prime_pages_to_sg(obj->dev, vmw_tt->dma_ttm.pages, vmw_tt->dma_ttm.num_pages);
+	/*
+	 * Do not return &vmw_tt->sgt: the core owns what this returns and
+	 * drm_gem_unmap_dma_buf() sg_free_table()s and kfree()s it, but that
+	 * sg_table is embedded in the vmw_ttm_tt allocation.
+	 */
+	return drm_prime_pages_to_sg(obj->dev, vmw_tt->dma_ttm.pages,
+				     vmw_tt->dma_ttm.num_pages);
 }
 
 static int vmw_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
-- 
2.43.0

Re: [PATCH] drm/vmwgfx: do not hand the embedded sg_table to the PRIME core

Posted by Zack Rusin 21 hours ago

On Wed, Sep 23, 2026 at 8:26 AM Aldo Ariel Panzardo <qwe.aldo@gmail.com> wrote:
>
> vmw_gem_object_get_sg_table() returns vmw_tt->vsgt.sgt when the buffer
> object has already been DMA mapped.  vmw_ttm_map_dma() sets that field
> to &vmw_tt->sgt, which is a member embedded inside the struct
> vmw_ttm_tt allocation and not a table of its own.
>
> The core owns whatever .get_sg_table returns.  drm_gem_map_dma_buf()
> takes the table and drm_gem_unmap_dma_buf() destroys it in full:
>
>         dma_unmap_sgtable(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC);
>         sg_free_table(sgt);
>         kfree(sgt);
>
> Both are ops of drm_gem_prime_dmabuf_ops, so the core ends up calling
> kfree() on &vmw_tt->sgt, which is not the start of an allocation.  The
> preceding sg_free_table() also destroys a scatterlist that vmwgfx still
> considers its own and frees again from vmw_ttm_unmap_dma().
>
> drm_gem_unmap_dma_buf() runs from dma_buf_detach(), including the
> importer's error path, so a failed import is enough to reach it:
> exporting a bound buffer with DRM_IOCTL_PRIME_HANDLE_TO_FD and
> importing it on a second DRM device with DRM_IOCTL_PRIME_FD_TO_HANDLE
> is sufficient.  Both ioctls are DRM_RENDER_ALLOW.
>
> Observed on 6.12.101 with KASAN, as uid 65534:
>
>   BUG: KASAN: invalid-free in drm_gem_unmap_dma_buf+0xb3/0xf0
>   Free of addr ffff888008290450 by task poc_mm04_sgtabl/321
>   CPU: 1 UID: 65534 PID: 321 Comm: poc_mm04_sgtabl Not tainted 6.12.101 #4
>    kasan_report_invalid_free+0x94/0xc0
>    check_slab_allocation+0x116/0x120
>    kfree+0x103/0x360
>    drm_gem_unmap_dma_buf+0xb3/0xf0
>    dma_buf_detach+0x165/0x510
>    drm_gem_prime_import_dev+0x33e/0x430
>   which belongs to the cache kmalloc-192 of size 192
>   The buggy address is located 80 bytes inside of
>    144-byte region [ffff888008290400, ffff888008290490)
>
> 80 is offsetof(struct vmw_ttm_tt, sgt) and 144 is sizeof(struct
> vmw_ttm_tt), which identifies the freed pointer as the embedded member.
>
> Always return a table the core can own, as the other drivers do.
> Reusing the cached representation would require copying it into a
> freshly allocated sg_table, never returning the alias.
>
> Fixes: 8afa13a0583f ("drm/vmwgfx: Implement DRIVER_GEM")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> index 39f8c4655..a0233729b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> @@ -73,10 +73,13 @@ static struct sg_table *vmw_gem_object_get_sg_table(struct drm_gem_object *obj)
>         struct vmw_ttm_tt *vmw_tt =
>                 container_of(bo->ttm, struct vmw_ttm_tt, dma_ttm);
>
> -       if (vmw_tt->vsgt.sgt)
> -               return vmw_tt->vsgt.sgt;
> -
> -       return drm_prime_pages_to_sg(obj->dev, vmw_tt->dma_ttm.pages, vmw_tt->dma_ttm.num_pages);
> +       /*
> +        * Do not return &vmw_tt->sgt: the core owns what this returns and
> +        * drm_gem_unmap_dma_buf() sg_free_table()s and kfree()s it, but that
> +        * sg_table is embedded in the vmw_ttm_tt allocation.
> +        */
> +       return drm_prime_pages_to_sg(obj->dev, vmw_tt->dma_ttm.pages,
> +                                    vmw_tt->dma_ttm.num_pages);
>  }
>
>  static int vmw_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
> --
> 2.43.0
>

Thank you. Because of the influx of llm patches it's very hard to
reason about what's valid and what's not if it comes without a
reproducible testcase. Could you please add an igt testcase for this
and then include the full igt results for vmwgfx before and after this
change? Same for your other change.

z