linux-kernel - Re: [PATCH v2 04/13] KVM: nSVM: Fix consistency checks for NP

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aThp19OAXDoZlk3k@google.com>
Date: Tue, 9 Dec 2025 10:26:31 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
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 v2 04/13] KVM: nSVM: Fix consistency checks for NP_ENABLE

On Tue, Dec 09, 2025, Yosry Ahmed wrote:
> On Tue, Dec 09, 2025 at 08:27:39AM -0800, Sean Christopherson wrote:
> > On Mon, Nov 10, 2025, Yosry Ahmed wrote:
> > > @@ -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
> > 
> > No pronouns.
> 
> I thought that rule was for commit logs. 

In KVM x86, it's a rule everywhere.  Pronouns often add ambiguity, and it's much
easier to have a hard "no pronouns" rule than to try and enforce an inherently
subjective "is this ambiguous or not" rule.

> There are plenty of 'we's in the KVM x86 code (and all x86 code for that
> matter) :P

Ya, KVM is an 18+ year old code base.  There's also a ton of bare "unsigned" usage,
and other things that are frowned upon and/or flagged by checkpatch.  I'm all for
cleaning things up when touching the code, but I'm staunchly against "tree"-wide
cleanups just to make checkpatch happy, and so there's quite a few historical
violations of the current "rules".

> > > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> > > index f6fb70ddf7272..3e805a43ffcdb 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;
> > 
> > I would rather rely on Kevin's patch to clear unsupported features.
> 
> Not sure how Kevin's patch is relevant here, could you please clarify?

Doh, Kevin's patch only touches intercepts.  What I was trying to say is that I
would rather sanitize the snapshot (the approach Kevin's patch takes with the
intercepts), as opposed to guarding the accessor.  That way we can't have bugs
where KVM checks svm->nested.ctl.nested_ctl directly and bypasses the caps check.