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]>
---
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;
