Re: [PATCH v5 25/26] KVM: nSVM: Sanitize control fields copied from VMCB12

[Sean Christopherson]

On Mon, Feb 23, 2026, Yosry Ahmed wrote:
> > > @@ -499,32 +499,35 @@ void __nested_copy_vmcb_control_to_cache(struct kvm_vcpu *vcpu,
> > >       if (!guest_cpu_cap_has(vcpu, X86_FEATURE_NPT))
> > >               to->misc_ctl &= ~SVM_MISC_ENABLE_NP;
> > >
> > > -     to->iopm_base_pa        = from->iopm_base_pa;
> > > -     to->msrpm_base_pa       = from->msrpm_base_pa;
> > > +     /*
> > > +      * Copy the ASID here because nested_vmcb_check_controls() will check
> > > +      * it.  The ASID could be invalid, or conflict with another VM's ASID ,
> >
> > Spurious space before the command.
> >
> > > +      * so it should never be used directly to run L2.
> > > +      */
> > > +     to->asid = from->asid;
> > > +
> > > +     /* Lower bits of IOPM_BASE_PA and MSRPM_BASE_PA are ignored */
> > > +     to->iopm_base_pa        = from->iopm_base_pa & PAGE_MASK;
> > > +     to->msrpm_base_pa       = from->msrpm_base_pa & PAGE_MASK;
> > >>      to->tsc_offset          = from->tsc_offset;
> > > -     to->tlb_ctl             = from->tlb_ctl;
> >
> > I don't think we should completely drop tlb_ctl.  KVM doesn't do anything with
> > vmcb12's tlb_ctl only because we haven't addressed the TODO list in
> > nested_svm_transition_tlb_flush().  I think I would rather update this code to
> > sanitize the field now, as opposed to waiting until we address that TODO.
> >
> > KVM advertises X86_FEATURE_FLUSHBYASID, so I think we can do the right thing
> > without having to speculate on what the future will bring.
> >
> > Alternatively, we could add a TODO here or update the one in
> > nested_svm_transition_tlb_flush(), but that seems like more overall work than
> > just hardening the code.
> 
> I will drop the ASID change.
> 
> I honestly don't know where to draw the line at this point. Should I
> split sanitizing all different fields into different patches? Or just
> split the tlb_ctl change? What about the I/O and MSR bitmap change?

FWIW, my rule of thumb is that, when writing the changelog, if I find myself
either (a) having to explain mostly unrelated things or (b) generalizing away
details, then I should split the patch.

In this case, I would do:

  1. int_ctl and friends
  2. tlb_cnt (with a changelog explaining why we're keeping it even though it's unused)
  3. ~0xfffull => PAGE_MASK cleanup
  4. ASID comment

Though honestly, I would drop the ASID change.  The comment is misleading and
arguably flat out wrong.  This

  The ASID could be invalid, or conflict with another VM's ASID, so it should
  never be used directly to run L2.

isn't why KVM musn't use the ASID from vmcb12 verbatim, the real reason is that
the ASID space managed by KVM is logical different than the one managed by L1.
I.e. KVM needs to shadow L1's ASID space, for all intents and purposes.  And so
KVM should never use vmcb12's ASID verbatim because it needs to be "translated".