[v7] Nested SVM fixes, cleanups, and hardening

[PATCH v7 15/26] KVM: nSVM: Add missing consistency check for nCR3 validity

Posted by Yosry Ahmed 1 month ago

From the APM 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.

Add the consistency check for nCR3 being a legal GPA with no MBZ bits
set. The G_PAT.PA check was proposed separately [*].

[*]https://lore.kernel.org/kvm/20260205214326.1029278-3-jmattson@google.com/

Fixes: 4b16184c1cca ("KVM: SVM: Initialize Nested Nested MMU context on VMRUN")
Cc: stable@vger.kernel.org
Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/kvm/svm/nested.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 613d5e2e7c3d1..3aaa4f0bb31ab 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -348,6 +348,11 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
 	if (CC(control->asid == 0))
 		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(!nested_svm_check_bitmap_pa(vcpu, control->msrpm_base_pa,
 					   MSRPM_SIZE)))
 		return false;
-- 
2.53.0.473.g4a7958ca14-goog

Re: [PATCH v7 15/26] KVM: nSVM: Add missing consistency check for nCR3 validity

Posted by Sean Christopherson 4 weeks, 1 day ago

On Tue, Mar 03, 2026, Yosry Ahmed wrote:
> >From the APM 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.
> 
> Add the consistency check for nCR3 being a legal GPA with no MBZ bits
> set. The G_PAT.PA check was proposed separately [*].
> 
> [*]https://lore.kernel.org/kvm/20260205214326.1029278-3-jmattson@google.com/
> 
> Fixes: 4b16184c1cca ("KVM: SVM: Initialize Nested Nested MMU context on VMRUN")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yosry Ahmed <yosry@kernel.org>
> ---
>  arch/x86/kvm/svm/nested.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 613d5e2e7c3d1..3aaa4f0bb31ab 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -348,6 +348,11 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
>  	if (CC(control->asid == 0))
>  		return false;
>  
> +	if (control->nested_ctl & SVM_NESTED_CTL_NP_ENABLE) {
> +		if (CC(!kvm_vcpu_is_legal_gpa(vcpu, control->nested_cr3)))
> +			return false;

Put the full if-statement in CC(), that way the tracepoint will capture the entire
clause, i.e. will help the reader understand than nested_cr3 was checked
specifically because NPT was enabled.

	if (CC((control->nested_ctl & SVM_NESTED_CTL_NP_ENABLE) &&
	       !kvm_vcpu_is_legal_gpa(vcpu, control->nested_cr3)))
		return false;

Re: [PATCH v7 15/26] KVM: nSVM: Add missing consistency check for nCR3 validity

Posted by Yosry Ahmed 4 weeks, 1 day ago

On Tue, Mar 3, 2026 at 8:56 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Mar 03, 2026, Yosry Ahmed wrote:
> > >From the APM 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.
> >
> > Add the consistency check for nCR3 being a legal GPA with no MBZ bits
> > set. The G_PAT.PA check was proposed separately [*].
> >
> > [*]https://lore.kernel.org/kvm/20260205214326.1029278-3-jmattson@google.com/
> >
> > Fixes: 4b16184c1cca ("KVM: SVM: Initialize Nested Nested MMU context on VMRUN")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yosry Ahmed <yosry@kernel.org>
> > ---
> >  arch/x86/kvm/svm/nested.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 613d5e2e7c3d1..3aaa4f0bb31ab 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -348,6 +348,11 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
> >       if (CC(control->asid == 0))
> >               return false;
> >
> > +     if (control->nested_ctl & SVM_NESTED_CTL_NP_ENABLE) {
> > +             if (CC(!kvm_vcpu_is_legal_gpa(vcpu, control->nested_cr3)))
> > +                     return false;
>
> Put the full if-statement in CC(), that way the tracepoint will capture the entire
> clause, i.e. will help the reader understand than nested_cr3 was checked
> specifically because NPT was enabled.

I had it this way in v6 because there was another consistency check
dependent on NPT being enabled:
https://lore.kernel.org/kvm/20260224223405.3270433-21-yosry@kernel.org/.

I dropped the patch in v7 as I realized L1's CR0.PG was already being
checked, but it didn't occur to me to go back and update this. Good
catch.

>
>         if (CC((control->nested_ctl & SVM_NESTED_CTL_NP_ENABLE) &&
>                !kvm_vcpu_is_legal_gpa(vcpu, control->nested_cr3)))
>                 return false;