Re: Nullptr dereference in nested VMX when shadow VMCS support is available

[Manuel Andreas <manuel.andreas@xxxxxx>]

On 6/2/25 4:12 PM, Jan Beulich wrote:

> On 02.06.2025 15:39, Manuel Andreas wrote:
> > I've discovered an issue in the nested VMX implementation, where an 
> > unprivileged domain is able to force Xen to dereference a NULL pointer, 
> > resulting in a panic.
> >       
> Sadly you provide no details on this NULL deref.
>     
    Here's the respective dump:
    
    ----[ Xen-4.20.0  x86_64  debug=y 
      Tainted:     H  ]----
      (XEN) CPU:    1
      (XEN) RIP:    e008:[<ffff82d0402ae2b8>]
      nvmx_handle_vmx_insn+0x7ab/0xccb
      (XEN) RFLAGS: 0000000000010202   CONTEXT: hypervisor (d1v0)
      (XEN) rax: 0000000000000000   rbx: 0000000000000000   rcx:
      8000000000000002
      (XEN) rdx: 0000000000000000   rsi: 01ffffffffffffff   rdi:
      ffff82e0020155e0
      (XEN) rbp: ffff830179407e68   rsp: ffff830179407e00   r8: 
      ffff82c00023b000
      (XEN) r9:  ffff830179413c40   r10: 0000000000000000   r11:
      0000000000000200
      (XEN) r12: ffff83010483d000  r13: ffff830179407ef8   r14:
      0000000000000000
      (XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4:
      00000000003526e0
      (XEN) cr3: 000000010498f000   cr2: 0000000000000000
      (XEN) fsb: 0000000000000000   gsb: 0000000000000000   gss:
      0000000000000000
      (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs:
      e008
      (XEN) Xen code around <ffff82d0402ae2b8>
      (nvmx_handle_vmx_insn+0x7ab/0xccb):
      (XEN)  75 b0 0f 86 12 05 00 00 <81> 08 00 00 00 80 41 8b 84
      24 f4 05 00 00 80 cc
      (XEN) Xen stack trace from rsp=ffff830179407e00:
      (XEN)    ffff83010483d000 000000000011e000 0000000000000000
      0000000000000000
      (XEN)    ffff82d0402bfc4a 0000000100000000 0000000000119fa8
      ffff82d000000008
      (XEN)    ffff830100000006 ffff830179407ef8 0000000000000015
      ffff83010483d000
      (XEN)    0000000000000000 ffff830179407ee8 ffff82d0402a9a19
      ffff82d04020361b
      (XEN)    0000000000000000 0000000000000000 0000000000000000
      ffff830100997000
      (XEN)    ffff82d040203615 ffff82d04020361b ffff82d040203615
      ffff82d04020361b
      (XEN)    ffff83010483d000 0000000000000000 0000000000000000
      0000000000000000
      (XEN)    0000000000000000 00007cfe86bf80e7 ffff82d040203673
      0000000000000000
      (XEN)    0000000000000000 0000000000000000 0000000000000000
      0000000000000001
      (XEN)    0000000000000000 0000000000000000 0000000000000043
      000000000000007b
      (XEN)    0000000000000043 0000000000000000 0000000000000000
      0000000011e57e00
      (XEN)    0000000000000000 0000000000000000 0000beef0000beef
      0000000000103fa2
      (XEN)    000000bf0000beef 0000000000000046 0000000000119fa0
      000000000000beef
      (XEN)    000000000000beef 000000000000beef 000000000000beef
      000000000000beef
      (XEN)    0000e01000000001 ffff83010483d000 0000003136627000
      00000000003526e0
      (XEN)    0000000000000000 0000000000000000 0000000300000001
      0000004e00000003
      (XEN) Xen call trace:
      (XEN)    [<ffff82d0402ae2b8>] R
      nvmx_handle_vmx_insn+0x7ab/0xccb
      (XEN)    [<ffff82d0402a9a19>] F
      vmx_vmexit_handler+0xd97/0x1e14
      (XEN)    [<ffff82d040203673>] F
      vmx_asm_vmexit_handler+0x103/0x220
      (XEN)
      (XEN) Pagetable walk from 0000000000000000:

(XEN)  L4[0x000] = 0000000000000000 ffffffffffffffff

Where nvmx_handle_vmx_insn+0x7ab/0xccb resolves to xen/arch/x86/hvm/vmx/vvmx.c:1169
Specifically, in nvmx_handle_vmptrld we have:

1830    if ( cpu_has_vmx_vmcs_shadowing )
1831        nvmx_set_vmcs_pointer(v, nvcpu->nv_vvmcx);

Which is reachable with nvcpu->nv_vvmcx untouched in the case of the p2m mapping for gpa not being writable (see lines 1793 - 1827) .
Therefore, nvcpu->nv_vvmcx will remain NULL (as initialized in nvmx_vcpu_initialize).
Further, inside nvmx_set_vmcs_pointer:

1164    static void nvmx_set_vmcs_pointer(struct vcpu *v, struct vmcs_struct *vvmcs)
1165    {
1166        paddr_t vvmcs_maddr = v->arch.hvm.vmx.vmcs_shadow_maddr;
1167
1168        __vmpclear(vvmcs_maddr);
1169        vvmcs->vmcs_revision_id |= VMCS_RID_TYPE_MASK;
1170        /* ... */

will then dereference vvmcs, which is NULL.

> > This is possible when:
> >
> >  1. The malicious domain has nested HVM capabilities.
> >  2. The CPU is running on top of VMX and supports shadow VMCS.
> >
> > To trigger the bug, the domain must first enable VMX operation for 
> > itself, execute VMXON and then finally execute VMPTRLD on a guest 
> > physical address that is backed by a non-writable p2m mapping.
> > In `nvmx_handle_vmptrld`, after attempting to map the nested VMCS, Xen 
> > will check whether or not this mapping is suitable for writing and if 
> > not immediately unmap the nested VMCS again and abort the setup of 
> > `nvcpu->nv_vvmcx`. However, Xen at this point erroneously continues 
> > emulation of the VMPTRLD. In particular, if VMCS shadowing is available, 
> > Xen will nonetheless attempt to link up the nested VMCS to its own VMCS 
> > in `nvmx_set_vmcs_pointer`. Importantly, Xen here attempts to 
> > dereference the presumably mapped nested VMCS (which now is merely a 
> > NULL pointer) in order to mark it as a shadow VMCS by applying the 
> > `VMCS_RID_TYPE_MASK` to its revision identifier. Following, the page 
> > fault handler will panic Xen.
> >
> > I've attached an XTF reproducer that triggers the bug. To setup such a 
> > non-writable p2m mapping for the malicious VMCS, I first setup an 
> > appropriate grant table entry. I've tested it on Xen version 4.20.0.
> >       
> I expect this to not work anymore on current staging or 4.20.1-pre.
> See a8325f981ce4 ("x86/P2M: synchronize fast and slow paths of
> p2m_get_page_from_gfn()").
>     
    On first glance I don't see how that would impact the type of the
    established p2m mapping.
    If you want I can compile staging and test if the PoC does not work
    there anymore.
> > To fix the issue I believe the following patch should be suitable:
> >
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -1817,7 +1817,9 @@ static int nvmx_handle_vmptrld(struct 
> > cpu_user_regs *regs)
> >               else
> >               {
> >                   hvm_unmap_guest_frame(vvmcx, 1);
> > -                vvmcx = NULL;
> > +                vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
> > +
> > +                return X86EMUL_OKAY;
> >               }
> >           }
> >           else
> >
> > The VMX error AFAICT does not strictly adhere to the Intel SDM, but 
> > providing the guest some indication on what went wrong is likely more 
> > sensible than silently failing.
> >       
> Giving the guest some indication is certainly right. If we want to follow
> the above route, I think the change would want doing a little differently,
> to take the path that presently is the "else" at the bottom of the hunk
> above. However, I can't presently see how invoking vmfail() would make a
> difference as to the subsequent NULL deref: The guest could continue the
> same irrespective of the failure. Hence why I'd like to understand what
> NULL deref you did observe. (We may hence need two patches - one along
> the above lines, and another one dealing with the NULL issue.)
>

The issue is really just in the latter part of nvmx_handle_vmptrld, which attempts to initialize its shadow VMCS even though establishing a mapping for the nested VMCS failed.
An early exit from that function (as my patch suggests) should be sufficient for that case.
Other parts of the nested VMX code already check for a valid nested VMCS via vvmcx_valid. There simply isn't such a check here presumably because the assumption was that any previous errors in mapping the nested VMCS were already handled by returning from the function or jumping to the out label.

Best,
Manuel