[PATCH] drm/vmwgfx: return an error when a shared GB surface has no backup buffer

[PATCH] drm/vmwgfx: return an error when a shared GB surface has no backup buffer

[Aldo Ariel Panzardo]

vmw_gb_surface_reference_internal() rejects a surface without a backup
buffer by logging an error and jumping to the exit label, but it never
sets ret on that path:

	ret = vmw_surface_handle_reference(dev_priv, file_priv, req->sid,
					   req->handle_type, &base);
	if (unlikely(ret != 0))
		return ret;
	...
	if (!srf->res.guest_memory_bo) {
		DRM_ERROR("Shared GB surface is missing a backup buffer.\n");
		goto out_bad_resource;
	}
	...
out_bad_resource:
	ttm_base_object_unref(&base);

	return ret;

ret is still 0 from the successful vmw_surface_handle_reference() above,
so the function returns success while leaving *rep completely unwritten.
The other goto to the same label is inside an if (ret != 0) block and so
carries a real error; this one is the only path that reaches the label
with ret == 0.

The caller then copies that untouched output structure to user space.
For DRM_VMW_GB_SURFACE_REF, vmw_gb_surface_reference_ioctl() passes a
stack local:

	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;

so 48 + 24 = 72 bytes of an uninitialised stack variable are copied into
the ioctl buffer, and drm_ioctl() copies that buffer back out.  The ioctl
is a DRM_IOWR of exactly 72 bytes, so in_size == out_size and the core
does not zero any tail.  DRM_VMW_GB_SURFACE_REF is DRM_RENDER_ALLOW, so
this is reachable by an unprivileged local user through a render node.

What actually leaks depends on how the kernel was built.  With
CONFIG_INIT_STACK_ALL_ZERO, which is the Kconfig default whenever the
compiler supports it and therefore what the major distributions ship,
rep_ext is zeroed on function entry and user space receives 72 zero
bytes.  With CONFIG_INIT_STACK_NONE the contents are whatever the
previous call at that stack depth left behind; on a test kernel built
that way, 53 of the 72 bytes came back non-zero and several of them were
kernel pointers.  So the information disclosure is configuration
dependent, but the control flow defect is not: on every configuration
the ioctl reports success and hands back a reply that was never
produced, which user space cannot distinguish from a real one.

