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

[Jan Beulich <jbeulich@xxxxxxxx>]

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