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

[Yosry Ahmed]

[..]
> > For the remaining fields, make sure only defined bits are copied from
> > L1's VMCB12 into KVM'cache by defining appropriate masks where needed.
> > The only exception is tlb_ctl, which is unused, so remove it.
> >
> > Opportunistically cleanup ignoring the lower bits of {io/msr}pm_base_pa
> > in __nested_copy_vmcb_control_to_cache() by using PAGE_MASK. Also, move
> > the ASID copying ahead with other special cases, and expand the comment
> > about the ASID being copied only for consistency checks.
>
> Stop. Bundling. Changes.
>
> This is not a hypothetical situation, bundling small changes like this is quite
> literally making review take 3-4x longer than it should.
>
> The interrupt changes are trivial to review.
>
> The I/O and MSR bitmap changes are also easy enough, but I wanted to double that
> PAGE_MASK does indeed equal ~0x0fffULL (__PHYSICAL_MASK is the one that can be dynamic).
>
> I disagree the the tlb_ctl change.
>
> Moving the ASID handling is _completely_ superfluous.
>
> Combining any two of those is annoying to deal with.  Combining all of them wastes
> a non-trivial amount of time.  What should have taken me ~5 minutes to review is
> dragging into 20+ minutes, because I keep having to cross-reference the changelog
> with the code to understand WTF is going on.

Sigh, I kept it as-is because these changes have been together since
the first or second version. You did mention cleaning up the
definitions being split into a different patch (which I did, and then
we dropped it entirely), but I thought keeping the asid and tlb_ctl
changes here were fine.

[..]
> > @@ -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?