Manually look for a CPUID.0x1 entry instead of bouncing through
kvm_cpuid() when retrieving the Family-Model-Stepping information for
vCPU RESET/INIT. This fixes a potential undefined behavior bug due to
kvm_cpuid() using the uninitialized "dummy" param as the ECX _input_,
a.k.a. the index.
A more minimal fix would be to simply zero "dummy", but the extra work in
kvm_cpuid() is wasteful, and KVM should be treating the FMS retrieval as
an out-of-band access, e.g. same as how KVM computes guest.MAXPHYADDR.
Both Intel's SDM and AMD's APM describe the RDX value at RESET/INIT as
holding the CPU's FMS information, not as holding CPUID.0x1.EAX. KVM's
usage of CPUID entries to get FMS is simply a pragmatic approach to avoid
having yet another way for userspace to provide inconsistent data.
No functional change intended.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/x86.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 46ee9bf61df4..14562ea5d78d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10941,9 +10941,9 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
{
+ struct kvm_cpuid_entry2 *cpuid_0x1;
unsigned long old_cr0 = kvm_read_cr0(vcpu);
unsigned long new_cr0;
- u32 eax, dummy;
/*
* Several of the "set" flows, e.g. ->set_cr0(), read other registers
@@ -11025,12 +11025,11 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
* if no CPUID match is found. Note, it's impossible to get a match at
* RESET since KVM emulates RESET before exposing the vCPU to userspace,
* i.e. it'simpossible for kvm_cpuid() to find a valid entry on RESET.
- * But, go through the motions in case that's ever remedied.
+ * But, go through the motions in case that's ever remedied. Note, the
+ * index for CPUID.0x1 is not significant, arbitrarily specify '0'.
*/
- eax = 1;
- if (!kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy, true))
- eax = 0x600;
- kvm_rdx_write(vcpu, eax);
+ cpuid_0x1 = kvm_find_cpuid_entry(vcpu, 1, 0);
+ kvm_rdx_write(vcpu, cpuid_0x1 ? cpuid_0x1->eax : 0x600);
vcpu->arch.ia32_xss = 0;
--
2.33.0.685.g46640cef36-goog
On Wed, Sep 29, 2021 at 3:24 PM Sean Christopherson <[email protected]> wrote:
>
> Manually look for a CPUID.0x1 entry instead of bouncing through
> kvm_cpuid() when retrieving the Family-Model-Stepping information for
> vCPU RESET/INIT. This fixes a potential undefined behavior bug due to
> kvm_cpuid() using the uninitialized "dummy" param as the ECX _input_,
> a.k.a. the index.
>
> A more minimal fix would be to simply zero "dummy", but the extra work in
> kvm_cpuid() is wasteful, and KVM should be treating the FMS retrieval as
> an out-of-band access, e.g. same as how KVM computes guest.MAXPHYADDR.
> Both Intel's SDM and AMD's APM describe the RDX value at RESET/INIT as
> holding the CPU's FMS information, not as holding CPUID.0x1.EAX. KVM's
> usage of CPUID entries to get FMS is simply a pragmatic approach to avoid
> having yet another way for userspace to provide inconsistent data.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Jim Mattson <[email protected]>
On 30/09/21 00:24, Sean Christopherson wrote:
> * RESET since KVM emulates RESET before exposing the vCPU to userspace,
> * i.e. it'simpossible for kvm_cpuid() to find a valid entry on RESET.
> + * But, go through the motions in case that's ever remedied. Note, the
> + * index for CPUID.0x1 is not significant, arbitrarily specify '0'.
Just one nit, this comment change is not really needed because almost
all callers are using '0' for the same reason.
But, perhaps adding kvm_find_cpuid_entry_index and removing the last
parameter from kvm_find_cpuid_entry would be a good idea.
Also, the kvm_cpuid() reference needs to be changed, which I did upon
commit.
Paolo
> */
> - eax = 1;
> - if (!kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy, true))
> - eax = 0x600;
> - kvm_rdx_write(vcpu, eax);
> + cpuid_0x1 = kvm_find_cpuid_entry(vcpu, 1, 0);
> + kvm_rdx_write(vcpu, cpuid_0x1 ? cpuid_0x1->eax : 0x600);
On 30/09/21 17:11, Sean Christopherson wrote:
>>
>> But, perhaps adding kvm_find_cpuid_entry_index and removing the last
>> parameter from kvm_find_cpuid_entry would be a good idea.
> I like this idea, but only if callers are forced to specify the index when the
> index is significant, e.g. add a magic CPUID_INDEX_DONT_CARE and WARN in
> cpuid_entry2_find() if index is significant and index == DONT_CARE.
Yeah, or it can just be that kvm_find_cpuid_entry passes -1 which acts
as the magic value.
> I'll fiddle with this, unless you want the honors?
Not really. :) Thanks,
Paolo
On Thu, Sep 30, 2021, Paolo Bonzini wrote:
> On 30/09/21 00:24, Sean Christopherson wrote:
> > * RESET since KVM emulates RESET before exposing the vCPU to userspace,
> > * i.e. it'simpossible for kvm_cpuid() to find a valid entry on RESET.
> > + * But, go through the motions in case that's ever remedied. Note, the
> > + * index for CPUID.0x1 is not significant, arbitrarily specify '0'.
>
> Just one nit, this comment change is not really needed because almost all
> callers are using '0' for the same reason.
>
> But, perhaps adding kvm_find_cpuid_entry_index and removing the last
> parameter from kvm_find_cpuid_entry would be a good idea.
I like this idea, but only if callers are forced to specify the index when the
index is significant, e.g. add a magic CPUID_INDEX_DONT_CARE and WARN in
cpuid_entry2_find() if index is significant and index == DONT_CARE.
I'll fiddle with this, unless you want the honors?
> Also, the kvm_cpuid() reference needs to be changed, which I did upon
> commit.
Doh, thanks!