linux-kernel - Re: [PATCH v2 0/8] KVM: x86: Emulator

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] [day] [month] [year] [list]

Message-ID: <efa20602-b17b-adac-927f-fa38aa6d22d6@redhat.com>
Date:   Wed, 8 Jun 2022 14:23:08 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>,
        Robert Dinse <nanook@...imo.com>
Subject: Re: [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups

On 5/26/22 23:08, Sean Christopherson wrote:
> Clean up and harden the use of the x86_emulate_ctxt._regs, which is
> surrounded by a fair bit of magic.  This series was prompted by bug reports
> by Kees and Robert where GCC-12 flags an out-of-bounds _regs access.  The
> warning is a false positive due to a now-known GCC bug, but it's cheap and
> easy to harden the _regs usage, and doing so minimizing the risk of more
> precisely handling 32-bit vs. 64-bit GPRs.
> 
> I didn't tag patch 2 with Fixes or Cc: stable@.  It does remedy the
> GCC-12 warning, but AIUI the GCC-12 bug affects other KVM paths that
> already have explicit guardrails, i.e. fixing this one case doesn't
> guarantee happiness when building with CONFIG_KVM_WERROR=y, let alone
> CONFIG_WERROR=y.  That said, it might be worth sending to the v5.18 stable
> tree[*] as it does appear to make some configs/setups happy.
> 
> [*] KVM hasn't changed, but the warning=>error was introduced in v5.18 by
>     commit e6148767825c ("Makefile: Enable -Warray-bounds").
> 
> v2:
>    - Collect reviews and tests. [Vitaly, Kees, Robert]
>    - Tweak patch 1's changelog to explicitly call out that dirty_regs is a
>      4 byte field. [Vitaly]
>    - Add Reported-by for Kees and Robert since this does technically fix a
>      build breakage.
>    - Use a raw literal for NR_EMULATOR_GPRS instead of VCPU_REGS_R15+1 to
>      play nice with 32-bit builds. [kernel test robot]
>    - Reduce the number of emulated GPRs to 8 for 32-bit builds.
>    - Add and use KVM_EMULATOR_BUG_ON() to bug/kill the VM when an emulator
>      bug is detected.  [Vitaly]
> 
> v1: https://lore.kernel.org/all/20220525222604.2810054-1-seanjc@google.com
> 
> Sean Christopherson (8):
>    KVM: x86: Grab regs_dirty in local 'unsigned long'
>    KVM: x86: Harden _regs accesses to guard against buggy input
>    KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array
>    KVM: x86: Use 16-bit fields to track dirty/valid emulator GPRs
>    KVM: x86: Reduce the number of emulator GPRs to '8' for 32-bit KVM
>    KVM: x86: Bug the VM if the emulator accesses a non-existent GPR
>    KVM: x86: Bug the VM if the emulator generates a bogus exception
>      vector
>    KVM: x86: Bug the VM on an out-of-bounds data read
> 
>   arch/x86/kvm/emulate.c     | 26 ++++++++++++++++++++------
>   arch/x86/kvm/kvm_emulate.h | 28 +++++++++++++++++++++++++---
>   arch/x86/kvm/x86.c         |  9 +++++++++
>   3 files changed, 54 insertions(+), 9 deletions(-)
> 
> 
> base-commit: 90bde5bea810d766e7046bf5884f2ccf76dd78e9

Queued, thanks.

Paolo