On Tue, 2024-07-09 at 12:46 -0700, Sean Christopherson wrote:
> On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> > > /*
> > > * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
> > > * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
> > > @@ -440,6 +440,15 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
> > > * whether the supplied CPUID data is equal to what's already set.
> > > */
> > > if (kvm_vcpu_has_run(vcpu)) {
> > > + /*
> > > + * Note, runtime CPUID updates may consume other CPUID-driven
> > > + * vCPU state, e.g. KVM or Xen CPUID bases. Updating runtime
> > > + * state before full CPUID processing is functionally correct
> > > + * only because any change in CPUID is disallowed, i.e. using
> > > + * stale data is ok because KVM will reject the change.
> > > + */
> >
> > If I understand correctly the sole reason for the below
> > __kvm_update_cpuid_runtime is to ensure that kvm_cpuid_check_equal doesn't
> > fail because current cpuid also was post-processed with runtime updates.
>
> Yep.
>
> > Can we have a comment stating this? Or even better how about moving the
> > call to __kvm_update_cpuid_runtime into the kvm_cpuid_check_equal,
> > to emphasize this?
>
> Ya, I'll do both.
>
> > > + __kvm_update_cpuid_runtime(vcpu, e2, nent);
> > > +
> > > r = kvm_cpuid_check_equal(vcpu, e2, nent);
> > > if (r)
> > > return r;
> >
> >
> > Overall I am not 100% sure what is better:
> >
> > Before the patch it was roughly like this:
> >
> > 1. Post process the user given cpuid with bits of KVM runtime state (like xcr0)
> > At that point the vcpu->arch.cpuid_entries is stale but consistent, it is just old CPUID.
> >
> > 2. kvm_hv_vcpu_init call (IMHO this call can be moved to kvm_vcpu_after_set_cpuid)
> >
> > 3. kvm_check_cpuid on the user provided cpuid
> >
> > 4. Update the vcpu->arch.cpuid_entries with new and post processed cpuid
> >
> > 5. kvm_get_hypervisor_cpuid - I think this also can be cosmetically moved to kvm_vcpu_after_set_cpuid
> >
> > 6. kvm_vcpu_after_set_cpuid itself.
> >
> >
> > After this change it works like that:
> >
> > 1. kvm_hv_vcpu_init (again this belongs more to kvm_vcpu_after_set_cpuid)
> > 2. kvm_check_cpuid on the user cpuid without post processing - in theory this can cause bugs
> > 3. Update the vcpu->arch.cpuid_entries with new cpuid but without post-processing
> > 4. kvm_get_hypervisor_cpuid
> > 5. kvm_update_cpuid_runtime
> > 6. The old kvm_vcpu_after_set_cpuid
> >
> > I'm honestly not sure what is better but IMHO moving the kvm_hv_vcpu_init and
> > kvm_get_hypervisor_cpuid into kvm_vcpu_after_set_cpuid would clean up this
> > mess a bit regardless of this patch.
>
> It takes many more patches, but doing the swap() allows for the removal of several
> APIs that poke into a "raw" kvm_cpuid_entry2 array, and by the end of the series
> (with your above feedback addressed) the code gets to (sans comments):
>
> swap(vcpu->arch.cpuid_entries, e2);
> swap(vcpu->arch.cpuid_nent, nent);
>
> memcpy(vcpu_caps, vcpu->arch.cpu_caps, sizeof(vcpu_caps));
> BUILD_BUG_ON(sizeof(vcpu_caps) != sizeof(vcpu->arch.cpu_caps));
>
> if (kvm_vcpu_has_run(vcpu)) {
> r = kvm_cpuid_check_equal(vcpu, e2, nent);
> if (r)
> goto err;
> goto success;
> }
>
> Those are really just bonuses though, the main goal is to prevent recurrences of
> bugs where KVM consumes stale vCPU state[*], which is what prompted this change.
>
> [*] https://lore.kernel.org/all/20240228101837.93642-1-vkuznets@xxxxxxxxxx
>
All makes sense, thanks!
Best regards,
Maxim Levitsky
