[PATCH 0/2] KVM: x86: Fix mostly theoretical undefined behavior

[PATCH 0/2] KVM: x86: Fix mostly theoretical undefined behavior

[Sean Christopherson]

Fix a mostly theoretical undefined behavior bug due to consuming
uninitialized data when searching for a matching CPUID entry during vCPU
RESET/INIT.  The bug is mostly theoretical because it requires very
aggressive inlining from the compiler, as well as deliberate "sabotage"
from the compiler (which _is_ allowed by the C standard) in the face of
known uninitialized data.

Patch 1, the "fix" that is tagged for stable, is all kinds of gross in that
it doesn't directly address uninitialized data, and instead tweaks a low
level CPUID helper to avoid consuming the uninitialized data.  I went that
route for the fix so that the fix would be easily/directy consumable
downstream, as porting the fix from v5.15-rcN to literally any other buggy
kernel would require hand coding the fix due to refactoring and code
movement across files.

Patch 2 directly addresses the uninitialized data.

If patch 1 is unpalatable, an alternative would be to do a bit of merge
magic and feed in a fix to initialize "dummy" in svm.c, which was the only
buggy path prior to v5.15-rcN.  However, KVM lived from 2012-2020 with
what's effectively the behavior after applying patch 1, and no one noticed
that the behavior was broken in 2020 until v5.15-rc1 introduced the bad
behavior to VMX, i.e. opened up the validation surface to bots that
presumably run the majority of their cycles on Intel CPUs.

Sean Christopherson (2):
  KVM: x86: Swap order of CPUID entry "index" vs. "significant flag"
    checks
  KVM: x86: Manually retrieve CPUID.0x1 when getting FMS for RESET/INIT

 arch/x86/kvm/cpuid.c |  4 ++--
 arch/x86/kvm/x86.c   | 11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)

-- 
2.33.0.685.g46640cef36-goog

[PATCH 1/2] KVM: x86: Swap order of CPUID entry "index" vs. "significant flag" checks

[Sean Christopherson]

Check whether a CPUID entry's index is significant before checking for a
matching index to hack-a-fix an undefined behavior bug due to consuming
uninitialized data.  RESET/INIT emulation uses kvm_cpuid() to retrieve
CPUID.0x1, which does _not_ have a significant index, and fails to
initialize the dummy variable that doubles as EBX/ECX/EDX output _and_
ECX, a.k.a. index, input.

Practically speaking, it's _extremely_  unlikely any compiler will yield
code that causes problems, as the compiler would need to inline the
kvm_cpuid() call to detect the uninitialized data, and intentionally hose
the kernel, e.g. insert ud2, instead of simply ignoring the result of
the index comparison.

