KVM currenty fails a nested VMRUN and injects VMEXIT_INVALID (aka SVM_EXIT_ERR) if L1 sets NP_ENABLE and the host does not support NPTs. On first glance, it seems like the check should actually be for guest_cpu_cap_has(X86_FEATURE_NPT) instead, as it is possible for the host to support NPTs but the guest CPUID to not advertise it.
However, the consistency check is not architectural to begin with. The APM does not mention VMEXIT_INVALID if NP_ENABLE is set on a processor that does not have X86_FEATURE_NPT. Hence, NP_ENABLE should be ignored if X86_FEATURE_NPT is not available for L1. Apart from the consistency check, this is currently the case because NP_ENABLE is actually copied from VMCB01 to VMCB02, not from VMCB12.
On the other hand, the APM does mention two other consistency checks for NP_ENABLE, both of which are missing (paraphrased):
In Volume #2, 15.25.3 (24593—Rev. 3.42—March 2024):
If VMRUN is executed with hCR0.PG cleared to zero and NP_ENABLE set to 1, VMRUN terminates with #VMEXIT(VMEXIT_INVALID)
In Volume #2, 15.25.4 (24593—Rev. 3.42—March 2024):
When VMRUN is executed with nested paging enabled (NP_ENABLE = 1), the following conditions are considered illegal state combinations, in addition to those mentioned in “Canonicalization and Consistency Checks”: • Any MBZ bit of nCR3 is set. • Any G_PAT.PA field has an unsupported type encoding or any reserved field in G_PAT has a nonzero value.
Replace the existing consistency check with consistency checks on hCR0.PG and nCR3. The G_PAT consistency check will be addressed separately.
Pass L1's CR0 to __nested_vmcb_check_controls(). In nested_vmcb_check_controls(), L1's CR0 is available through kvm_read_cr0(), as vcpu->arch.cr0 is not updated to L2's CR0 until later through nested_vmcb02_prepare_save() -> svm_set_cr0().
In svm_set_nested_state(), L1's CR0 is available in the captured save area, as svm_get_nested_state() captures L1's save area when running L2, and L1's CR0 is stashed in VMCB01 on nested VMRUN (in nested_svm_vmrun()).
Fixes: 4b16184c1cca ("KVM: SVM: Initialize Nested Nested MMU context on VMRUN") Cc: stable@vger.kernel.org
Signed-off-by: Yosry Ahmed yosry.ahmed@linux.dev --- arch/x86/kvm/svm/nested.c | 21 ++++++++++++++++----- arch/x86/kvm/svm/svm.h | 3 ++- 2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 83de3456df708..9a534f04bdc83 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -325,7 +325,8 @@ static bool nested_svm_check_bitmap_pa(struct kvm_vcpu *vcpu, u64 pa, u32 size) }
static bool __nested_vmcb_check_controls(struct kvm_vcpu *vcpu, - struct vmcb_ctrl_area_cached *control) + struct vmcb_ctrl_area_cached *control, + unsigned long l1_cr0) { if (CC(!vmcb12_is_intercept(control, INTERCEPT_VMRUN))) return false; @@ -333,8 +334,12 @@ static bool __nested_vmcb_check_controls(struct kvm_vcpu *vcpu, if (CC(control->asid == 0)) return false;
- if (CC((control->nested_ctl & SVM_NESTED_CTL_NP_ENABLE) && !npt_enabled)) - return false; + if (control->nested_ctl & SVM_NESTED_CTL_NP_ENABLE) { + if (CC(!kvm_vcpu_is_legal_gpa(vcpu, control->nested_cr3))) + return false; + if (CC(!(l1_cr0 & X86_CR0_PG))) + return false; + }
if (CC(!nested_svm_check_bitmap_pa(vcpu, control->msrpm_base_pa, MSRPM_SIZE))) @@ -400,7 +405,12 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu) struct vcpu_svm *svm = to_svm(vcpu); struct vmcb_ctrl_area_cached *ctl = &svm->nested.ctl;
- return __nested_vmcb_check_controls(vcpu, ctl); + /* + * Make sure we did not enter guest mode yet, in which case + * kvm_read_cr0() could return L2's CR0. + */ + WARN_ON_ONCE(is_guest_mode(vcpu)); + return __nested_vmcb_check_controls(vcpu, ctl, kvm_read_cr0(vcpu)); }
static @@ -1832,7 +1842,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
ret = -EINVAL; __nested_copy_vmcb_control_to_cache(vcpu, &ctl_cached, ctl); - if (!__nested_vmcb_check_controls(vcpu, &ctl_cached)) + /* 'save' contains L1 state saved from before VMRUN */ + if (!__nested_vmcb_check_controls(vcpu, &ctl_cached, save->cr0)) goto out_free;
/* diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 6765a5e433cea..0a2908e22d746 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -552,7 +552,8 @@ static inline bool gif_set(struct vcpu_svm *svm)
static inline bool nested_npt_enabled(struct vcpu_svm *svm) { - return svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE; + return guest_cpu_cap_has(&svm->vcpu, X86_FEATURE_NPT) && + svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE; }
static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)