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.
> 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()").
> 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.)
Jan
