Message ID | 20220525222604.2810054-3-seanjc@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: x86: Emulator _regs fixes and cleanups | expand |
Sean Christopherson <seanjc@google.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@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; 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];
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
On Thu, May 26, 2022, Vitaly Kuznetsov wrote: > Sean Christopherson <seanjc@google.com> writes: > > --- > > 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? ctxt->vcpu is a 'void *' due to the (IMO futile) separation of the emulator from regular KVM. I.e. this doesn't have access to the 'kvm'.
Sean Christopherson <seanjc@google.com> writes: > On Thu, May 26, 2022, Vitaly Kuznetsov wrote: >> Sean Christopherson <seanjc@google.com> writes: >> > --- >> > 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? > > ctxt->vcpu is a 'void *' due to the (IMO futile) separation of the emulator from > regular KVM. I.e. this doesn't have access to the 'kvm'. Well, if we're not emulating something correctly for whatever reason, killing the VM is likely the right thing to do so I'm going to vote for abandoning the futility, making ctxt->vcpu 'struct kvm_vcpu *' and doing KVM_BUG_ON(). (Not necessarily now, the patch looks good to me).
On Thu, May 26, 2022, Kees Cook wrote: > On Wed, May 25, 2022 at 10:26:02PM +0000, Sean Christopherson wrote: > > 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? Eh, IMO it doesn't provide any meaningful value, and requires documenting why the emulator allocates an extra register. The guest is still going to experience data loss/corruption if KVM drops a write or reads zeros instead whatever register it was supposed to access. I.e. the guest is equally hosed either way. One idea along the lines of Vitaly's idea of KVM_BUG_ON() would be to add an emulator hook to bug the VM, e.g. #define KVM_EMULATOR_BUG_ON(cond, ctxt) \ ({ \ int __ret = (cond); \ \ if (WARN_ON_ONCE(__ret)) \ ctxt->ops->vm_bugged(ctxt); \ unlikely(__ret); \ }) to workaround not having access to the 'struct kvm_vcpu' in the emulator. The bad access will still go through, but the VM will be killed before the vCPU can re-enter the guest and do more damage.
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; + 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];
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@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(+)