lkml.org 
[lkml]   [2022]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 2/4] KVM: x86: Harden _regs accesses to guard against buggy input
On Wed, May 25, 2022 at 10:26:02PM +0000, Sean Christopherson wrote:
> 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]

I can confirm this is one of the instances of the now-isolated GCC 12
bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679

Regardless, I think the cleanup is still useful from a robustness
perspective. Better to be as defensive as possible in KVM. :)

> 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@eskimo.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Sean Christopherson <seanjc@google.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;

Instead of doing a modulo here, what about forcing it into an "unused"
slot?

i.e. define _regs as an array of [16 + 1], and:

if (WARN_ON_ONCE(nr >= 16)
nr = 16;

Then there is both no out-of-bounds access, but also no weird "actual"
register indexed?

-Kees
--
Kees Cook

\
 
 \ /
  Last update: 2022-05-26 17:40    [W:0.708 / U:0.580 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site