All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups
@ 2022-05-26 21:08 Sean Christopherson
  2022-05-26 21:08 ` [PATCH v2 1/8] KVM: x86: Grab regs_dirty in local 'unsigned long' Sean Christopherson
                   ` (9 more replies)
  0 siblings, 10 replies; 20+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook, Robert Dinse

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
-- 
2.36.1.255.ge46751e96f-goog


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 1/8] KVM: x86: Grab regs_dirty in local 'unsigned long'
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
@ 2022-05-26 21:08 ` Sean Christopherson
  2022-05-26 21:08 ` [PATCH v2 2/8] KVM: x86: Harden _regs accesses to guard against buggy input Sean Christopherson
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook, Robert Dinse

Capture ctxt->regs_dirty in a local 'unsigned long' instead of casting it
to an 'unsigned long *' for use in for_each_set_bit().  The bitops helpers
really do read the entire 'unsigned long', even though the walking of the
read value is capped at the specified size.  I.e. 64-bit KVM is reading
memory beyond ctxt->regs_dirty, which is a u32 and thus 4 bytes, whereas
an unsigned long is 8 bytes.  Functionally it's not an issue because
regs_dirty is in the middle of x86_emulate_ctxt, i.e. KVM is just reading
its own memory, but relying on that coincidence is gross and unsafe.

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 89b11e7dca8a..7226a127ccb4 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -269,9 +269,10 @@ static ulong *reg_rmw(struct x86_emulate_ctxt *ctxt, unsigned nr)
 
 static void writeback_registers(struct x86_emulate_ctxt *ctxt)
 {
+	unsigned long dirty = ctxt->regs_dirty;
 	unsigned reg;
 
-	for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, 16)
+	for_each_set_bit(reg, &dirty, 16)
 		ctxt->ops->write_gpr(ctxt, reg, ctxt->_regs[reg]);
 }
 
-- 
2.36.1.255.ge46751e96f-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 2/8] KVM: x86: Harden _regs accesses to guard against buggy input
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
  2022-05-26 21:08 ` [PATCH v2 1/8] KVM: x86: Grab regs_dirty in local 'unsigned long' Sean Christopherson
@ 2022-05-26 21:08 ` Sean Christopherson
  2022-05-26 21:08 ` [PATCH v2 3/8] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array Sean Christopherson
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook, Robert Dinse

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 false-positive due to a GCC bug:

  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
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216026
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679
Reported-and-tested-by: Robert Dinse <nanook@eskimo.com>
Reported-by: Kees Cook <keescook@chromium.org>
Reviewed-by: 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;
+
 	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];
-- 
2.36.1.255.ge46751e96f-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 3/8] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
  2022-05-26 21:08 ` [PATCH v2 1/8] KVM: x86: Grab regs_dirty in local 'unsigned long' Sean Christopherson
  2022-05-26 21:08 ` [PATCH v2 2/8] KVM: x86: Harden _regs accesses to guard against buggy input Sean Christopherson
@ 2022-05-26 21:08 ` Sean Christopherson
  2022-05-31 18:04   ` Kees Cook
  2022-05-26 21:08 ` [PATCH v2 4/8] KVM: x86: Use 16-bit fields to track dirty/valid emulator GPRs Sean Christopherson
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook, Robert Dinse

Omit RIP from the emulator's _regs array, which is used only for GPRs,
i.e. registers that can be referenced via ModRM and/or SIB bytes.  The
emulator uses the dedicated _eip field for RIP, and manually reads from
_eip to handle RIP-relative addressing.

To avoid an even bigger, slightly more dangerous change, hardcode the
number of GPRs to 16 for the time being even though 32-bit KVM's emulator
technically should only have 8 GPRs.  Add a TODO to address that in a
future commit.

See also the comments above the read_gpr() and write_gpr() declarations,
and obviously the handling in writeback_registers().

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c     | 10 +++++-----
 arch/x86/kvm/kvm_emulate.h | 13 ++++++++++++-
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c58366ae4da2..c74c0fd3b860 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -247,8 +247,8 @@ 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 (WARN_ON_ONCE(nr >= NR_EMULATOR_GPRS))
+		nr &= NR_EMULATOR_GPRS - 1;
 
 	if (!(ctxt->regs_valid & (1 << nr))) {
 		ctxt->regs_valid |= 1 << nr;
@@ -259,8 +259,8 @@ 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;
+	if (WARN_ON_ONCE(nr >= NR_EMULATOR_GPRS))
+		nr &= NR_EMULATOR_GPRS - 1;
 
 	ctxt->regs_valid |= 1 << nr;
 	ctxt->regs_dirty |= 1 << nr;
@@ -278,7 +278,7 @@ static void writeback_registers(struct x86_emulate_ctxt *ctxt)
 	unsigned long dirty = ctxt->regs_dirty;
 	unsigned reg;
 
-	for_each_set_bit(reg, &dirty, 16)
+	for_each_set_bit(reg, &dirty, NR_EMULATOR_GPRS)
 		ctxt->ops->write_gpr(ctxt, reg, ctxt->_regs[reg]);
 }
 
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 8dff25d267b7..bc3f8295c8c8 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -301,6 +301,17 @@ struct fastop;
 
 typedef void (*fastop_t)(struct fastop *);
 
