linux-kernel - Re: [PATCH 2/4] KVM: x86: Harden _regs accesses to guard against buggy input

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: <87r14gqte2.fsf@redhat.com>
Date:   Thu, 26 May 2022 16:07:49 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Robert Dinse <nanook@...imo.com>,
        Kees Cook <keescook@...omium.org>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH 2/4] KVM: x86: Harden _regs accesses to guard against
 buggy input

Sean Christopherson <seanjc@...gle.com> writes:

> WARN and truncate the incoming GPR number/index when reading/writing GPRs
> in the emulator to guard against KVM bugs, e.g. to avoid out-of-bounds
> accesses to ctxt->_regs[] if KVM generates a bogus index.  Truncate the
> index instead of returning e.g. zero, as reg_write() returns a pointer
> to the register, i.e. returning zero would result in a NULL pointer
> dereference.  KVM could also force the index to any arbitrary GPR, but
> that's no better or worse, just different.
>
> Open code the restriction to 16 registers; RIP is handled via _eip and
> should never be accessed through reg_read() or reg_write().  See the
> comments above the declarations of reg_read() and reg_write(), and the
> behavior of writeback_registers().  The horrific open coded mess will be
> cleaned up in a future commit.
>
> There are no such bugs known to exist in the emulator, but determining
> that KVM is bug-free is not at all simple and requires a deep dive into
> the emulator.  The code is so convoluted that GCC-12 with the recently
> enable -Warray-bounds spits out a (suspected) false-positive:
>
>   arch/x86/kvm/emulate.c:254:27: warning: array subscript 32 is above array
>                                  bounds of 'long unsigned int[17]' [-Warray-bounds]
>     254 |         return ctxt->_regs[nr];
>         |                ~~~~~~~~~~~^~~~
>   In file included from arch/x86/kvm/emulate.c:23:
>   arch/x86/kvm/kvm_emulate.h: In function 'reg_rmw':
>   arch/x86/kvm/kvm_emulate.h:366:23: note: while referencing '_regs'
>     366 |         unsigned long _regs[NR_VCPU_REGS];
>         |                       ^~~~~
>
> Link: https://lore.kernel.org/all/YofQlBrlx18J7h9Y@google.com
> Cc: Robert Dinse <nanook@...imo.com>
> Cc: Kees Cook <keescook@...omium.org>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/emulate.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 7226a127ccb4..c58366ae4da2 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -247,6 +247,9 @@ enum x86_transfer_type {
>  
>  static ulong reg_read(struct x86_emulate_ctxt *ctxt, unsigned nr)
>  {
> +	if (WARN_ON_ONCE(nr >= 16))
> +		nr &= 16 - 1;

As the result of this is unlikely to match the expectation (and I'm
unsure what's the expectation here in the first place :-), why not use 
KVM_BUG_ON() here instead?

> +
>  	if (!(ctxt->regs_valid & (1 << nr))) {
>  		ctxt->regs_valid |= 1 << nr;
>  		ctxt->_regs[nr] = ctxt->ops->read_gpr(ctxt, nr);
> @@ -256,6 +259,9 @@ static ulong reg_read(struct x86_emulate_ctxt *ctxt, unsigned nr)
>  
>  static ulong *reg_write(struct x86_emulate_ctxt *ctxt, unsigned nr)
>  {
> +	if (WARN_ON_ONCE(nr >= 16))
> +		nr &= 16 - 1;
> +
>  	ctxt->regs_valid |= 1 << nr;
>  	ctxt->regs_dirty |= 1 << nr;
>  	return &ctxt->_regs[nr];

-- 
Vitaly