• Sean Christopherson's avatar
    KVM: x86: Swap order of CPUID entry "index" vs. "significant flag" checks · e8a747d0
    Sean Christopherson authored
    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
    66f7b72e ("KVM: x86: Make register state after reset conform to
    specification"), it wasn't actually broken until commit 7ff6c035
    ("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: default avatarAlexander Potapenko <glider@google.com>
    Fixes: 2a24be79 ("KVM: VMX: Set EDX at INIT with CPUID.0x1, Family-Model-Stepping")
    Fixes: 7ff6c035 ("KVM: x86: Remove stateful CPUID handling")
    Cc: stable@vger.kernel.org
    Signed-off-by: default avatarSean Christopherson <seanjc@google.com>
    Reviewed-by: default avatarJim Mattson <jmattson@google.com>
    Message-Id: <20210929222426.1855730-2-seanjc@google.com>
    Signed-off-by: default avatarPaolo Bonzini <pbonzini@redhat.com>
    e8a747d0
cpuid.c 33.3 KB