The path was correct when the ioctl was introduced in a97e21923b42
("drm/vmwgfx: Hook up guest-backed surfaces"): ret was initialised to
-EINVAL and the first assignment to it came after this goto, so the
label really did return -EINVAL.  14b1c33e8429 split the handler into
vmw_gb_surface_reference_internal() and moved the
vmw_surface_handle_reference() call - and with it the first assignment
to ret - above the check, which left the initialiser dead and this path
returning 0.  The dead initialiser was removed later, as a Coverity
"unused value", by c594285f30fa ("drm/vmwgfx: remove redundant
assignment to variable ret"); that removal was correct in itself.

The driver already knows the request failed - it logs an error - so
report that to the caller.

Fixes: 14b1c33e8429 ("drm/vmwgfx: Add new ioctl for GB surface create and reference")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -1720,6 +1720,7 @@ vmw_gb_surface_reference_internal(struct drm_device *dev,
 	srf = &user_srf->srf;
 	if (!srf->res.guest_memory_bo) {
 		DRM_ERROR("Shared GB surface is missing a backup buffer.\n");
+		ret = -EINVAL;
 		goto out_bad_resource;
 	}
 	metadata = &srf->metadata;

Re: [PATCH] drm/vmwgfx: return an error when a shared GB surface has no backup buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] TOCTOU race on `srf->res.guest_memory_bo` leading to a NULL pointer dereference in `drm_gem_handle_create`.
- [Critical] Lockless accesses to `guest_memory_bo` after releasing `cmdbuf_mutex` can lead to a Use-After-Free or NULL pointer dereference.
- [High] Leaked DRM handle on error paths in `vmw_gb_surface_reference_internal` when processing a PRIME file descriptor.
- [High] GEM handle leak in `vmw_gb_surface_define_internal` when initialization fails after handle creation.
- [Medium] The function `vmw_gb_surface_reference_internal` fails to populate the `buffer_byte_stride` field in the extended reply structure.
--

--- Patch [1]: [PATCH] drm/vmwgfx: return an error when a shared GB surface has no backup buffer ---
commit 6bbaf49225562d900a468ed3b35d8995cea03c99
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

drm/vmwgfx: return an error when a shared GB surface has no backup buffer

This commit fixes a missing error assignment in
vmw_gb_surface_reference_internal() when a shared GB surface lacks a backup
buffer. By returning -EINVAL instead of 0, it prevents the ioctl from
returning uninitialized stack memory to userspace.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index bd0563741e89d..014f674744ac3 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -1720,6 +1720,7 @@ vmw_gb_surface_reference_internal(struct drm_device *dev,
>  	srf = &user_srf->srf;
>  	if (!srf->res.guest_memory_bo) {
>  		DRM_ERROR("Shared GB surface is missing a backup buffer.\n");
> +		ret = -EINVAL;
>  		goto out_bad_resource;
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this lockless check create a TOCTOU
race?

If a concurrent thread detaches the backup buffer and sets guest_memory_bo to
NULL under cmdbuf_mutex, the lockless check here might pass, but a NULL pointer
would then be dereferenced in drm_gem_handle_create():

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_internal() {
...
	mutex_lock(&dev_priv->cmdbuf_mutex);
	ret = drm_gem_handle_create(file_priv,
				    &srf->res.guest_memory_bo->tbo.base,
				    &backup_handle);
	mutex_unlock(&dev_priv->cmdbuf_mutex);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but does taking this error path leak
a DRM handle?

When handle_type == DRM_VMW_HANDLE_PRIME, vmw_surface_handle_reference() adds a
reference to the file's handle table. The cleanup path at out_bad_resource
drops the object reference but doesn't remove the newly created handle from the
file:

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_internal() {
...
out_bad_resource:
	ttm_base_object_unref(&base);

	return ret;
}

>  	metadata = &srf->metadata;

[Severity: Critical]
This is a pre-existing issue, but are there unsafe lockless accesses to
guest_memory_bo later in this function?

After releasing cmdbuf_mutex, the function populates mapping handles and sizes
by locklessly dereferencing srf->res.guest_memory_bo:

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_internal() {
...
	rep->crep.buffer_map_handle =
		drm_vma_node_offset_addr(
			&srf->res.guest_memory_bo->tbo.base.vma_node);
	rep->crep.buffer_size = srf->res.guest_memory_bo->tbo.base.size;
...
}

Could this lead to a NULL pointer dereference or Use-After-Free if another
thread detaches or frees the backup buffer concurrently?

[Severity: Medium]
This isn't a bug introduced by this patch, but does this function fail to
populate the buffer_byte_stride field in the extended reply structure?

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_internal() {
...
	rep->creq.version = drm_vmw_gb_surface_v1;
	rep->creq.svga3d_flags_upper_32_bits =
		SVGA3D_FLAGS_UPPER_32(metadata->flags);
	rep->creq.multisample_pattern = metadata->multisample_pattern;
	rep->creq.quality_level = metadata->quality_level;
	rep->creq.must_be_zero = 0;
...
}

[Severity: High]
This is a pre-existing issue, but does vmw_gb_surface_define_internal() leak a
GEM handle when initialization fails?

If any initialization fails after vmw_gem_object_create_with_handle()
successfully allocates a GEM buffer object and registers backup_handle, the
cleanup path fails to call drm_gem_handle_delete():

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_define_internal() {
...
		ret = vmw_gem_object_create_with_handle(dev_priv, file_priv,
							res->guest_memory_size,
							&backup_handle,
							&res->guest_memory_bo);
	}

	if (unlikely(ret != 0)) {
		vmw_resource_unreference(&res);
		goto out_unlock;
	}
...
out_unlock:
	vmw_resource_unreference(&tmp);
	vmw_resource_unreference(&res);
	return ret;
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808231002.67559-1-qwe.aldo@gmail.com?part=1

Re: [PATCH] drm/vmwgfx: return an error when a shared GB surface has no backup buffer

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 4644 bytes --]

On Sat, Aug 8, 2026 at 7:10 PM Aldo Ariel Panzardo <qwe.aldo@gmail.com> wrote:
>
> vmw_gb_surface_reference_internal() rejects a surface without a backup
> buffer by logging an error and jumping to the exit label, but it never
> sets ret on that path:
>
>         ret = vmw_surface_handle_reference(dev_priv, file_priv, req->sid,
>                                            req->handle_type, &base);
>         if (unlikely(ret != 0))
>                 return ret;
>         ...
>         if (!srf->res.guest_memory_bo) {
>                 DRM_ERROR("Shared GB surface is missing a backup buffer.\n");
>                 goto out_bad_resource;
>         }
>         ...
> out_bad_resource:
>         ttm_base_object_unref(&base);
>
>         return ret;
>
> ret is still 0 from the successful vmw_surface_handle_reference() above,
> so the function returns success while leaving *rep completely unwritten.
> The other goto to the same label is inside an if (ret != 0) block and so
> carries a real error; this one is the only path that reaches the label
> with ret == 0.
>
> The caller then copies that untouched output structure to user space.
> For DRM_VMW_GB_SURFACE_REF, vmw_gb_surface_reference_ioctl() passes a
> stack local:
>
>         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;
>
> so 48 + 24 = 72 bytes of an uninitialised stack variable are copied into
> the ioctl buffer, and drm_ioctl() copies that buffer back out.  The ioctl
> is a DRM_IOWR of exactly 72 bytes, so in_size == out_size and the core
> does not zero any tail.  DRM_VMW_GB_SURFACE_REF is DRM_RENDER_ALLOW, so
> this is reachable by an unprivileged local user through a render node.
>
> What actually leaks depends on how the kernel was built.  With
> CONFIG_INIT_STACK_ALL_ZERO, which is the Kconfig default whenever the
> compiler supports it and therefore what the major distributions ship,
> rep_ext is zeroed on function entry and user space receives 72 zero
> bytes.  With CONFIG_INIT_STACK_NONE the contents are whatever the
> previous call at that stack depth left behind; on a test kernel built
> that way, 53 of the 72 bytes came back non-zero and several of them were
> kernel pointers.  So the information disclosure is configuration
> dependent, but the control flow defect is not: on every configuration
> the ioctl reports success and hands back a reply that was never
> produced, which user space cannot distinguish from a real one.
>
> The path was correct when the ioctl was introduced in a97e21923b42
> ("drm/vmwgfx: Hook up guest-backed surfaces"): ret was initialised to
> -EINVAL and the first assignment to it came after this goto, so the
> label really did return -EINVAL.  14b1c33e8429 split the handler into
> vmw_gb_surface_reference_internal() and moved the
> vmw_surface_handle_reference() call - and with it the first assignment
> to ret - above the check, which left the initialiser dead and this path
> returning 0.  The dead initialiser was removed later, as a Coverity
> "unused value", by c594285f30fa ("drm/vmwgfx: remove redundant
> assignment to variable ret"); that removal was correct in itself.
>
> The driver already knows the request failed - it logs an error - so
> report that to the caller.
>
> Fixes: 14b1c33e8429 ("drm/vmwgfx: Add new ioctl for GB surface create and reference")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

Hi, thanks for the patch! Did you forget to disclose the llm used to
find and fix it? I'm asking because that commit description is very
hard to read for what is essentially "ret hasn't been correctly
assigned, returning stale success status and leading to possible
invalid reads in userspace". Plus, I'd like to know how to handle the
commits referenced in the description. In general, I'd trust them if
you have looked them up yourself by hand but I'll need to validate a
lot more carefully if they're llm generated.

We probably also want to add a ttm_ref_object_base_unref(tfile,
base->handle); to the out_bad_resource section because before with
this function returning success it, accidently, made userspace still
call DRM_VMW_UNREF_SURFACE balencing out the ttm file reference count.
Now we're just going to be leaking the reference added by
ttm_ref_object_add in vmw_surface_handle_reference.

z

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5414 bytes --]

Re: [PATCH] drm/vmwgfx: return an error when a shared GB surface has no backup buffer

[Aldo Ariel]

[-- Attachment #1: Type: text/plain, Size: 6049 bytes --]

Hi Zack, thanks for the careful review.

I used an LLM only to help with the analysis. The work behind it is
manual, though — I audit the vmwgfx code by hand and run instrumented
builds of the driver (KASAN/KMSAN under syzkaller).

This particular bug came from reading vmw_gb_surface_reference_internal()
rather than from a fuzzer. I confirmed the disclosure by building with
CONFIG_INIT_STACK_NONE and watching the reply come back with 53 of the 72
bytes non-zero, several of them kernel pointers — so the impact is
measured, not assumed. (KASAN wouldn't flag this one on its own, since
it's an uninitialised-stack read.)

The three commits I referenced (a97e21923b42, 14b1c33e8429, c594285f30fa) I
traced by hand through the git history.

Agreed the description is far too long for what it is; I'll cut it down.

And thanks for the ttm_ref_object_base_unref() catch — you're right that
-EINVAL turns the previously-balanced reference into a leak (user space
no longer calls DRM_VMW_UNREF_SURFACE). I'll drop it on the error path in
v2.

v2 coming with both changes.

Thanks,
Aldo

El dom, 9 de ago de 2026, 14:35, Zack Rusin <zack.rusin@broadcom.com>
escribió:

> On Sat, Aug 8, 2026 at 7:10 PM Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> wrote:
> >
> > vmw_gb_surface_reference_internal() rejects a surface without a backup
> > buffer by logging an error and jumping to the exit label, but it never
> > sets ret on that path:
> >
> >         ret = vmw_surface_handle_reference(dev_priv, file_priv, req->sid,
> >                                            req->handle_type, &base);
> >         if (unlikely(ret != 0))
> >                 return ret;
> >         ...
> >         if (!srf->res.guest_memory_bo) {
> >                 DRM_ERROR("Shared GB surface is missing a backup
> buffer.\n");
> >                 goto out_bad_resource;
> >         }
> >         ...
> > out_bad_resource:
> >         ttm_base_object_unref(&base);
> >
> >         return ret;
> >
> > ret is still 0 from the successful vmw_surface_handle_reference() above,
> > so the function returns success while leaving *rep completely unwritten.
> > The other goto to the same label is inside an if (ret != 0) block and so
> > carries a real error; this one is the only path that reaches the label
> > with ret == 0.
> >
> > The caller then copies that untouched output structure to user space.
> > For DRM_VMW_GB_SURFACE_REF, vmw_gb_surface_reference_ioctl() passes a
> > stack local:
> >
> >         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;
> >
> > so 48 + 24 = 72 bytes of an uninitialised stack variable are copied into
> > the ioctl buffer, and drm_ioctl() copies that buffer back out.  The ioctl
> > is a DRM_IOWR of exactly 72 bytes, so in_size == out_size and the core
> > does not zero any tail.  DRM_VMW_GB_SURFACE_REF is DRM_RENDER_ALLOW, so
> > this is reachable by an unprivileged local user through a render node.
> >
> > What actually leaks depends on how the kernel was built.  With
> > CONFIG_INIT_STACK_ALL_ZERO, which is the Kconfig default whenever the
> > compiler supports it and therefore what the major distributions ship,
> > rep_ext is zeroed on function entry and user space receives 72 zero
> > bytes.  With CONFIG_INIT_STACK_NONE the contents are whatever the
> > previous call at that stack depth left behind; on a test kernel built
> > that way, 53 of the 72 bytes came back non-zero and several of them were
> > kernel pointers.  So the information disclosure is configuration
> > dependent, but the control flow defect is not: on every configuration
> > the ioctl reports success and hands back a reply that was never
> > produced, which user space cannot distinguish from a real one.
> >
> > The path was correct when the ioctl was introduced in a97e21923b42
> > ("drm/vmwgfx: Hook up guest-backed surfaces"): ret was initialised to
> > -EINVAL and the first assignment to it came after this goto, so the
> > label really did return -EINVAL.  14b1c33e8429 split the handler into
> > vmw_gb_surface_reference_internal() and moved the
> > vmw_surface_handle_reference() call - and with it the first assignment
> > to ret - above the check, which left the initialiser dead and this path
> > returning 0.  The dead initialiser was removed later, as a Coverity
> > "unused value", by c594285f30fa ("drm/vmwgfx: remove redundant
> > assignment to variable ret"); that removal was correct in itself.
> >
> > The driver already knows the request failed - it logs an error - so
> > report that to the caller.
> >
> > Fixes: 14b1c33e8429 ("drm/vmwgfx: Add new ioctl for GB surface create
> and reference")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
>
> Hi, thanks for the patch! Did you forget to disclose the llm used to
> find and fix it? I'm asking because that commit description is very
> hard to read for what is essentially "ret hasn't been correctly
> assigned, returning stale success status and leading to possible
> invalid reads in userspace". Plus, I'd like to know how to handle the
> commits referenced in the description. In general, I'd trust them if
> you have looked them up yourself by hand but I'll need to validate a
> lot more carefully if they're llm generated.
>
> We probably also want to add a ttm_ref_object_base_unref(tfile,
> base->handle); to the out_bad_resource section because before with
> this function returning success it, accidently, made userspace still
> call DRM_VMW_UNREF_SURFACE balencing out the ttm file reference count.
> Now we're just going to be leaking the reference added by
> ttm_ref_object_add in vmw_surface_handle_reference.
>
> z
>

[-- Attachment #2: Type: text/html, Size: 7772 bytes --]