Re: [PATCH v2 04/13] KVM: nSVM: Fix consistency checks for NP_ENABLE

[Yosry Ahmed]

On Fri, Dec 12, 2025 at 10:32:23AM -0800, Sean Christopherson wrote:
> On Tue, Dec 09, 2025, Yosry Ahmed wrote:
> > On Tue, Dec 09, 2025 at 10:42:21AM -0800, Sean Christopherson wrote:
> > > 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.
> > 
> > Does this apply to patch 12 as well? In that patch I int_vector,
> 
> I <something>?

I "sanitize" int_vector..

Sorry :D

> 
> > int_state, and event_inj when copying them to VMCB02 in
> > nested_vmcb02_prepare_control(). Mainly because
> > nested_vmcb02_prepare_control() already kinda filters what to copy from
> > VMCB12 (e.g. int_ctl), so it seemed like a better fit.
> > 
> > Do I keep that as-is, or do you prefer that I also sanitize these fields
> > when copying to the cache in nested_copy_vmcb_control_to_cache()?
> 
> I don't think I follow.  What would the sanitization look like?  Note, I don't
> think we need to completely sanitize _every_ field.  The key fields are ones
> where KVM consumes and/or acts on the field.

Patch 12 currently sanitizes what is copied from VMCB12 to VMCB02 for
int_vector, int_state, and event_inj in nested_vmcb02_prepare_control():

@@ -890,9 +893,9 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 		(svm->nested.ctl.int_ctl & int_ctl_vmcb12_bits) |
 		(vmcb01->control.int_ctl & int_ctl_vmcb01_bits);

-	vmcb02->control.int_vector          = svm->nested.ctl.int_vector;
-	vmcb02->control.int_state           = svm->nested.ctl.int_state;
-	vmcb02->control.event_inj           = svm->nested.ctl.event_inj;
+	vmcb02->control.int_vector          = svm->nested.ctl.int_vector & SVM_INT_VECTOR_MASK;
+	vmcb02->control.int_state           = svm->nested.ctl.int_state & SVM_INTERRUPT_SHADOW_MASK;
+	vmcb02->control.event_inj           = svm->nested.ctl.event_inj & ~SVM_EVTINJ_RESERVED_BITS;
 	vmcb02->control.event_inj_err       = svm->nested.ctl.event_inj_err;

My question was: given this:

> I want to solidify sanitizing the cache as standard behavior

Do you prefer that I move this sanitization when copying from L1's
VMCB12 to the cached VMCB12 in nested_copy_vmcb_control_to_cache()?

I initially made it part of nested_vmcb02_prepare_control() as it
already filters what to pick from the VMCB12 for some other related
fields like int_ctl based on what features are exposed to the guest.