Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210929222426.1855730-1-seanjc@google.com> <20210929222426.1855730-2-seanjc@google.com>
In-Reply-To: <20210929222426.1855730-2-seanjc@google.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Wed, 29 Sep 2021 15:51:16 -0700
Message-ID: <CALMp9eS4S1d-8nSdJDG8E1unemVB06=cb3_OWSaVivPJmk63PQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] KVM: x86: Swap order of CPUID entry "index" vs.
 "significant flag" checks
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        syzbot+f3985126b746b3d59c9d@syzkaller.appspotmail.com,
        Alexander Potapenko <glider@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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>