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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALMp9eS4S1d-8nSdJDG8E1unemVB06=cb3_OWSaVivPJmk63PQ@mail.gmail.com>
Date:   Wed, 29 Sep 2021 15:51:16 -0700
From:   Jim Mattson <jmattson@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        syzbot+f3985126b746b3d59c9d@...kaller.appspotmail.com,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH 1/2] KVM: x86: Swap order of CPUID entry "index" vs.
 "significant flag" checks

On Wed, Sep 29, 2021 at 3:24 PM Sean Christopherson <seanjc@...gle.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@..._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@...kaller.appspotmail.com
> Reported-by: Alexander Potapenko <glider@...gle.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@...r.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
Reviewed-by: Jim Mattson <jmattson@...gle.com>