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: <aThtjYG3OZTtdwUA@google.com>
Date: Tue, 9 Dec 2025 10:42:21 -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 10:26:31AM -0800, Sean Christopherson wrote:
> > On Tue, Dec 09, 2025, Yosry Ahmed wrote:
> > > > > 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.
> 
> I see, so clear SVM_NESTED_CTL_NP_ENABLE in
> __nested_copy_vmcb_control_to_cache() instead.
> 
> If I drop the guest_cpu_cap_has() check here I will want to leave a
> comment so that it's obvious to readers that SVM_NESTED_CTL_NP_ENABLE is
> sanitized elsewhere if the guest cannot use NPTs. Alternatively, I can
> just keep the guest_cpu_cap_has() check as documentation and a second
> line of defense.
> 
> Any preferences?

Honestly, do nothing.  I want to solidify sanitizing the cache as standard behavior,
at which point adding a comment implies that nested_npt_enabled() is somehow special,
i.e. that it _doesn't_ follow the standard.