[PATCH 0/4] KVM: VMX: Fix handling of invalid L2 guest state

[PATCH 0/4] KVM: VMX: Fix handling of invalid L2 guest state

[Sean Christopherson]

Fixes and a test for invalid L2 guest state.  TL;DR: KVM should never
emulate L2 if L2's guest state is invalid.

Patch 01 fixes a regression found by syzbot. Patch 02 closes what I suspect
was the hole the buggy patch was trying to close.  Patch 03 is a related
docs update.  Patch 04 is a selftest for the regression and for a subset
of patch 02's behavior.

Sean Christopherson (4):
  KVM: VMX: Always clear vmx->fail on emulation_required
  KVM: nVMX: Synthesize TRIPLE_FAULT for L2 if emulation is required
  KVM: VMX: Fix stale docs for kvm-intel.emulate_invalid_guest_state
  KVM: selftests: Add test to verify TRIPLE_FAULT on invalid L2 guest
    state

 .../admin-guide/kernel-parameters.txt         |   8 +-
 arch/x86/kvm/vmx/vmx.c                        |  36 ++++--
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../x86_64/vmx_invalid_nested_guest_state.c   | 105 ++++++++++++++++++
 5 files changed, 138 insertions(+), 13 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c

-- 
2.34.1.400.ga245620fadb-goog

[PATCH 1/4] KVM: VMX: Always clear vmx->fail on emulation_required

[Sean Christopherson]

Revert a relatively recent change that set vmx->fail if the vCPU is in L2
and emulation_required is true, as that behavior is completely bogus.
Setting vmx->fail and synthesizing a VM-Exit is contradictory and wrong:

  (a) it's impossible to have both a VM-Fail and VM-Exit
  (b) vmcs.EXIT_REASON is not modified on VM-Fail
  (c) emulation_required refers to guest state and guest state checks are
      always VM-Exits, not VM-Fails.

For KVM specifically, emulation_required is handled before nested exits
in __vmx_handle_exit(), thus setting vmx->fail has no immediate effect,
i.e. KVM calls into handle_invalid_guest_state() and vmx->fail is ignored.
Setting vmx->fail can ultimately result in a WARN in nested_vmx_vmexit()
firing when tearing down the VM as KVM never expects vmx->fail to be set
when L2 is active, KVM always reflects those errors into L1.

  ------------[ cut here ]------------
  WARNING: CPU: 0 PID: 21158 at arch/x86/kvm/vmx/nested.c:4548
                                nested_vmx_vmexit+0x16bd/0x17e0
                                arch/x86/kvm/vmx/nested.c:4547
  Modules linked in:
  CPU: 0 PID: 21158 Comm: syz-executor.1 Not tainted 5.16.0-rc3-syzkaller #0
  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
  RIP: 0010:nested_vmx_vmexit+0x16bd/0x17e0 arch/x86/kvm/vmx/nested.c:4547
  Code: <0f> 0b e9 2e f8 ff ff e8 57 b3 5d 00 0f 0b e9 00 f1 ff ff 89 e9 80
  Call Trace:
   vmx_leave_nested arch/x86/kvm/vmx/nested.c:6220 [inline]
   nested_vmx_free_vcpu+0x83/0xc0 arch/x86/kvm/vmx/nested.c:330
   vmx_free_vcpu+0x11f/0x2a0 arch/x86/kvm/vmx/vmx.c:6799
   kvm_arch_vcpu_destroy+0x6b/0x240 arch/x86/kvm/x86.c:10989
   kvm_vcpu_destroy+0x29/0x90 arch/x86/kvm/../../../virt/kvm/kvm_main.c:441
   kvm_free_vcpus arch/x86/kvm/x86.c:11426 [inline]
   kvm_arch_destroy_vm+0x3ef/0x6b0 arch/x86/kvm/x86.c:11545
   kvm_destroy_vm arch/x86/kvm/../../../virt/kvm/kvm_main.c:1189 [inline]
   kvm_put_kvm+0x751/0xe40 arch/x86/kvm/../../../virt/kvm/kvm_main.c:1220
   kvm_vcpu_release+0x53/0x60 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3489
   __fput+0x3fc/0x870 fs/file_table.c:280
   task_work_run+0x146/0x1c0 kernel/task_work.c:164
   exit_task_work include/linux/task_work.h:32 [inline]
   do_exit+0x705/0x24f0 kernel/exit.c:832
   do_group_exit+0x168/0x2d0 kernel/exit.c:929
   get_signal+0x1740/0x2120 kernel/signal.c:2852
   arch_do_signal_or_restart+0x9c/0x730 arch/x86/kernel/signal.c:868
   handle_signal_work kernel/entry/common.c:148 [inline]
   exit_to_user_mode_loop kernel/entry/common.c:172 [inline]
   exit_to_user_mode_prepare+0x191/0x220 kernel/entry/common.c:207
   __syscall_exit_to_user_mode_work kernel/entry/common.c:289 [inline]
   syscall_exit_to_user_mode+0x2e/0x70 kernel/entry/common.c:300
   do_syscall_64+0x53/0xd0 arch/x86/entry/common.c:86
   entry_SYSCALL_64_after_hwframe+0x44/0xae

