[PATCH 0/4] KVM: x86: Emulator _regs fixes and cleanups

[PATCH 0/4] KVM: x86: Emulator _regs fixes and cleanups

[Sean Christopherson]

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.  I'm
99% certain GCC-12 is wrong and is generating a false positive, but just in
case...

I didn't tag patch 2 with Fixes or Cc: stable@; if it turns out to "fix"
the GCC-12 compilation error, it's probably worth sending to v5.18 stable
tree (KVM hasn't changed, but the warning=>error was "introdued in v5.18
by commit e6148767825c ("Makefile: Enable -Warray-bounds")).

Sean Christopherson (4):
  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

 arch/x86/kvm/emulate.c     | 14 ++++++++++++--
 arch/x86/kvm/kvm_emulate.h | 14 +++++++++++---
 2 files changed, 23 insertions(+), 5 deletions(-)


base-commit: 90bde5bea810d766e7046bf5884f2ccf76dd78e9
-- 
2.36.1.124.g0e6072fb45-goog

[PATCH 1/4] KVM: x86: Grab regs_dirty in local 'unsigned long'

[Sean Christopherson]

Capture ctxt->regs_dirty in a local 'unsigned long' instead of casting
ctxt->regs_dirty 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. KVM
is reading memory beyond ctxt->regs_dirty.  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.

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.124.g0e6072fb45-goog

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

[Sean Christopherson]

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;
+
 	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.124.g0e6072fb45-goog

[PATCH 3/4] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array

[Sean Christopherson]

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.

Replace all open coded instances of '16' with the new NR_EMULATOR_GPRS.

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     | 12 ++++++------
 arch/x86/kvm/kvm_emulate.h | 10 +++++++++-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c58366ae4da2..dd1bf116a9ed 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]);
 }
 
@@ -2495,7 +2495,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 8dff25d267b7..bdd4e9865ca9 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -301,6 +301,14 @@ 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.
+ */
+#define NR_EMULATOR_GPRS	(VCPU_REGS_R15 + 1)
+
 struct x86_emulate_ctxt {
 	void *vcpu;
 	const struct x86_emulate_ops *ops;
@@ -363,7 +371,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.124.g0e6072fb45-goog

[PATCH 4/4] KVM: x86: Use 16-bit fields to track dirty/valid emulator GPRs

[Sean Christopherson]

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, having 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).

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 dd1bf116a9ed..afb115b6a5a4 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 bdd4e9865ca9..fbe87ba78163 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -353,9 +353,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.124.g0e6072fb45-goog

Re: [PATCH 0/4] KVM: x86: Emulator _regs fixes and cleanups

[Robert Dinse]

      This set of patches did allow 5.18 to compile without errors using gcc 
12.1.  Thank you!

-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-
  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 Wed, 25 May 2022, Sean Christopherson wrote:

> Date: Wed, 25 May 2022 22:26:00 +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, Robert Dinse <nanook@eskimo.com>,
>     Kees Cook <keescook@chromium.org>
> Subject: [PATCH 0/4] 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.  I'm
> 99% certain GCC-12 is wrong and is generating a false positive, but just in
> case...
>
> I didn't tag patch 2 with Fixes or Cc: stable@; if it turns out to "fix"
> the GCC-12 compilation error, it's probably worth sending to v5.18 stable
> tree (KVM hasn't changed, but the warning=>error was "introdued in v5.18
> by commit e6148767825c ("Makefile: Enable -Warray-bounds")).
>
> Sean Christopherson (4):
>  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
>
> arch/x86/kvm/emulate.c     | 14 ++++++++++++--
> arch/x86/kvm/kvm_emulate.h | 14 +++++++++++---
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
>
> base-commit: 90bde5bea810d766e7046bf5884f2ccf76dd78e9
> -- 
> 2.36.1.124.g0e6072fb45-goog
>

Re: [PATCH 3/4] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array

[kernel test robot]

Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on 90bde5bea810d766e7046bf5884f2ccf76dd78e9]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Christopherson/KVM-x86-Emulator-_regs-fixes-and-cleanups/20220526-062734
base:   90bde5bea810d766e7046bf5884f2ccf76dd78e9
config: i386-debian-10.3-kselftests (https://download.01.org/0day-ci/archive/20220526/202205261040.m1luL6IW-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/a2e1bffa1b7c8179bed0c28ade24b1a73d7220de
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sean-Christopherson/KVM-x86-Emulator-_regs-fixes-and-cleanups/20220526-062734
        git checkout a2e1bffa1b7c8179bed0c28ade24b1a73d7220de
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash arch/x86/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/x86/kvm/x86.h:9,
                    from arch/x86/kvm/cpuid.h:5,
                    from arch/x86/kvm/mmu.h:7,
                    from arch/x86/kvm/x86.c:22:
>> arch/x86/kvm/kvm_emulate.h:310:34: error: 'VCPU_REGS_R15' undeclared here (not in a function); did you mean 'VCPU_REGS_RIP'?
     310 | #define NR_EMULATOR_GPRS        (VCPU_REGS_R15 + 1)
         |                                  ^~~~~~~~~~~~~
   arch/x86/kvm/kvm_emulate.h:374:29: note: in expansion of macro 'NR_EMULATOR_GPRS'
     374 |         unsigned long _regs[NR_EMULATOR_GPRS];
         |                             ^~~~~~~~~~~~~~~~
--
   In file included from arch/x86/kvm/emulate.c:23:
>> arch/x86/kvm/kvm_emulate.h:310:34: error: 'VCPU_REGS_R15' undeclared here (not in a function); did you mean 'VCPU_REGS_RIP'?
     310 | #define NR_EMULATOR_GPRS        (VCPU_REGS_R15 + 1)
         |                                  ^~~~~~~~~~~~~
   arch/x86/kvm/kvm_emulate.h:374:29: note: in expansion of macro 'NR_EMULATOR_GPRS'
     374 |         unsigned long _regs[NR_EMULATOR_GPRS];
         |                             ^~~~~~~~~~~~~~~~
   arch/x86/kvm/emulate.c: In function 'reg_write':
   arch/x86/kvm/emulate.c:268:1: error: control reaches end of non-void function [-Werror=return-type]
     268 | }
         | ^
   arch/x86/kvm/emulate.c: In function 'reg_read':
   arch/x86/kvm/emulate.c:258:1: error: control reaches end of non-void function [-Werror=return-type]
     258 | }
         | ^
   cc1: some warnings being treated as errors
--
   In file included from arch/x86/kvm/vmx/../x86.h:9,
                    from arch/x86/kvm/vmx/../cpuid.h:5,
                    from arch/x86/kvm/vmx/evmcs.c:7:
>> arch/x86/kvm/vmx/../kvm_emulate.h:310:34: error: 'VCPU_REGS_R15' undeclared here (not in a function); did you mean 'VCPU_REGS_RIP'?
     310 | #define NR_EMULATOR_GPRS        (VCPU_REGS_R15 + 1)
         |                                  ^~~~~~~~~~~~~
   arch/x86/kvm/vmx/../kvm_emulate.h:374:29: note: in expansion of macro 'NR_EMULATOR_GPRS'
     374 |         unsigned long _regs[NR_EMULATOR_GPRS];
         |                             ^~~~~~~~~~~~~~~~


vim +310 arch/x86/kvm/kvm_emulate.h

   303	
   304	/*
   305	 * The emulator's _regs array tracks only the GPRs, i.e. excludes RIP.  RIP is
   306	 * tracked/accessed via _eip, and except for RIP relative addressing, which
   307	 * also uses _eip, RIP cannot be a register operand nor can it be an operand in
   308	 * a ModRM or SIB byte.
   309	 */
 > 310	#define NR_EMULATOR_GPRS	(VCPU_REGS_R15 + 1)
   311	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 1/4] KVM: x86: Grab regs_dirty in local 'unsigned long'

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> Capture ctxt->regs_dirty in a local 'unsigned long' instead of casting
> ctxt->regs_dirty 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. KVM
> is reading memory beyond ctxt->regs_dirty.  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.

Your nearly perfect description misses one important part: in 'struct
x86_emulate_ctxt', 'regs_dirty' is actually 'u32' thus all this buzz :-)

>
> 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]);
>  }

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

-- 
Vitaly

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

[Vitaly Kuznetsov]

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];

-- 
Vitaly

Re: [PATCH 1/4] KVM: x86: Grab regs_dirty in local 'unsigned long'

[Kees Cook]

On Wed, May 25, 2022 at 10:26:01PM +0000, Sean Christopherson wrote:
> Capture ctxt->regs_dirty in a local 'unsigned long' instead of casting
> ctxt->regs_dirty 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. KVM
> is reading memory beyond ctxt->regs_dirty.  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.

Right; there have bean a handful of other bitops-using-stuff-cast-to-ulong
that has been recently fixed elsewhere. Better to get them all squashed.
:)

> Signed-off-by: Sean Christopherson <seanjc@google.com>

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

-- 
Kees Cook

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

[Kees Cook]

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

Re: [PATCH 4/4] KVM: x86: Use 16-bit fields to track dirty/valid emulator GPRs

[Kees Cook]

On Wed, May 25, 2022 at 10:26:04PM +0000, Sean Christopherson wrote:
> 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, having 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).
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

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

-- 
Kees Cook

Re: [PATCH 3/4] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array

[Sean Christopherson]

On Thu, May 26, 2022, kernel test robot wrote:
> Hi Sean,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on 90bde5bea810d766e7046bf5884f2ccf76dd78e9]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Christopherson/KVM-x86-Emulator-_regs-fixes-and-cleanups/20220526-062734
> base:   90bde5bea810d766e7046bf5884f2ccf76dd78e9
> config: i386-debian-10.3-kselftests (https://download.01.org/0day-ci/archive/20220526/202205261040.m1luL6IW-lkp@intel.com/config)

Doh, forgot to build and test 32-bit KVM.  For this patch, I think it makes sense
to just hardcode the number of registers to 16.  But in a follow-up, that can be
reduced to 8 for 32-bit KVM, and then rsm_load_state_32() can use NR_EMULATOR_GPRS
too.

Re: [PATCH 3/4] KVM: x86: Omit VCPU_REGS_RIP from emulator's _regs array

[Sean Christopherson]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On Thu, May 26, 2022, kernel test robot wrote:
> Hi Sean,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on 90bde5bea810d766e7046bf5884f2ccf76dd78e9]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Christopherson/KVM-x86-Emulator-_regs-fixes-and-cleanups/20220526-062734
> base:   90bde5bea810d766e7046bf5884f2ccf76dd78e9
> config: i386-debian-10.3-kselftests (https://download.01.org/0day-ci/archive/20220526/202205261040.m1luL6IW-lkp(a)intel.com/config)

Doh, forgot to build and test 32-bit KVM.  For this patch, I think it makes sense
to just hardcode the number of registers to 16.  But in a follow-up, that can be
reduced to 8 for 32-bit KVM, and then rsm_load_state_32() can use NR_EMULATOR_GPRS
too.

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

[Sean Christopherson]

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'.

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

[Vitaly Kuznetsov]

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).

-- 
Vitaly

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

[Sean Christopherson]

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.