Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx > Cc: Robert Dinse <nanook@xxxxxxxxxx> > Cc: Kees Cook <keescook@xxxxxxxxxxxx> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> > --- > 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
