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 <[email protected]> escribió: > On Sat, Aug 8, 2026 at 7:10 PM Aldo Ariel Panzardo <[email protected]> > 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: [email protected] > > Signed-off-by: Aldo Ariel Panzardo <[email protected]> > > 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 >