Fixes: c8607e4a086f ("KVM: x86: nVMX: don't fail nested VM entry on invalid guest state if !from_vmentry")
Reported-by: syzbot+f1d2136db9c80d4733e8@syzkaller.appspotmail.com
Cc: Maxim Levitsky <mlevitsk@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index efcc5a58abbc..9e415e5a91ab 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6631,9 +6631,7 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	 * consistency check VM-Exit due to invalid guest state and bail.
 	 */
 	if (unlikely(vmx->emulation_required)) {
-
-		/* We don't emulate invalid state of a nested guest */
-		vmx->fail = is_guest_mode(vcpu);
+		vmx->fail = 0;
 
 		vmx->exit_reason.full = EXIT_REASON_INVALID_STATE;
 		vmx->exit_reason.failed_vmentry = 1;
-- 
2.34.1.400.ga245620fadb-goog

[PATCH 2/4] KVM: nVMX: Synthesize TRIPLE_FAULT for L2 if emulation is required

[Sean Christopherson]

Synthesize a triple fault if L2 guest state is invalid at the time of
VM-Enter, which can happen if L1 modifies SMRAM or if userspace stuffs
guest state via ioctls(), e.g. KVM_SET_SREGS.  KVM should never emulate
invalid guest state, since from L1's perspective, it's architecturally
impossible for L2 to have invalid state while L2 is running in hardware.
E.g. attempts to set CR0 or CR4 to unsupported values will either VM-Exit
or #GP.

Modifying vCPU state via RSM+SMRAM and ioctl() are the only paths that
can trigger this scenario, as nested VM-Enter correctly rejects any
attempt to enter L2 with invalid state.

RSM is a straightforward case as (a) KVM follows AMD's SMRAM layout and
behavior, and (b) Intel's SDM states that loading reserved CR0/CR4 bits
via RSM results in shutdown, i.e. there is precedent for KVM's behavior.
Following AMD's SMRAM layout is important as AMD's layout saves/restores
the descriptor cache information, including CS.RPL and SS.RPL, and also
defines all the fields relevant to invalid guest state as read-only, i.e.
so long as the vCPU had valid state before the SMI, which is guaranteed
for L2, RSM will generate valid state unless SMRAM was modified.  Intel's
layout saves/restores only the selector, which means that scenarios where
the selector and cached RPL don't match, e.g. conforming code segments,
would yield invalid guest state.  Intel CPUs fudge around this issued by
stuffing SS.RPL and CS.RPL on RSM.  Per Intel's SDM on the "Default
Treatment of RSM", paraphrasing for brevity:

  IF internal storage indicates that the [CPU was post-VMXON]
  THEN
     enter VMX operation (root or non-root);
     restore VMX-critical state as defined in Section 34.14.1;
     set to their fixed values any bits in CR0 and CR4 whose values must
     be fixed in VMX operation [unless coming from an unrestricted guest];
     IF RFLAGS.VM = 0 AND (in VMX root operation OR the
        “unrestricted guest” VM-execution control is 0)
     THEN
       CS.RPL := SS.DPL;
       SS.RPL := SS.DPL;
     FI;
     restore current VMCS pointer;
  FI;

Note that Intel CPUs also overwrite the fixed CR0/CR4 bits, whereas KVM
will sythesize TRIPLE_FAULT in this scenario.  KVM's behavior is allowed
as both Intel and AMD define CR0/CR4 SMRAM fields as read-only, i.e. the
only way for CR0 and/or CR4 to have illegal values is if they were
modified by the L1 SMM handler, and Intel's SDM "SMRAM State Save Map"
section states "modifying these registers will result in unpredictable
behavior".

KVM's ioctl() behavior is less straightforward.  Because KVM allows
ioctls() to be executed in any order, rejecting an ioctl() if it would
result in invalid L2 guest state is not an option as KVM cannot know if
a future ioctl() would resolve the invalid state, e.g. KVM_SET_SREGS, or
drop the vCPU out of L2, e.g. KVM_SET_NESTED_STATE.  Ideally, KVM would
reject KVM_RUN if L2 contained invalid guest state, but that carries the
risk of a false positive, e.g. if RSM loaded invalid guest state and KVM
exited to userspace.  Setting a flag/request to detect such a scenario is
undesirable because (a) it's extremely unlikely to add value to KVM as a
whole, and (b) KVM would need to consider ioctl() interactions with such
a flag, e.g. if userspace migrated the vCPU while the flag were set.

Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9e415e5a91ab..5307fcee3b3b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5900,18 +5900,14 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 		vmx_flush_pml_buffer(vcpu);
 
 	/*
-	 * We should never reach this point with a pending nested VM-Enter, and
-	 * more specifically emulation of L2 due to invalid guest state (see
-	 * below) should never happen as that means we incorrectly allowed a
-	 * nested VM-Enter with an invalid vmcs12.
+	 * KVM should never reach this point with a pending nested VM-Enter.
+	 * More specifically, short-circuiting VM-Entry to emulate L2 due to
+	 * invalid guest state should never happen as that means KVM knowingly
+	 * allowed a nested VM-Enter with an invalid vmcs12.  More below.
 	 */
 	if (KVM_BUG_ON(vmx->nested.nested_run_pending, vcpu->kvm))
 		return -EIO;
 
-	/* If guest state is invalid, start emulating */
-	if (vmx->emulation_required)
-		return handle_invalid_guest_state(vcpu);
-
 	if (is_guest_mode(vcpu)) {
 		/*
 		 * PML is never enabled when running L2, bail immediately if a
@@ -5933,10 +5929,30 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 		 */
 		nested_mark_vmcs12_pages_dirty(vcpu);
 
+		/*
+		 * Synthesize a triple fault if L2 state is invalid.  In normal
+		 * operation, nested VM-Enter rejects any attempt to enter L2
+		 * with invalid state.  However, those checks are skipped if
+		 * state is being stuffed via RSM or KVM_SET_NESTED_STATE.  If
+		 * L2 state is invalid, it means either L1 modified SMRAM state
+		 * or userspace provided bad state.  Synthesize TRIPLE_FAULT as
+		 * doing so is architecturally allowed in the RSM case, and is
+		 * the least awful solution for the userspace case without
+		 * risking false positives.
+		 */
+		if (vmx->emulation_required) {
+			nested_vmx_vmexit(vcpu, EXIT_REASON_TRIPLE_FAULT, 0, 0);
+			return 1;
+		}
+
 		if (nested_vmx_reflect_vmexit(vcpu))
 			return 1;
 	}
 
+	/* If guest state is invalid, start emulating.  L2 is handled above. */
+	if (vmx->emulation_required)
+		return handle_invalid_guest_state(vcpu);
+
 	if (exit_reason.failed_vmentry) {
 		dump_vmcs(vcpu);
 		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
-- 
2.34.1.400.ga245620fadb-goog

[PATCH 3/4] KVM: VMX: Fix stale docs for kvm-intel.emulate_invalid_guest_state

[Sean Christopherson]

Update the documentation for kvm-intel's emulate_invalid_guest_state to
rectify the description of KVM's default behavior, and to document that
the behavior and thus parameter only applies to L1.

Fixes: a27685c33acc ("KVM: VMX: Emulate invalid guest state by default")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 9725c546a0d4..fc34332c8d9a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2413,8 +2413,12 @@
 			Default is 1 (enabled)
 
 	kvm-intel.emulate_invalid_guest_state=
-			[KVM,Intel] Enable emulation of invalid guest states
-			Default is 0 (disabled)
+			[KVM,Intel] Disable emulation of invalid guest state.
+			Ignored if kvm-intel.enable_unrestricted_guest=1, as
+			guest state is never invalid for unrestricted guests.
+			This param doesn't apply to nested guests (L2), as KVM
+			never emulates invalid L2 guest state.
+			Default is 1 (enabled)
 
 	kvm-intel.flexpriority=
 			[KVM,Intel] Disable FlexPriority feature (TPR shadow).
-- 
2.34.1.400.ga245620fadb-goog

[PATCH 4/4] KVM: selftests: Add test to verify TRIPLE_FAULT on invalid L2 guest state

[Sean Christopherson]

Add a selftest to attempt to enter L2 with invalid guests state by
exiting to userspace via I/O from L2, and then using KVM_SET_SREGS to set
invalid guest state (marking TR unusable is arbitrary chosen for its
relative simplicity).

This is a regression test for a bug introduced by commit c8607e4a086f
("KVM: x86: nVMX: don't fail nested VM entry on invalid guest state if
!from_vmentry"), which incorrectly set vmx->fail=true when L2 had invalid
guest state and ultimately triggered a WARN due to nested_vmx_vmexit()
seeing vmx->fail==true while attempting to synthesize a nested VM-Exit.

The is also a functional test to verify that KVM sythesizes TRIPLE_FAULT
for L2, which is somewhat arbitrary behavior, instead of emulating L2.
KVM should never emulate L2 due to invalid guest state, as it's
architecturally impossible for L1 to run an L2 guest with invalid state
as nested VM-Enter should always fail, i.e. L1 needs to do the emulation.
Stuffing state via KVM ioctl() is a non-architctural, out-of-band case,
hence the TRIPLE_FAULT being rather arbitrary.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../x86_64/vmx_invalid_nested_guest_state.c   | 105 ++++++++++++++++++
 3 files changed, 107 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 3763105029fb..de41afb01a77 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -34,6 +34,7 @@
 /x86_64/vmx_apic_access_test
 /x86_64/vmx_close_while_nested_test
 /x86_64/vmx_dirty_log_test
+/x86_64/vmx_invalid_nested_guest_state
 /x86_64/vmx_preemption_timer_test
 /x86_64/vmx_set_nested_state_test
 /x86_64/vmx_tsc_adjust_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index c4e34717826a..6be4144c8d25 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -63,6 +63,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/userspace_msr_exit_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_apic_access_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_close_while_nested_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_dirty_log_test
+TEST_GEN_PROGS_x86_64 += x86_64/vmx_invalid_nested_guest_state
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_set_nested_state_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_nested_tsc_scaling_test
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c b/tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c
new file mode 100644
index 000000000000..489fbed4ca6f
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+#include "vmx.h"
+
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "kselftest.h"
+
+#define VCPU_ID	0
+#define ARBITRARY_IO_PORT 0x2000
+
+static struct kvm_vm *vm;
+
+static void l2_guest_code(void)
+{
+	/*
+	 * Generate an exit to L0 userspace, i.e. main(), via I/O to an
+	 * arbitrary port.
+	 */
+	asm volatile("inb %%dx, %%al"
+		     : : [port] "d" (ARBITRARY_IO_PORT) : "rax");
+}
+
+static void l1_guest_code(struct vmx_pages *vmx_pages)
+{
+#define L2_GUEST_STACK_SIZE 64
+	unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
+
+	GUEST_ASSERT(prepare_for_vmx_operation(vmx_pages));
+	GUEST_ASSERT(load_vmcs(vmx_pages));
+
+	/* Prepare the VMCS for L2 execution. */
+	prepare_vmcs(vmx_pages, l2_guest_code,
+		     &l2_guest_stack[L2_GUEST_STACK_SIZE]);
+
+	/*
+	 * L2 must be run without unrestricted guest, verify that the selftests
+	 * library hasn't enabled it.  Because KVM selftests jump directly to
+	 * 64-bit mode, unrestricted guest support isn't required.
+	 */
+	GUEST_ASSERT(!(vmreadz(CPU_BASED_VM_EXEC_CONTROL) & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) ||
+		     !(vmreadz(SECONDARY_VM_EXEC_CONTROL) & SECONDARY_EXEC_UNRESTRICTED_GUEST));
+
+	GUEST_ASSERT(!vmlaunch());
+
+	/* L2 should triple fault after main() stuffs invalid guest state. */
+	GUEST_ASSERT(vmreadz(VM_EXIT_REASON) == EXIT_REASON_TRIPLE_FAULT);
+	GUEST_DONE();
+}
+
+int main(int argc, char *argv[])
+{
+	vm_vaddr_t vmx_pages_gva;
+	struct kvm_sregs sregs;
+	struct kvm_run *run;
+	struct ucall uc;
+
+	nested_vmx_check_supported();
+
+	vm = vm_create_default(VCPU_ID, 0, (void *) l1_guest_code);
+
+	/* Allocate VMX pages and shared descriptors (vmx_pages). */
+	vcpu_alloc_vmx(vm, &vmx_pages_gva);
+	vcpu_args_set(vm, VCPU_ID, 1, vmx_pages_gva);
+
+	vcpu_run(vm, VCPU_ID);
+
+	run = vcpu_state(vm, VCPU_ID);
+
+	/*
+	 * The first exit to L0 userspace should be an I/O access from L2.
+	 * Running L1 should launch L2 without triggering an exit to userspace.
+	 */
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+		    "Expected KVM_EXIT_IO, got: %u (%s)\n",
+		    run->exit_reason, exit_reason_str(run->exit_reason));
+
+	TEST_ASSERT(run->io.port == ARBITRARY_IO_PORT,
+		    "Expected IN from port %d from L2, got port %d",
+		    ARBITRARY_IO_PORT, run->io.port);
+
+	/*
+	 * Stuff invalid guest state for L2 by making TR unusuable.  The next
+	 * KVM_RUN should induce a TRIPLE_FAULT in L2 as KVM doesn't support
+	 * emulating invalid guest state for L2.
+	 */
+	memset(&sregs, 0, sizeof(sregs));
+	vcpu_sregs_get(vm, VCPU_ID, &sregs);
+	sregs.tr.unusable = 1;
+	vcpu_sregs_set(vm, VCPU_ID, &sregs);
+
+	vcpu_run(vm, VCPU_ID);
+
+	switch (get_ucall(vm, VCPU_ID, &uc)) {
+	case UCALL_DONE:
+		break;
+	case UCALL_ABORT:
+		TEST_FAIL("%s", (const char *)uc.args[0]);
+	default:
+		TEST_FAIL("Unexpected ucall: %lu", uc.cmd);
+	}
+}
-- 
2.34.1.400.ga245620fadb-goog

Re: [PATCH 0/4] KVM: VMX: Fix handling of invalid L2 guest state

[Paolo Bonzini]

On 12/7/21 20:30, Sean Christopherson wrote:
> Fixes and a test for invalid L2 guest state.  TL;DR: KVM should never
> emulate L2 if L2's guest state is invalid.
> 
> Patch 01 fixes a regression found by syzbot. Patch 02 closes what I suspect
> was the hole the buggy patch was trying to close.  Patch 03 is a related
> docs update.  Patch 04 is a selftest for the regression and for a subset
> of patch 02's behavior.
> 
> Sean Christopherson (4):
>    KVM: VMX: Always clear vmx->fail on emulation_required
>    KVM: nVMX: Synthesize TRIPLE_FAULT for L2 if emulation is required
>    KVM: VMX: Fix stale docs for kvm-intel.emulate_invalid_guest_state
>    KVM: selftests: Add test to verify TRIPLE_FAULT on invalid L2 guest
>      state
> 
>   .../admin-guide/kernel-parameters.txt         |   8 +-
>   arch/x86/kvm/vmx/vmx.c                        |  36 ++++--
>   tools/testing/selftests/kvm/.gitignore        |   1 +
>   tools/testing/selftests/kvm/Makefile          |   1 +
>   .../x86_64/vmx_invalid_nested_guest_state.c   | 105 ++++++++++++++++++
>   5 files changed, 138 insertions(+), 13 deletions(-)
>   create mode 100644 tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c
> 

Queued, thanks.

Paolo

Re: [PATCH 1/4] KVM: VMX: Always clear vmx->fail on emulation_required

[Maxim Levitsky]

On Tue, 2021-12-07 at 19:30 +0000, Sean Christopherson wrote:
> Revert a relatively recent change that set vmx->fail if the vCPU is in L2
> and emulation_required is true, as that behavior is completely bogus.
> Setting vmx->fail and synthesizing a VM-Exit is contradictory and wrong:
> 
>   (a) it's impossible to have both a VM-Fail and VM-Exit
>   (b) vmcs.EXIT_REASON is not modified on VM-Fail
>   (c) emulation_required refers to guest state and guest state checks are
>       always VM-Exits, not VM-Fails.
> 
> For KVM specifically, emulation_required is handled before nested exits
> in __vmx_handle_exit(), thus setting vmx->fail has no immediate effect,
> i.e. KVM calls into handle_invalid_guest_state() and vmx->fail is ignored.
> Setting vmx->fail can ultimately result in a WARN in nested_vmx_vmexit()
> firing when tearing down the VM as KVM never expects vmx->fail to be set
> when L2 is active, KVM always reflects those errors into L1.
> 
>   ------------[ cut here ]------------
>   WARNING: CPU: 0 PID: 21158 at arch/x86/kvm/vmx/nested.c:4548
>                                 nested_vmx_vmexit+0x16bd/0x17e0
>                                 arch/x86/kvm/vmx/nested.c:4547
>   Modules linked in:
>   CPU: 0 PID: 21158 Comm: syz-executor.1 Not tainted 5.16.0-rc3-syzkaller #0
>   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>   RIP: 0010:nested_vmx_vmexit+0x16bd/0x17e0 arch/x86/kvm/vmx/nested.c:4547
>   Code: <0f> 0b e9 2e f8 ff ff e8 57 b3 5d 00 0f 0b e9 00 f1 ff ff 89 e9 80
>   Call Trace:
>    vmx_leave_nested arch/x86/kvm/vmx/nested.c:6220 [inline]
>    nested_vmx_free_vcpu+0x83/0xc0 arch/x86/kvm/vmx/nested.c:330
>    vmx_free_vcpu+0x11f/0x2a0 arch/x86/kvm/vmx/vmx.c:6799
>    kvm_arch_vcpu_destroy+0x6b/0x240 arch/x86/kvm/x86.c:10989
>    kvm_vcpu_destroy+0x29/0x90 arch/x86/kvm/../../../virt/kvm/kvm_main.c:441
>    kvm_free_vcpus arch/x86/kvm/x86.c:11426 [inline]
>    kvm_arch_destroy_vm+0x3ef/0x6b0 arch/x86/kvm/x86.c:11545
>    kvm_destroy_vm arch/x86/kvm/../../../virt/kvm/kvm_main.c:1189 [inline]
>    kvm_put_kvm+0x751/0xe40 arch/x86/kvm/../../../virt/kvm/kvm_main.c:1220
>    kvm_vcpu_release+0x53/0x60 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3489
>    __fput+0x3fc/0x870 fs/file_table.c:280
>    task_work_run+0x146/0x1c0 kernel/task_work.c:164
>    exit_task_work include/linux/task_work.h:32 [inline]
>    do_exit+0x705/0x24f0 kernel/exit.c:832
>    do_group_exit+0x168/0x2d0 kernel/exit.c:929
>    get_signal+0x1740/0x2120 kernel/signal.c:2852
>    arch_do_signal_or_restart+0x9c/0x730 arch/x86/kernel/signal.c:868
>    handle_signal_work kernel/entry/common.c:148 [inline]
>    exit_to_user_mode_loop kernel/entry/common.c:172 [inline]
>    exit_to_user_mode_prepare+0x191/0x220 kernel/entry/common.c:207
>    __syscall_exit_to_user_mode_work kernel/entry/common.c:289 [inline]
>    syscall_exit_to_user_mode+0x2e/0x70 kernel/entry/common.c:300
>    do_syscall_64+0x53/0xd0 arch/x86/entry/common.c:86
>    entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Fixes: c8607e4a086f ("KVM: x86: nVMX: don't fail nested VM entry on invalid guest state if !from_vmentry")
> Reported-by: syzbot+f1d2136db9c80d4733e8@syzkaller.appspotmail.com
> Cc: Maxim Levitsky <mlevitsk@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index efcc5a58abbc..9e415e5a91ab 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6631,9 +6631,7 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  	 * consistency check VM-Exit due to invalid guest state and bail.
>  	 */
>  	if (unlikely(vmx->emulation_required)) {
> -
> -		/* We don't emulate invalid state of a nested guest */
> -		vmx->fail = is_guest_mode(vcpu);
> +		vmx->fail = 0;
>  
>  		vmx->exit_reason.full = EXIT_REASON_INVALID_STATE;
>  		vmx->exit_reason.failed_vmentry = 1;

Now after swapping in all of the gory details, this does make sense.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [PATCH 2/4] KVM: nVMX: Synthesize TRIPLE_FAULT for L2 if emulation is required

[Maxim Levitsky]

On Tue, 2021-12-07 at 19:30 +0000, Sean Christopherson wrote:
> Synthesize a triple fault if L2 guest state is invalid at the time of
> VM-Enter, which can happen if L1 modifies SMRAM or if userspace stuffs
> guest state via ioctls(), e.g. KVM_SET_SREGS.  KVM should never emulate
> invalid guest state, since from L1's perspective, it's architecturally
> impossible for L2 to have invalid state while L2 is running in hardware.
> E.g. attempts to set CR0 or CR4 to unsupported values will either VM-Exit
> or #GP.
> 
> Modifying vCPU state via RSM+SMRAM and ioctl() are the only paths that
> can trigger this scenario, as nested VM-Enter correctly rejects any
> attempt to enter L2 with invalid state.
> 
> RSM is a straightforward case as (a) KVM follows AMD's SMRAM layout and
> behavior, and (b) Intel's SDM states that loading reserved CR0/CR4 bits
> via RSM results in shutdown, i.e. there is precedent for KVM's behavior.
> Following AMD's SMRAM layout is important as AMD's layout saves/restores
> the descriptor cache information, including CS.RPL and SS.RPL, and also
> defines all the fields relevant to invalid guest state as read-only, i.e.
> so long as the vCPU had valid state before the SMI, which is guaranteed
> for L2, RSM will generate valid state unless SMRAM was modified.  Intel's
> layout saves/restores only the selector, which means that scenarios where
> the selector and cached RPL don't match, e.g. conforming code segments,
> would yield invalid guest state.  Intel CPUs fudge around this issued by
> stuffing SS.RPL and CS.RPL on RSM.  Per Intel's SDM on the "Default
> Treatment of RSM", paraphrasing for brevity:
> 
>   IF internal storage indicates that the [CPU was post-VMXON]
>   THEN
>      enter VMX operation (root or non-root);
>      restore VMX-critical state as defined in Section 34.14.1;
>      set to their fixed values any bits in CR0 and CR4 whose values must
>      be fixed in VMX operation [unless coming from an unrestricted guest];
>      IF RFLAGS.VM = 0 AND (in VMX root operation OR the
>         “unrestricted guest” VM-execution control is 0)
>      THEN
>        CS.RPL := SS.DPL;
>        SS.RPL := SS.DPL;
>      FI;
>      restore current VMCS pointer;
>   FI;
> 
> Note that Intel CPUs also overwrite the fixed CR0/CR4 bits, whereas KVM
> will sythesize TRIPLE_FAULT in this scenario.  KVM's behavior is allowed
> as both Intel and AMD define CR0/CR4 SMRAM fields as read-only, i.e. the
> only way for CR0 and/or CR4 to have illegal values is if they were
> modified by the L1 SMM handler, and Intel's SDM "SMRAM State Save Map"
> section states "modifying these registers will result in unpredictable
> behavior".
> 
> KVM's ioctl() behavior is less straightforward.  Because KVM allows
> ioctls() to be executed in any order, rejecting an ioctl() if it would
> result in invalid L2 guest state is not an option as KVM cannot know if
> a future ioctl() would resolve the invalid state, e.g. KVM_SET_SREGS, or
> drop the vCPU out of L2, e.g. KVM_SET_NESTED_STATE.  Ideally, KVM would
> reject KVM_RUN if L2 contained invalid guest state, but that carries the
> risk of a false positive, e.g. if RSM loaded invalid guest state and KVM
> exited to userspace.  Setting a flag/request to detect such a scenario is
> undesirable because (a) it's extremely unlikely to add value to KVM as a
> whole, and (b) KVM would need to consider ioctl() interactions with such
> a flag, e.g. if userspace migrated the vCPU while the flag were set.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9e415e5a91ab..5307fcee3b3b 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5900,18 +5900,14 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  		vmx_flush_pml_buffer(vcpu);
>  
>  	/*
> -	 * We should never reach this point with a pending nested VM-Enter, and
> -	 * more specifically emulation of L2 due to invalid guest state (see
> -	 * below) should never happen as that means we incorrectly allowed a
> -	 * nested VM-Enter with an invalid vmcs12.
> +	 * KVM should never reach this point with a pending nested VM-Enter.
> +	 * More specifically, short-circuiting VM-Entry to emulate L2 due to
> +	 * invalid guest state should never happen as that means KVM knowingly
> +	 * allowed a nested VM-Enter with an invalid vmcs12.  More below.
>  	 */
>  	if (KVM_BUG_ON(vmx->nested.nested_run_pending, vcpu->kvm))
>  		return -EIO;
>  
> -	/* If guest state is invalid, start emulating */
> -	if (vmx->emulation_required)
> -		return handle_invalid_guest_state(vcpu);
> -
>  	if (is_guest_mode(vcpu)) {
>  		/*
>  		 * PML is never enabled when running L2, bail immediately if a
> @@ -5933,10 +5929,30 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  		 */
>  		nested_mark_vmcs12_pages_dirty(vcpu);
>  
> +		/*
> +		 * Synthesize a triple fault if L2 state is invalid.  In normal
> +		 * operation, nested VM-Enter rejects any attempt to enter L2
> +		 * with invalid state.  However, those checks are skipped if
> +		 * state is being stuffed via RSM or KVM_SET_NESTED_STATE.  If
> +		 * L2 state is invalid, it means either L1 modified SMRAM state
> +		 * or userspace provided bad state.  Synthesize TRIPLE_FAULT as
> +		 * doing so is architecturally allowed in the RSM case, and is
> +		 * the least awful solution for the userspace case without
> +		 * risking false positives.
> +		 */
> +		if (vmx->emulation_required) {
> +			nested_vmx_vmexit(vcpu, EXIT_REASON_TRIPLE_FAULT, 0, 0);
> +			return 1;
> +		}
> +
>  		if (nested_vmx_reflect_vmexit(vcpu))
>  			return 1;
>  	}
>  
> +	/* If guest state is invalid, start emulating.  L2 is handled above. */
> +	if (vmx->emulation_required)
> +		return handle_invalid_guest_state(vcpu);
> +
>  	if (exit_reason.failed_vmentry) {
>  		dump_vmcs(vcpu);
>  		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;



Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [PATCH 3/4] KVM: VMX: Fix stale docs for kvm-intel.emulate_invalid_guest_state

[Maxim Levitsky]

On Tue, 2021-12-07 at 19:30 +0000, Sean Christopherson wrote:
> Update the documentation for kvm-intel's emulate_invalid_guest_state to
> rectify the description of KVM's default behavior, and to document that
> the behavior and thus parameter only applies to L1.
> 
> Fixes: a27685c33acc ("KVM: VMX: Emulate invalid guest state by default")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 9725c546a0d4..fc34332c8d9a 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2413,8 +2413,12 @@
>  			Default is 1 (enabled)
>  
>  	kvm-intel.emulate_invalid_guest_state=
> -			[KVM,Intel] Enable emulation of invalid guest states
> -			Default is 0 (disabled)
> +			[KVM,Intel] Disable emulation of invalid guest state.
> +			Ignored if kvm-intel.enable_unrestricted_guest=1, as
> +			guest state is never invalid for unrestricted guests.
> +			This param doesn't apply to nested guests (L2), as KVM
> +			never emulates invalid L2 guest state.
> +			Default is 1 (enabled)
>  
>  	kvm-intel.flexpriority=
>  			[KVM,Intel] Disable FlexPriority feature (TPR shadow).
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [PATCH 4/4] KVM: selftests: Add test to verify TRIPLE_FAULT on invalid L2 guest state

[Maxim Levitsky]

On Tue, 2021-12-07 at 19:30 +0000, Sean Christopherson wrote:
> Add a selftest to attempt to enter L2 with invalid guests state by
> exiting to userspace via I/O from L2, and then using KVM_SET_SREGS to set
> invalid guest state (marking TR unusable is arbitrary chosen for its
> relative simplicity).
> 
> This is a regression test for a bug introduced by commit c8607e4a086f
> ("KVM: x86: nVMX: don't fail nested VM entry on invalid guest state if
> !from_vmentry"), which incorrectly set vmx->fail=true when L2 had invalid
> guest state and ultimately triggered a WARN due to nested_vmx_vmexit()
> seeing vmx->fail==true while attempting to synthesize a nested VM-Exit.
> 
> The is also a functional test to verify that KVM sythesizes TRIPLE_FAULT
> for L2, which is somewhat arbitrary behavior, instead of emulating L2.
> KVM should never emulate L2 due to invalid guest state, as it's
> architecturally impossible for L1 to run an L2 guest with invalid state
> as nested VM-Enter should always fail, i.e. L1 needs to do the emulation.
> Stuffing state via KVM ioctl() is a non-architctural, out-of-band case,
> hence the TRIPLE_FAULT being rather arbitrary.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../x86_64/vmx_invalid_nested_guest_state.c   | 105 ++++++++++++++++++
>  3 files changed, 107 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 3763105029fb..de41afb01a77 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -34,6 +34,7 @@
>  /x86_64/vmx_apic_access_test
>  /x86_64/vmx_close_while_nested_test
>  /x86_64/vmx_dirty_log_test
> +/x86_64/vmx_invalid_nested_guest_state
>  /x86_64/vmx_preemption_timer_test
>  /x86_64/vmx_set_nested_state_test
>  /x86_64/vmx_tsc_adjust_test
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index c4e34717826a..6be4144c8d25 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -63,6 +63,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/userspace_msr_exit_test
>  TEST_GEN_PROGS_x86_64 += x86_64/vmx_apic_access_test
>  TEST_GEN_PROGS_x86_64 += x86_64/vmx_close_while_nested_test
>  TEST_GEN_PROGS_x86_64 += x86_64/vmx_dirty_log_test
> +TEST_GEN_PROGS_x86_64 += x86_64/vmx_invalid_nested_guest_state
>  TEST_GEN_PROGS_x86_64 += x86_64/vmx_set_nested_state_test
>  TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
>  TEST_GEN_PROGS_x86_64 += x86_64/vmx_nested_tsc_scaling_test
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c b/tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c
> new file mode 100644
> index 000000000000..489fbed4ca6f
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_invalid_nested_guest_state.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "test_util.h"
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "vmx.h"
> +
> +#include <string.h>
> +#include <sys/ioctl.h>
> +
> +#include "kselftest.h"
> +
> +#define VCPU_ID	0
> +#define ARBITRARY_IO_PORT 0x2000
> +
> +static struct kvm_vm *vm;
> +
> +static void l2_guest_code(void)
> +{
> +	/*
> +	 * Generate an exit to L0 userspace, i.e. main(), via I/O to an
> +	 * arbitrary port.
> +	 */
> +	asm volatile("inb %%dx, %%al"
> +		     : : [port] "d" (ARBITRARY_IO_PORT) : "rax");
> +}
> +
> +static void l1_guest_code(struct vmx_pages *vmx_pages)
> +{
> +#define L2_GUEST_STACK_SIZE 64
> +	unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
> +
> +	GUEST_ASSERT(prepare_for_vmx_operation(vmx_pages));
> +	GUEST_ASSERT(load_vmcs(vmx_pages));
> +
> +	/* Prepare the VMCS for L2 execution. */
> +	prepare_vmcs(vmx_pages, l2_guest_code,
> +		     &l2_guest_stack[L2_GUEST_STACK_SIZE]);
> +
> +	/*
> +	 * L2 must be run without unrestricted guest, verify that the selftests
> +	 * library hasn't enabled it.  Because KVM selftests jump directly to
> +	 * 64-bit mode, unrestricted guest support isn't required.
> +	 */
> +	GUEST_ASSERT(!(vmreadz(CPU_BASED_VM_EXEC_CONTROL) & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) ||
> +		     !(vmreadz(SECONDARY_VM_EXEC_CONTROL) & SECONDARY_EXEC_UNRESTRICTED_GUEST));
> +
> +	GUEST_ASSERT(!vmlaunch());
> +
> +	/* L2 should triple fault after main() stuffs invalid guest state. */
> +	GUEST_ASSERT(vmreadz(VM_EXIT_REASON) == EXIT_REASON_TRIPLE_FAULT);
> +	GUEST_DONE();
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	vm_vaddr_t vmx_pages_gva;
> +	struct kvm_sregs sregs;
> +	struct kvm_run *run;
> +	struct ucall uc;
> +
> +	nested_vmx_check_supported();
> +
> +	vm = vm_create_default(VCPU_ID, 0, (void *) l1_guest_code);
> +
> +	/* Allocate VMX pages and shared descriptors (vmx_pages). */
> +	vcpu_alloc_vmx(vm, &vmx_pages_gva);
> +	vcpu_args_set(vm, VCPU_ID, 1, vmx_pages_gva);
> +
> +	vcpu_run(vm, VCPU_ID);
> +
> +	run = vcpu_state(vm, VCPU_ID);
> +
> +	/*
> +	 * The first exit to L0 userspace should be an I/O access from L2.
> +	 * Running L1 should launch L2 without triggering an exit to userspace.
> +	 */
> +	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
> +		    "Expected KVM_EXIT_IO, got: %u (%s)\n",
> +		    run->exit_reason, exit_reason_str(run->exit_reason));
> +
> +	TEST_ASSERT(run->io.port == ARBITRARY_IO_PORT,
> +		    "Expected IN from port %d from L2, got port %d",
> +		    ARBITRARY_IO_PORT, run->io.port);
> +
> +	/*
> +	 * Stuff invalid guest state for L2 by making TR unusuable.  The next
> +	 * KVM_RUN should induce a TRIPLE_FAULT in L2 as KVM doesn't support
> +	 * emulating invalid guest state for L2.
> +	 */
> +	memset(&sregs, 0, sizeof(sregs));
> +	vcpu_sregs_get(vm, VCPU_ID, &sregs);
> +	sregs.tr.unusable = 1;
> +	vcpu_sregs_set(vm, VCPU_ID, &sregs);
> +
> +	vcpu_run(vm, VCPU_ID);
> +
> +	switch (get_ucall(vm, VCPU_ID, &uc)) {
> +	case UCALL_DONE:
> +		break;
> +	case UCALL_ABORT:
> +		TEST_FAIL("%s", (const char *)uc.args[0]);
> +	default:
> +		TEST_FAIL("Unexpected ucall: %lu", uc.cmd);
> +	}
> +}

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky