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

[Zack Rusin]

On Sat, Aug 8, 2026 at 7:10 PM Aldo Ariel Panzardo <qwe.aldo@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@xxxxxxxxx>

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: smime.p7s
Description: S/MIME Cryptographic Signature