Re: [PATCH v2] KVM: vVMX: signal failure for nested VMEntry if emulation_required

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2018-03-12 10:56-0700, Sean Christopherson:
> Fail a nested VMEntry with EXIT_REASON_INVALID_STATE if L2 guest state
> is invalid, i.e. vmcs12 contained invalid guest state, and unrestricted
> guest is disabled in L0 (and by extension disabled in L1).
> 
> WARN_ON_ONCE in handle_invalid_guest_state() if we're attempting to
> emulate L2, i.e. nested_run_pending is true, to aid debug in the
> (hopefully unlikely) scenario that we somehow skip the nested VMEntry
> consistency check, e.g. due to a L0 bug.
> 
> Note: KVM relies on hardware to detect the scenario where unrestricted
> guest is enabled in L0 but disabled in L1 and vmcs12 contains invalid
> guest state, i.e. checking emulation_required in prepare_vmcs02 is
> required only to handle the case were unrestricted guest is disabled
> in L0 since L0 never actually attempts VMLAUNCH/VMRESUME with vmcs02.

Oh, right, it is an L0 bug if we ever get to emulate the state.

> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> v2: Rework the patch to fix the underlying bug of not catching nested
>     VMEntry with invalid guest state and unrestricted guest disabled.
> 
> v1: https://patchwork.kernel.org/patch/10259397
> 
>  arch/x86/kvm/vmx.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 5912148..e4858f6 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6823,6 +6823,13 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
>  	bool intr_window_requested;
>  	unsigned count = 130;
>  
> +	/*
> +	 * We should never reach the point where we are emulating L2
> +	 * due to invalid guest state as that means we incorrectly
> +	 * allowed a nested VMEntry with an invalid vmcs12.
> +	 */
> +	WARN_ON_ONCE(vmx->emulation_required && vmx->nested.nested_run_pending);
> +
>  	cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
>  	intr_window_requested = cpu_exec_ctrl & CPU_BASED_VIRTUAL_INTR_PENDING;
>  
> @@ -10945,6 +10952,14 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>  		vmx->nested.dirty_vmcs12 = false;
>  	}
>  
> +	/*
> +	 * Guest state is invalid and unrestricted guest is disabled,
> +	 * which means L1 attempted VMEntry to L2 with invalid state.
> +	 * Fail the VMEntry.
> +	 */
> +	if (vmx->emulation_required)

We should do "*entry_failure_code = ENTRY_FAIL_DEFAULT" to avoid leaking
the L0 stack to L1.  With that,

Reviewed-by: Radim Krčmář <rkrcmar@xxxxxxxxxx>

thanks.

> +		return 1;
> +
>  	/* Shadow page tables on either EPT or shadow page tables. */
>  	if (nested_vmx_load_cr3(vcpu, vmcs12->guest_cr3, nested_cpu_has_ept(vmcs12),
>  				entry_failure_code))
> -- 
> 2.7.4
> 



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux