| Message ID | 20240517173926.965351-11-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID overhaul, fixes, and caching | expand |
On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > Drop the manual initialization of maxphyaddr and reserved_gpa_bits during > vCPU creation now that kvm_arch_vcpu_create() unconditionally invokes > kvm_vcpu_after_set_cpuid(), which handles all such CPUID caching. > > None of the helpers between the existing code in kvm_arch_vcpu_create() > and the call to kvm_vcpu_after_set_cpuid() consume maxphyaddr or > reserved_gpa_bits (though auditing vmx_vcpu_create() and svm_vcpu_create() > isn't exactly easy). And even if that weren't the case, KVM _must_ > refresh any affected state during kvm_vcpu_after_set_cpuid(), e.g. to > correctly handle KVM_SET_CPUID2. In other words, this can't introduce a > new bug, only expose an existing bug (of which there don't appear to be > any). IMHO the change is not as bulletproof as claimed: If some code does access the uninitialized state (e.g vcpu->arch.maxphyaddr which will be zero, I assume), in between these calls, then even though later the correct CPUID will be set and should override the incorrect state set earlier, the problem *is* that the mentioned code will have to deal with non architecturally possible value (e.g maxphyaddr == 0) which might cause a bug in it. Of course such code currently doesn't exist, so it works but it can fail in the future. How about we move the call to kvm_vcpu_after_set_cpuid upward? Best regards, Maxim Levitsky > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/x86.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 2f6dda723005..bb34891d2f0a 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -12190,9 +12190,6 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) > goto free_emulate_ctxt; > } > > - vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); > - vcpu->arch.reserved_gpa_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu); > - > vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; > > kvm_async_pf_hash_reset(vcpu);
On Thu, Jul 04, 2024, Maxim Levitsky wrote: > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > > Drop the manual initialization of maxphyaddr and reserved_gpa_bits during > > vCPU creation now that kvm_arch_vcpu_create() unconditionally invokes > > kvm_vcpu_after_set_cpuid(), which handles all such CPUID caching. > > > > None of the helpers between the existing code in kvm_arch_vcpu_create() > > and the call to kvm_vcpu_after_set_cpuid() consume maxphyaddr or > > reserved_gpa_bits (though auditing vmx_vcpu_create() and svm_vcpu_create() > > isn't exactly easy). And even if that weren't the case, KVM _must_ > > refresh any affected state during kvm_vcpu_after_set_cpuid(), e.g. to > > correctly handle KVM_SET_CPUID2. In other words, this can't introduce a > > new bug, only expose an existing bug (of which there don't appear to be > > any). > > > IMHO the change is not as bulletproof as claimed: > > If some code does access the uninitialized state (e.g vcpu->arch.maxphyaddr > which will be zero, I assume), in between these calls, then even though later > the correct CPUID will be set and should override the incorrect state set > earlier, the problem *is* that the mentioned code will have to deal with non > architecturally possible value (e.g maxphyaddr == 0) which might cause a bug > in it. > > Of course such code currently doesn't exist, so it works but it can fail in > the future. Similar to not consuming a null cpuid_entries, any such future bug should never escape developer testing since this is a very fixed sequence. And practically speaking, completely closing these holes isn't feasible because it's impossible to initialize everything simultaneously, i.e. some amount of code will always need to execute with zero-initialized vCPU state. > How about we move the call to kvm_vcpu_after_set_cpuid upward? A drop-in replacement was my preference too, but it doesn't work. :-/ kvm_vcpu_after_set_cpuid() needs to be called after vcpu_load(), e.g. VMX's hook will do VMWRITE.
On Mon, 2024-07-08 at 12:53 -0700, Sean Christopherson wrote: > On Thu, Jul 04, 2024, Maxim Levitsky wrote: > > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > > > Drop the manual initialization of maxphyaddr and reserved_gpa_bits during > > > vCPU creation now that kvm_arch_vcpu_create() unconditionally invokes > > > kvm_vcpu_after_set_cpuid(), which handles all such CPUID caching. > > > > > > None of the helpers between the existing code in kvm_arch_vcpu_create() > > > and the call to kvm_vcpu_after_set_cpuid() consume maxphyaddr or > > > reserved_gpa_bits (though auditing vmx_vcpu_create() and svm_vcpu_create() > > > isn't exactly easy). And even if that weren't the case, KVM _must_ > > > refresh any affected state during kvm_vcpu_after_set_cpuid(), e.g. to > > > correctly handle KVM_SET_CPUID2. In other words, this can't introduce a > > > new bug, only expose an existing bug (of which there don't appear to be > > > any). > > > > IMHO the change is not as bulletproof as claimed: > > > > If some code does access the uninitialized state (e.g vcpu->arch.maxphyaddr > > which will be zero, I assume), in between these calls, then even though later > > the correct CPUID will be set and should override the incorrect state set > > earlier, the problem *is* that the mentioned code will have to deal with non > > architecturally possible value (e.g maxphyaddr == 0) which might cause a bug > > in it. > > > > Of course such code currently doesn't exist, so it works but it can fail in > > the future. > > Similar to not consuming a null cpuid_entries, any such future bug should never > escape developer testing since this is a very fixed sequence. And practically > speaking, completely closing these holes isn't feasible because it's impossible > to initialize everything simultaneously, i.e. some amount of code will always > need to execute with zero-initialized vCPU state. > > > How about we move the call to kvm_vcpu_after_set_cpuid upward? > > A drop-in replacement was my preference too, but it doesn't work. :-/ > kvm_vcpu_after_set_cpuid() needs to be called after vcpu_load(), e.g. VMX's > hook will do VMWRITE. > Let it be then, but let's at least drop the part of the commit message after 'And even if that weren't the case', just not to confuse future reader, because as I explained, this is not 100% bulletproof. Best regards, Maxim Levitsky
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 2f6dda723005..bb34891d2f0a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12190,9 +12190,6 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) goto free_emulate_ctxt; } - vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); - vcpu->arch.reserved_gpa_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu); - vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; kvm_async_pf_hash_reset(vcpu);
Drop the manual initialization of maxphyaddr and reserved_gpa_bits during vCPU creation now that kvm_arch_vcpu_create() unconditionally invokes kvm_vcpu_after_set_cpuid(), which handles all such CPUID caching. None of the helpers between the existing code in kvm_arch_vcpu_create() and the call to kvm_vcpu_after_set_cpuid() consume maxphyaddr or reserved_gpa_bits (though auditing vmx_vcpu_create() and svm_vcpu_create() isn't exactly easy). And even if that weren't the case, KVM _must_ refresh any affected state during kvm_vcpu_after_set_cpuid(), e.g. to correctly handle KVM_SET_CPUID2. In other words, this can't introduce a new bug, only expose an existing bug (of which there don't appear to be any). Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/x86.c | 3 --- 1 file changed, 3 deletions(-)