[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@xxxxxxxxxxxxxxx
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@xxxxxxxxx>
---
 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;