| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bghpqd23vy6pszckz4psxox3ww256uazmoa6zadagnn4zclja2@x5cb23tigyfw>
Date: Wed, 5 Nov 2025 18:52:58 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Jim Mattson <jmattson@...gle.com>,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 01/11] KVM: nSVM: Fix consistency checks for NP_ENABLE
On Tue, Nov 04, 2025 at 07:59:39PM +0000, Yosry Ahmed wrote:
> 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@...r.kernel.org
>
> Signed-off-by: Yosry Ahmed <yosry.ahmed@...ux.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) {
I think this should actually be nested_npt_enabled(), because we
shouldn't do these consistency checks if NPT is not supported on the
vCPU at all (which was kinda the point).
> + 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)
> --
> 2.51.2.1026.g39e6a42477-goog
>
Powered by blists - more mailing lists