Although the sketchy "dummy" pattern was introduced in SVM by commit
66f7b72e1171 ("KVM: x86: Make register state after reset conform to
specification"), it wasn't actually broken until commit 7ff6c0350315
("KVM: x86: Remove stateful CPUID handling") arbitrarily swapped the
order of operations such that "index" was checked before the significant
flag.

Avoid consuming uninitialized data by reverting to checking the flag
before the index purely so that the fix can be easily backported; the
offending RESET/INIT code has been refactored, moved, and consolidated
from vendor code to common x86 since the bug was introduced.  A future
patch will directly address the bad RESET/INIT behavior.

The undefined behavior was detected by syzbot + KernelMemorySanitizer.

  BUG: KMSAN: uninit-value in cpuid_entry2_find arch/x86/kvm/cpuid.c:68
  BUG: KMSAN: uninit-value in kvm_find_cpuid_entry arch/x86/kvm/cpuid.c:1103
  BUG: KMSAN: uninit-value in kvm_cpuid+0x456/0x28f0 arch/x86/kvm/cpuid.c:1183
   cpuid_entry2_find arch/x86/kvm/cpuid.c:68 [inline]
   kvm_find_cpuid_entry arch/x86/kvm/cpuid.c:1103 [inline]
   kvm_cpuid+0x456/0x28f0 arch/x86/kvm/cpuid.c:1183
   kvm_vcpu_reset+0x13fb/0x1c20 arch/x86/kvm/x86.c:10885
   kvm_apic_accept_events+0x58f/0x8c0 arch/x86/kvm/lapic.c:2923
   vcpu_enter_guest+0xfd2/0x6d80 arch/x86/kvm/x86.c:9534
   vcpu_run+0x7f5/0x18d0 arch/x86/kvm/x86.c:9788
   kvm_arch_vcpu_ioctl_run+0x245b/0x2d10 arch/x86/kvm/x86.c:10020

  Local variable ----dummy@kvm_vcpu_reset created at:
   kvm_vcpu_reset+0x1fb/0x1c20 arch/x86/kvm/x86.c:10812
   kvm_apic_accept_events+0x58f/0x8c0 arch/x86/kvm/lapic.c:2923

Reported-by: syzbot+f3985126b746b3d59c9d@syzkaller.appspotmail.com
Reported-by: Alexander Potapenko <glider@google.com>
Fixes: 2a24be79b6b7 ("KVM: VMX: Set EDX at INIT with CPUID.0x1, Family-Model-Stepping")
Fixes: 7ff6c0350315 ("KVM: x86: Remove stateful CPUID handling")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index a02a8b0408ff..2d70edb0f323 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -72,8 +72,8 @@ static inline struct kvm_cpuid_entry2 *cpuid_entry2_find(
 	for (i = 0; i < nent; i++) {
 		e = &entries[i];
 
-		if (e->function == function && (e->index == index ||
-		    !(e->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)))
+		if (e->function == function &&
+		    (!(e->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX) || e->index == index))
 			return e;
 	}
 
-- 
2.33.0.685.g46640cef36-goog

Re: [PATCH 1/2] KVM: x86: Swap order of CPUID entry "index" vs. "significant flag" checks

[Jim Mattson]

On Wed, Sep 29, 2021 at 3:24 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Check whether a CPUID entry's index is significant before checking for a
> matching index to hack-a-fix an undefined behavior bug due to consuming
> uninitialized data.  RESET/INIT emulation uses kvm_cpuid() to retrieve
> CPUID.0x1, which does _not_ have a significant index, and fails to
> initialize the dummy variable that doubles as EBX/ECX/EDX output _and_
> ECX, a.k.a. index, input.
>
> Practically speaking, it's _extremely_  unlikely any compiler will yield
> code that causes problems, as the compiler would need to inline the
> kvm_cpuid() call to detect the uninitialized data, and intentionally hose
> the kernel, e.g. insert ud2, instead of simply ignoring the result of
> the index comparison.
>
> Although the sketchy "dummy" pattern was introduced in SVM by commit
> 66f7b72e1171 ("KVM: x86: Make register state after reset conform to
> specification"), it wasn't actually broken until commit 7ff6c0350315
> ("KVM: x86: Remove stateful CPUID handling") arbitrarily swapped the
> order of operations such that "index" was checked before the significant
> flag.
>
> Avoid consuming uninitialized data by reverting to checking the flag
> before the index purely so that the fix can be easily backported; the
> offending RESET/INIT code has been refactored, moved, and consolidated
> from vendor code to common x86 since the bug was introduced.  A future
> patch will directly address the bad RESET/INIT behavior.
>
> The undefined behavior was detected by syzbot + KernelMemorySanitizer.
>
>   BUG: KMSAN: uninit-value in cpuid_entry2_find arch/x86/kvm/cpuid.c:68
>   BUG: KMSAN: uninit-value in kvm_find_cpuid_entry arch/x86/kvm/cpuid.c:1103
>   BUG: KMSAN: uninit-value in kvm_cpuid+0x456/0x28f0 arch/x86/kvm/cpuid.c:1183
>    cpuid_entry2_find arch/x86/kvm/cpuid.c:68 [inline]
>    kvm_find_cpuid_entry arch/x86/kvm/cpuid.c:1103 [inline]
>    kvm_cpuid+0x456/0x28f0 arch/x86/kvm/cpuid.c:1183
>    kvm_vcpu_reset+0x13fb/0x1c20 arch/x86/kvm/x86.c:10885
>    kvm_apic_accept_events+0x58f/0x8c0 arch/x86/kvm/lapic.c:2923
>    vcpu_enter_guest+0xfd2/0x6d80 arch/x86/kvm/x86.c:9534
>    vcpu_run+0x7f5/0x18d0 arch/x86/kvm/x86.c:9788
>    kvm_arch_vcpu_ioctl_run+0x245b/0x2d10 arch/x86/kvm/x86.c:10020
>
>   Local variable ----dummy@kvm_vcpu_reset created at:
>    kvm_vcpu_reset+0x1fb/0x1c20 arch/x86/kvm/x86.c:10812
>    kvm_apic_accept_events+0x58f/0x8c0 arch/x86/kvm/lapic.c:2923
>
> Reported-by: syzbot+f3985126b746b3d59c9d@syzkaller.appspotmail.com
> Reported-by: Alexander Potapenko <glider@google.com>
> Fixes: 2a24be79b6b7 ("KVM: VMX: Set EDX at INIT with CPUID.0x1, Family-Model-Stepping")
> Fixes: 7ff6c0350315 ("KVM: x86: Remove stateful CPUID handling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

[PATCH 2/2] KVM: x86: Manually retrieve CPUID.0x1 when getting FMS for RESET/INIT

[Sean Christopherson]

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 <seanjc@google.com>
---
 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

Re: [PATCH 2/2] KVM: x86: Manually retrieve CPUID.0x1 when getting FMS for RESET/INIT

[Jim Mattson]

On Wed, Sep 29, 2021 at 3:24 PM Sean Christopherson <seanjc@google.com> 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 <seanjc@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH 2/2] KVM: x86: Manually retrieve CPUID.0x1 when getting FMS for RESET/INIT

[Paolo Bonzini]

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);

Re: [PATCH 2/2] KVM: x86: Manually retrieve CPUID.0x1 when getting FMS for RESET/INIT

[Sean Christopherson]

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!

Re: [PATCH 2/2] KVM: x86: Manually retrieve CPUID.0x1 when getting FMS for RESET/INIT

[Paolo Bonzini]

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