+/*
+ * The emulator's _regs array tracks only the GPRs, i.e. excludes RIP.  RIP is
+ * tracked/accessed via _eip, and except for RIP relative addressing, which
+ * also uses _eip, RIP cannot be a register operand nor can it be an operand in
+ * a ModRM or SIB byte.
+ *
+ * TODO: this is technically wrong for 32-bit KVM, which only supports 8 GPRs;
+ * R8-R15 don't exist.
+ */
+#define NR_EMULATOR_GPRS	16
+
 struct x86_emulate_ctxt {
 	void *vcpu;
 	const struct x86_emulate_ops *ops;
@@ -363,7 +374,7 @@ struct x86_emulate_ctxt {
 	struct operand src2;
 	struct operand dst;
 	struct operand memop;
-	unsigned long _regs[NR_VCPU_REGS];
+	unsigned long _regs[NR_EMULATOR_GPRS];
 	struct operand *memopp;
 	struct fetch_cache fetch;
 	struct read_cache io_read;
-- 
2.36.1.255.ge46751e96f-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 4/8] KVM: x86: Use 16-bit fields to track dirty/valid emulator GPRs
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
                   ` (2 preceding siblings ...)
  2022-05-26 21:08 ` [PATCH v2 3/8] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array Sean Christopherson
@ 2022-05-26 21:08 ` Sean Christopherson
  2022-05-26 21:08 ` [PATCH v2 5/8] KVM: x86: Reduce the number of emulator GPRs to '8' for 32-bit KVM Sean Christopherson
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook, Robert Dinse

Use a u16 instead of a u32 to track the dirty/valid status of GPRs in the
emulator.  Unlike struct kvm_vcpu_arch, x86_emulate_ctxt tracks only the
"true" GPRs, i.e. doesn't include RIP in its array, and so only needs to
track 16 registers.

Note, maxing out at 16 GPRs is a fundamental property of x86-64 and will
not change barring a massive architecture update.  Legacy x86 ModRM and
SIB encodings use 3 bits for GPRs, i.e. support 8 registers.  x86-64 uses
a single bit in the REX prefix for each possible reference type to double
the number of supported GPRs to 16 registers (4 bits).

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c     | 3 +++
 arch/x86/kvm/kvm_emulate.h | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c74c0fd3b860..5aaf1d1200af 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -262,6 +262,9 @@ static ulong *reg_write(struct x86_emulate_ctxt *ctxt, unsigned nr)
 	if (WARN_ON_ONCE(nr >= NR_EMULATOR_GPRS))
 		nr &= NR_EMULATOR_GPRS - 1;
 
+	BUILD_BUG_ON(sizeof(ctxt->regs_dirty) * BITS_PER_BYTE < NR_EMULATOR_GPRS);
+	BUILD_BUG_ON(sizeof(ctxt->regs_valid) * BITS_PER_BYTE < NR_EMULATOR_GPRS);
+
 	ctxt->regs_valid |= 1 << nr;
 	ctxt->regs_dirty |= 1 << nr;
 	return &ctxt->_regs[nr];
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index bc3f8295c8c8..3a65d6ea7fe6 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -356,9 +356,9 @@ struct x86_emulate_ctxt {
 	u8 lock_prefix;
 	u8 rep_prefix;
 	/* bitmaps of registers in _regs[] that can be read */
-	u32 regs_valid;
+	u16 regs_valid;
 	/* bitmaps of registers in _regs[] that have been written */
-	u32 regs_dirty;
+	u16 regs_dirty;
 	/* modrm */
 	u8 modrm;
 	u8 modrm_mod;
-- 
2.36.1.255.ge46751e96f-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 5/8] KVM: x86: Reduce the number of emulator GPRs to '8' for 32-bit KVM
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
                   ` (3 preceding siblings ...)
  2022-05-26 21:08 ` [PATCH v2 4/8] KVM: x86: Use 16-bit fields to track dirty/valid emulator GPRs Sean Christopherson
@ 2022-05-26 21:08 ` Sean Christopherson
  2022-05-31 18:04   ` Kees Cook
  2022-05-26 21:08 ` [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR Sean Christopherson
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook, Robert Dinse

Reduce the number of GPRs emulated by 32-bit KVM from 16 to 8.  KVM does
not support emulating 64-bit mode on 32-bit host kernels, and so should
never generate accesses to R8-15.

Opportunistically use NR_EMULATOR_GPRS in rsm_load_state_{32,64}() now
that it is precise and accurate for both flavors.

Wrap the definition with full #ifdef ugliness; sadly, IS_ENABLED()
doesn't guarantee a compile-time constant as far as BUILD_BUG_ON() is
concerned.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c     | 4 ++--
 arch/x86/kvm/kvm_emulate.h | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 5aaf1d1200af..77161f57c8d3 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2441,7 +2441,7 @@ static int rsm_load_state_32(struct x86_emulate_ctxt *ctxt,
 	ctxt->eflags =             GET_SMSTATE(u32, smstate, 0x7ff4) | X86_EFLAGS_FIXED;
 	ctxt->_eip =               GET_SMSTATE(u32, smstate, 0x7ff0);
 
-	for (i = 0; i < 8; i++)
+	for (i = 0; i < NR_EMULATOR_GPRS; i++)
 		*reg_write(ctxt, i) = GET_SMSTATE(u32, smstate, 0x7fd0 + i * 4);
 
 	val = GET_SMSTATE(u32, smstate, 0x7fcc);
@@ -2498,7 +2498,7 @@ static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
 	u16 selector;
 	int i, r;
 
-	for (i = 0; i < 16; i++)
+	for (i = 0; i < NR_EMULATOR_GPRS; i++)
 		*reg_write(ctxt, i) = GET_SMSTATE(u64, smstate, 0x7ff8 - i * 8);
 
 	ctxt->_eip   = GET_SMSTATE(u64, smstate, 0x7f78);
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 3a65d6ea7fe6..034c845b3c63 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -306,11 +306,12 @@ typedef void (*fastop_t)(struct fastop *);
  * tracked/accessed via _eip, and except for RIP relative addressing, which
  * also uses _eip, RIP cannot be a register operand nor can it be an operand in
  * a ModRM or SIB byte.
- *
- * TODO: this is technically wrong for 32-bit KVM, which only supports 8 GPRs;
- * R8-R15 don't exist.
  */
+#ifdef CONFIG_X86_64
 #define NR_EMULATOR_GPRS	16
+#else
+#define NR_EMULATOR_GPRS	8
+#endif
 
 struct x86_emulate_ctxt {
 	void *vcpu;
-- 
2.36.1.255.ge46751e96f-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
                   ` (4 preceding siblings ...)
  2022-05-26 21:08 ` [PATCH v2 5/8] KVM: x86: Reduce the number of emulator GPRs to '8' for 32-bit KVM Sean Christopherson
@ 2022-05-26 21:08 ` Sean Christopherson
  2022-05-31 18:06   ` Kees Cook
  2022-06-01  8:29   ` Vitaly Kuznetsov
  2022-05-26 21:08 ` [PATCH v2 7/8] KVM: x86: Bug the VM if the emulator generates a bogus exception vector Sean Christopherson
                   ` (3 subsequent siblings)
  9 siblings, 2 replies; 20+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook, Robert Dinse

Bug the VM, i.e. kill it, if the emulator accesses a non-existent GPR,
i.e. generates an out-of-bounds GPR index.  Continuing on all but
gaurantees some form of data corruption in the guest, e.g. even if KVM
were to redirect to a dummy register, KVM would be incorrectly read zeros
and drop writes.

Note, bugging the VM doesn't completely prevent data corruption, e.g. the
current round of emulation will complete before the vCPU bails out to
userspace.  But, the very act of killing the guest can also cause data
corruption, e.g. due to lack of file writeback before termination, so
taking on additional complexity to cleanly bail out of the emulator isn't
justified, the goal is purely to stem the bleeding and alert userspace
that something has gone horribly wrong, i.e. to avoid _silent_ data
corruption.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c     |  4 ++--
 arch/x86/kvm/kvm_emulate.h | 10 ++++++++++
 arch/x86/kvm/x86.c         |  9 +++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 77161f57c8d3..70a8e0cd9fdc 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -247,7 +247,7 @@ enum x86_transfer_type {
 
 static ulong reg_read(struct x86_emulate_ctxt *ctxt, unsigned nr)
 {
-	if (WARN_ON_ONCE(nr >= NR_EMULATOR_GPRS))
+	if (KVM_EMULATOR_BUG_ON(nr >= NR_EMULATOR_GPRS, ctxt))
 		nr &= NR_EMULATOR_GPRS - 1;
 
 	if (!(ctxt->regs_valid & (1 << nr))) {
@@ -259,7 +259,7 @@ 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 >= NR_EMULATOR_GPRS))
+	if (KVM_EMULATOR_BUG_ON(nr >= NR_EMULATOR_GPRS, ctxt))
 		nr &= NR_EMULATOR_GPRS - 1;
 
 	BUILD_BUG_ON(sizeof(ctxt->regs_dirty) * BITS_PER_BYTE < NR_EMULATOR_GPRS);
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 034c845b3c63..89246446d6aa 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -89,6 +89,7 @@ struct x86_instruction_info {
 #define X86EMUL_INTERCEPTED     6 /* Intercepted by nested VMCB/VMCS */
 
 struct x86_emulate_ops {
+	void (*vm_bugged)(struct x86_emulate_ctxt *ctxt);
 	/*
 	 * read_gpr: read a general purpose register (rax - r15)
 	 *
@@ -383,6 +384,15 @@ struct x86_emulate_ctxt {
 	bool is_branch;
 };
 
+#define KVM_EMULATOR_BUG_ON(cond, ctxt)		\
+({						\
+	int __ret = (cond);			\
+						\
+	if (WARN_ON_ONCE(__ret))		\
+		ctxt->ops->vm_bugged(ctxt);	\
+	unlikely(__ret);			\
+})
+
 /* Repeat String Operation Prefix */
 #define REPE_PREFIX	0xf3
 #define REPNE_PREFIX	0xf2
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7460b9a77d9a..e60badfbbc42 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7887,7 +7887,16 @@ static int emulator_set_xcr(struct x86_emulate_ctxt *ctxt, u32 index, u64 xcr)
 	return __kvm_set_xcr(emul_to_vcpu(ctxt), index, xcr);
 }
 
+static void emulator_vm_bugged(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm *kvm = emul_to_vcpu(ctxt)->kvm;
+
+	if (!kvm->vm_bugged)
+		kvm_vm_bugged(kvm);
+}
+
 static const struct x86_emulate_ops emulate_ops = {
+	.vm_bugged           = emulator_vm_bugged,
 	.read_gpr            = emulator_read_gpr,
 	.write_gpr           = emulator_write_gpr,
 	.read_std            = emulator_read_std,
-- 
2.36.1.255.ge46751e96f-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 7/8] KVM: x86: Bug the VM if the emulator generates a bogus exception vector
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
                   ` (5 preceding siblings ...)
  2022-05-26 21:08 ` [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR Sean Christopherson
@ 2022-05-26 21:08 ` Sean Christopherson
  2022-05-31 18:06   ` Kees Cook
  2022-06-01  8:31   ` Vitaly Kuznetsov
  2022-05-26 21:08 ` [PATCH v2 8/8] KVM: x86: Bug the VM on an out-of-bounds data read Sean Christopherson
                   ` (2 subsequent siblings)
  9 siblings, 2 replies; 20+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook, Robert Dinse

Bug the VM if KVM's emulator attempts to inject a bogus exception vector.
The guest is likely doomed even if KVM continues on, and propagating a
bad vector to the rest of KVM runs the risk of breaking other assumptions
in KVM and thus triggering a more egregious bug.

All existing users of emulate_exception() have hardcoded vector numbers
(__load_segment_descriptor() uses a few different vectors, but they're
all hardcoded), and future users are likely to follow suit, i.e. the
change to emulate_exception() is a glorified nop.

As for the ctxt->exception.vector check in x86_emulate_insn(), the few
known times the WARN has been triggered in the past is when the field was
not set when synthesizing a fault, i.e. for all intents and purposes the
check protects against consumption of uninitialized data.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 70a8e0cd9fdc..2aa17462a9ac 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -624,7 +624,9 @@ static unsigned long seg_base(struct x86_emulate_ctxt *ctxt, int seg)
 static int emulate_exception(struct x86_emulate_ctxt *ctxt, int vec,
 			     u32 error, bool valid)
 {
-	WARN_ON(vec > 0x1f);
+	if (KVM_EMULATOR_BUG_ON(vec > 0x1f, ctxt))
+		return X86EMUL_UNHANDLEABLE;
+
 	ctxt->exception.vector = vec;
 	ctxt->exception.error_code = error;
 	ctxt->exception.error_code_valid = valid;
@@ -5728,7 +5730,8 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 
 done:
 	if (rc == X86EMUL_PROPAGATE_FAULT) {
-		WARN_ON(ctxt->exception.vector > 0x1f);
+		if (KVM_EMULATOR_BUG_ON(ctxt->exception.vector > 0x1f, ctxt))
+			return EMULATION_FAILED;
 		ctxt->have_exception = true;
 	}
 	if (rc == X86EMUL_INTERCEPTED)
-- 
2.36.1.255.ge46751e96f-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 8/8] KVM: x86: Bug the VM on an out-of-bounds data read
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
                   ` (6 preceding siblings ...)
  2022-05-26 21:08 ` [PATCH v2 7/8] KVM: x86: Bug the VM if the emulator generates a bogus exception vector Sean Christopherson
@ 2022-05-26 21:08 ` Sean Christopherson
  2022-05-31 18:06   ` Kees Cook
  2022-06-01  8:39   ` Vitaly Kuznetsov
  2022-05-26 23:14 ` [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Robert Dinse
  2022-06-08 12:23 ` Paolo Bonzini
  9 siblings, 2 replies; 20+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook, Robert Dinse

Bug the VM and terminate emulation if an out-of-bounds read into the
emulator's data cache occurs.  Knowingly contuining on all but guarantees
that KVM will overwrite random kernel data, which is far, far worse than
killing the VM.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 2aa17462a9ac..39ea9138224c 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1373,7 +1373,8 @@ static int read_emulated(struct x86_emulate_ctxt *ctxt,
 	if (mc->pos < mc->end)
 		goto read_cached;
 
-	WARN_ON((mc->end + size) >= sizeof(mc->data));
+	if (KVM_EMULATOR_BUG_ON((mc->end + size) >= sizeof(mc->data), ctxt))
+		return X86EMUL_UNHANDLEABLE;
 
 	rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
 				      &ctxt->exception);
-- 
2.36.1.255.ge46751e96f-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
                   ` (7 preceding siblings ...)
  2022-05-26 21:08 ` [PATCH v2 8/8] KVM: x86: Bug the VM on an out-of-bounds data read Sean Christopherson
@ 2022-05-26 23:14 ` Robert Dinse
  2022-06-08 12:23 ` Paolo Bonzini
  9 siblings, 0 replies; 20+ messages in thread
From: Robert Dinse @ 2022-05-26 23:14 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Kees Cook


      It >DID< build ok with this patch, I applied it and built last night,
I'm presently running that kernel with a KVM/Qemu Windows guest on my
workstation:

Linux nanook 5.18.0 #1 SMP PREEMPT Wed May 25 18:14:43 PDT 2022 x86_64 x86_64 
x86_64 GNU/Linux

      The Windows guest starts and runs but networking isn't working (I am
using a bridge for the guest hosts and a static IP for both the main machine
and guest), it all works including networking with 5.17.9.

-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-
  Eskimo North Linux Friendly Internet Access, Shell Accounts, and Hosting.
    Knowledgeable human assistance, not telephone trees or script readers.
  See our web site: http://www.eskimo.com/ (206) 812-0051 or (800) 246-6874.

On Thu, 26 May 2022, Sean Christopherson wrote:

> Date: Thu, 26 May 2022 21:08:09 +0000
> From: Sean Christopherson <seanjc@google.com>
> To: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>,
>     Vitaly Kuznetsov <vkuznets@redhat.com>,
>     Wanpeng Li <wanpengli@tencent.com>, Jim Mattson <jmattson@google.com>,
>     Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
>     linux-kernel@vger.kernel.org, Kees Cook <keescook@chromium.org>,
>     Robert Dinse <nanook@eskimo.com>
> Subject: [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups
> 
> 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
> -- 
> 2.36.1.255.ge46751e96f-goog
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/8] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array
  2022-05-26 21:08 ` [PATCH v2 3/8] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array Sean Christopherson
@ 2022-05-31 18:04   ` Kees Cook
  0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2022-05-31 18:04 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Robert Dinse

On Thu, May 26, 2022 at 09:08:12PM +0000, Sean Christopherson wrote:
> Omit RIP from the emulator's _regs array, which is used only for GPRs,
> i.e. registers that can be referenced via ModRM and/or SIB bytes.  The
> emulator uses the dedicated _eip field for RIP, and manually reads from
> _eip to handle RIP-relative addressing.
> 
> To avoid an even bigger, slightly more dangerous change, hardcode the
> number of GPRs to 16 for the time being even though 32-bit KVM's emulator
> technically should only have 8 GPRs.  Add a TODO to address that in a
> future commit.
> 
> See also the comments above the read_gpr() and write_gpr() declarations,
> and obviously the handling in writeback_registers().
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 5/8] KVM: x86: Reduce the number of emulator GPRs to '8' for 32-bit KVM
  2022-05-26 21:08 ` [PATCH v2 5/8] KVM: x86: Reduce the number of emulator GPRs to '8' for 32-bit KVM Sean Christopherson
@ 2022-05-31 18:04   ` Kees Cook
  0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2022-05-31 18:04 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Robert Dinse

On Thu, May 26, 2022 at 09:08:14PM +0000, Sean Christopherson wrote:
> Reduce the number of GPRs emulated by 32-bit KVM from 16 to 8.  KVM does
> not support emulating 64-bit mode on 32-bit host kernels, and so should
> never generate accesses to R8-15.
> 
> Opportunistically use NR_EMULATOR_GPRS in rsm_load_state_{32,64}() now
> that it is precise and accurate for both flavors.
> 
> Wrap the definition with full #ifdef ugliness; sadly, IS_ENABLED()
> doesn't guarantee a compile-time constant as far as BUILD_BUG_ON() is
> concerned.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR
  2022-05-26 21:08 ` [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR Sean Christopherson
@ 2022-05-31 18:06   ` Kees Cook
  2022-06-01  8:29   ` Vitaly Kuznetsov
  1 sibling, 0 replies; 20+ messages in thread
From: Kees Cook @ 2022-05-31 18:06 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Robert Dinse

On Thu, May 26, 2022 at 09:08:15PM +0000, Sean Christopherson wrote:
> Bug the VM, i.e. kill it, if the emulator accesses a non-existent GPR,
> i.e. generates an out-of-bounds GPR index.  Continuing on all but
> gaurantees some form of data corruption in the guest, e.g. even if KVM
> were to redirect to a dummy register, KVM would be incorrectly read zeros
> and drop writes.
> 
> Note, bugging the VM doesn't completely prevent data corruption, e.g. the
> current round of emulation will complete before the vCPU bails out to
> userspace.  But, the very act of killing the guest can also cause data
> corruption, e.g. due to lack of file writeback before termination, so
> taking on additional complexity to cleanly bail out of the emulator isn't
> justified, the goal is purely to stem the bleeding and alert userspace
> that something has gone horribly wrong, i.e. to avoid _silent_ data
> corruption.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

I like this -- this ends up failing in a relatively clean fashion. (i.e.
it's not actually a BUG(), but rather tells the VM to stop.)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 7/8] KVM: x86: Bug the VM if the emulator generates a bogus exception vector
  2022-05-26 21:08 ` [PATCH v2 7/8] KVM: x86: Bug the VM if the emulator generates a bogus exception vector Sean Christopherson
@ 2022-05-31 18:06   ` Kees Cook
  2022-06-01  8:31   ` Vitaly Kuznetsov
  1 sibling, 0 replies; 20+ messages in thread
From: Kees Cook @ 2022-05-31 18:06 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Robert Dinse

On Thu, May 26, 2022 at 09:08:16PM +0000, Sean Christopherson wrote:
> Bug the VM if KVM's emulator attempts to inject a bogus exception vector.
> The guest is likely doomed even if KVM continues on, and propagating a
> bad vector to the rest of KVM runs the risk of breaking other assumptions
> in KVM and thus triggering a more egregious bug.
> 
> All existing users of emulate_exception() have hardcoded vector numbers
> (__load_segment_descriptor() uses a few different vectors, but they're
> all hardcoded), and future users are likely to follow suit, i.e. the
> change to emulate_exception() is a glorified nop.
> 
> As for the ctxt->exception.vector check in x86_emulate_insn(), the few
> known times the WARN has been triggered in the past is when the field was
> not set when synthesizing a fault, i.e. for all intents and purposes the
> check protects against consumption of uninitialized data.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 8/8] KVM: x86: Bug the VM on an out-of-bounds data read
  2022-05-26 21:08 ` [PATCH v2 8/8] KVM: x86: Bug the VM on an out-of-bounds data read Sean Christopherson
@ 2022-05-31 18:06   ` Kees Cook
  2022-06-01  8:39   ` Vitaly Kuznetsov
  1 sibling, 0 replies; 20+ messages in thread
From: Kees Cook @ 2022-05-31 18:06 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Robert Dinse

On Thu, May 26, 2022 at 09:08:17PM +0000, Sean Christopherson wrote:
> Bug the VM and terminate emulation if an out-of-bounds read into the
> emulator's data cache occurs.  Knowingly contuining on all but guarantees
> that KVM will overwrite random kernel data, which is far, far worse than
> killing the VM.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR
  2022-05-26 21:08 ` [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR Sean Christopherson
  2022-05-31 18:06   ` Kees Cook
@ 2022-06-01  8:29   ` Vitaly Kuznetsov
  2022-06-02 16:01     ` Sean Christopherson
  1 sibling, 1 reply; 20+ messages in thread
From: Vitaly Kuznetsov @ 2022-06-01  8:29 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel,
	Kees Cook, Robert Dinse, Paolo Bonzini

Sean Christopherson <seanjc@google.com> writes:

> Bug the VM, i.e. kill it, if the emulator accesses a non-existent GPR,
> i.e. generates an out-of-bounds GPR index.  Continuing on all but
> gaurantees some form of data corruption in the guest, e.g. even if KVM
> were to redirect to a dummy register, KVM would be incorrectly read zeros
> and drop writes.
>
> Note, bugging the VM doesn't completely prevent data corruption, e.g. the
> current round of emulation will complete before the vCPU bails out to
> userspace.  But, the very act of killing the guest can also cause data
> corruption, e.g. due to lack of file writeback before termination, so
> taking on additional complexity to cleanly bail out of the emulator isn't
> justified, the goal is purely to stem the bleeding and alert userspace
> that something has gone horribly wrong, i.e. to avoid _silent_ data
> corruption.

Thanks, I agree wholeheartedly :-)

>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/emulate.c     |  4 ++--
>  arch/x86/kvm/kvm_emulate.h | 10 ++++++++++
>  arch/x86/kvm/x86.c         |  9 +++++++++
>  3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 77161f57c8d3..70a8e0cd9fdc 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -247,7 +247,7 @@ enum x86_transfer_type {
>  
>  static ulong reg_read(struct x86_emulate_ctxt *ctxt, unsigned nr)
>  {
> -	if (WARN_ON_ONCE(nr >= NR_EMULATOR_GPRS))
> +	if (KVM_EMULATOR_BUG_ON(nr >= NR_EMULATOR_GPRS, ctxt))
>  		nr &= NR_EMULATOR_GPRS - 1;
>  
>  	if (!(ctxt->regs_valid & (1 << nr))) {
> @@ -259,7 +259,7 @@ 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 >= NR_EMULATOR_GPRS))
> +	if (KVM_EMULATOR_BUG_ON(nr >= NR_EMULATOR_GPRS, ctxt))
>  		nr &= NR_EMULATOR_GPRS - 1;
>  
>  	BUILD_BUG_ON(sizeof(ctxt->regs_dirty) * BITS_PER_BYTE < NR_EMULATOR_GPRS);
> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> index 034c845b3c63..89246446d6aa 100644
> --- a/arch/x86/kvm/kvm_emulate.h
> +++ b/arch/x86/kvm/kvm_emulate.h
> @@ -89,6 +89,7 @@ struct x86_instruction_info {
>  #define X86EMUL_INTERCEPTED     6 /* Intercepted by nested VMCB/VMCS */
>  
>  struct x86_emulate_ops {
> +	void (*vm_bugged)(struct x86_emulate_ctxt *ctxt);
>  	/*
>  	 * read_gpr: read a general purpose register (rax - r15)
>  	 *
> @@ -383,6 +384,15 @@ struct x86_emulate_ctxt {
>  	bool is_branch;
>  };
>  
> +#define KVM_EMULATOR_BUG_ON(cond, ctxt)		\
> +({						\
> +	int __ret = (cond);			\
> +						\
> +	if (WARN_ON_ONCE(__ret))		\
> +		ctxt->ops->vm_bugged(ctxt);	\
> +	unlikely(__ret);			\
> +})
> +
>  /* Repeat String Operation Prefix */
>  #define REPE_PREFIX	0xf3
>  #define REPNE_PREFIX	0xf2
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7460b9a77d9a..e60badfbbc42 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7887,7 +7887,16 @@ static int emulator_set_xcr(struct x86_emulate_ctxt *ctxt, u32 index, u64 xcr)
>  	return __kvm_set_xcr(emul_to_vcpu(ctxt), index, xcr);
>  }
>  
> +static void emulator_vm_bugged(struct x86_emulate_ctxt *ctxt)
> +{
> +	struct kvm *kvm = emul_to_vcpu(ctxt)->kvm;
> +
> +	if (!kvm->vm_bugged)
> +		kvm_vm_bugged(kvm);
> +}
> +
>  static const struct x86_emulate_ops emulate_ops = {
> +	.vm_bugged           = emulator_vm_bugged,
>  	.read_gpr            = emulator_read_gpr,
>  	.write_gpr           = emulator_write_gpr,
>  	.read_std            = emulator_read_std,

Is it actually "vm_bugged" or "kvm_bugged"? :-)

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 7/8] KVM: x86: Bug the VM if the emulator generates a bogus exception vector
  2022-05-26 21:08 ` [PATCH v2 7/8] KVM: x86: Bug the VM if the emulator generates a bogus exception vector Sean Christopherson
  2022-05-31 18:06   ` Kees Cook
@ 2022-06-01  8:31   ` Vitaly Kuznetsov
  1 sibling, 0 replies; 20+ messages in thread
From: Vitaly Kuznetsov @ 2022-06-01  8:31 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel,
	Kees Cook, Robert Dinse, Paolo Bonzini

Sean Christopherson <seanjc@google.com> writes:

> Bug the VM if KVM's emulator attempts to inject a bogus exception vector.
> The guest is likely doomed even if KVM continues on, and propagating a
> bad vector to the rest of KVM runs the risk of breaking other assumptions
> in KVM and thus triggering a more egregious bug.
>
> All existing users of emulate_exception() have hardcoded vector numbers
> (__load_segment_descriptor() uses a few different vectors, but they're
> all hardcoded), and future users are likely to follow suit, i.e. the
> change to emulate_exception() is a glorified nop.
>
> As for the ctxt->exception.vector check in x86_emulate_insn(), the few
> known times the WARN has been triggered in the past is when the field was
> not set when synthesizing a fault, i.e. for all intents and purposes the
> check protects against consumption of uninitialized data.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/emulate.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 70a8e0cd9fdc..2aa17462a9ac 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -624,7 +624,9 @@ static unsigned long seg_base(struct x86_emulate_ctxt *ctxt, int seg)
>  static int emulate_exception(struct x86_emulate_ctxt *ctxt, int vec,
>  			     u32 error, bool valid)
>  {
> -	WARN_ON(vec > 0x1f);
> +	if (KVM_EMULATOR_BUG_ON(vec > 0x1f, ctxt))
> +		return X86EMUL_UNHANDLEABLE;
> +
>  	ctxt->exception.vector = vec;
>  	ctxt->exception.error_code = error;
>  	ctxt->exception.error_code_valid = valid;
> @@ -5728,7 +5730,8 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
>  
>  done:
>  	if (rc == X86EMUL_PROPAGATE_FAULT) {
> -		WARN_ON(ctxt->exception.vector > 0x1f);
> +		if (KVM_EMULATOR_BUG_ON(ctxt->exception.vector > 0x1f, ctxt))
> +			return EMULATION_FAILED;
>  		ctxt->have_exception = true;
>  	}
>  	if (rc == X86EMUL_INTERCEPTED)

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 8/8] KVM: x86: Bug the VM on an out-of-bounds data read
  2022-05-26 21:08 ` [PATCH v2 8/8] KVM: x86: Bug the VM on an out-of-bounds data read Sean Christopherson
  2022-05-31 18:06   ` Kees Cook
@ 2022-06-01  8:39   ` Vitaly Kuznetsov
  1 sibling, 0 replies; 20+ messages in thread
From: Vitaly Kuznetsov @ 2022-06-01  8:39 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel,
	Kees Cook, Robert Dinse, Paolo Bonzini

Sean Christopherson <seanjc@google.com> writes:

> Bug the VM and terminate emulation if an out-of-bounds read into the
> emulator's data cache occurs.  Knowingly contuining on all but guarantees
> that KVM will overwrite random kernel data, which is far, far worse than
> killing the VM.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/emulate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 2aa17462a9ac..39ea9138224c 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1373,7 +1373,8 @@ static int read_emulated(struct x86_emulate_ctxt *ctxt,
>  	if (mc->pos < mc->end)
>  		goto read_cached;
>  
> -	WARN_ON((mc->end + size) >= sizeof(mc->data));
> +	if (KVM_EMULATOR_BUG_ON((mc->end + size) >= sizeof(mc->data), ctxt))
> +		return X86EMUL_UNHANDLEABLE;
>  
>  	rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
>  				      &ctxt->exception);

The last WARN_ON() is gone, cool)

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR
  2022-06-01  8:29   ` Vitaly Kuznetsov
@ 2022-06-02 16:01     ` Sean Christopherson
  0 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2022-06-02 16:01 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel,
	Kees Cook, Robert Dinse, Paolo Bonzini

On Wed, Jun 01, 2022, Vitaly Kuznetsov wrote:
> >  static const struct x86_emulate_ops emulate_ops = {
> > +	.vm_bugged           = emulator_vm_bugged,
> >  	.read_gpr            = emulator_read_gpr,
> >  	.write_gpr           = emulator_write_gpr,
> >  	.read_std            = emulator_read_std,
> 
> Is it actually "vm_bugged" or "kvm_bugged"? :-)

vm_bugged.  KVM_BUG_ON() because it's a KVM bug on the condition, but the invididual
VM is what's bugged/dead, i.e. other VMs and thus KVM itself get to live on. :-)

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups
  2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
                   ` (8 preceding siblings ...)
  2022-05-26 23:14 ` [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Robert Dinse
@ 2022-06-08 12:23 ` Paolo Bonzini
  9 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2022-06-08 12:23 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel, Kees Cook, Robert Dinse

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2022-06-08 12:23 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-26 21:08 [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Sean Christopherson
2022-05-26 21:08 ` [PATCH v2 1/8] KVM: x86: Grab regs_dirty in local 'unsigned long' Sean Christopherson
2022-05-26 21:08 ` [PATCH v2 2/8] KVM: x86: Harden _regs accesses to guard against buggy input Sean Christopherson
2022-05-26 21:08 ` [PATCH v2 3/8] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array Sean Christopherson
2022-05-31 18:04   ` Kees Cook
2022-05-26 21:08 ` [PATCH v2 4/8] KVM: x86: Use 16-bit fields to track dirty/valid emulator GPRs Sean Christopherson
2022-05-26 21:08 ` [PATCH v2 5/8] KVM: x86: Reduce the number of emulator GPRs to '8' for 32-bit KVM Sean Christopherson
2022-05-31 18:04   ` Kees Cook
2022-05-26 21:08 ` [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR Sean Christopherson
2022-05-31 18:06   ` Kees Cook
2022-06-01  8:29   ` Vitaly Kuznetsov
2022-06-02 16:01     ` Sean Christopherson
2022-05-26 21:08 ` [PATCH v2 7/8] KVM: x86: Bug the VM if the emulator generates a bogus exception vector Sean Christopherson
2022-05-31 18:06   ` Kees Cook
2022-06-01  8:31   ` Vitaly Kuznetsov
2022-05-26 21:08 ` [PATCH v2 8/8] KVM: x86: Bug the VM on an out-of-bounds data read Sean Christopherson
2022-05-31 18:06   ` Kees Cook
2022-06-01  8:39   ` Vitaly Kuznetsov
2022-05-26 23:14 ` [PATCH v2 0/8] KVM: x86: Emulator _regs fixes and cleanups Robert Dinse
2022-06-08 12:23 ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.