[PATCH v2 0/2] KVM: SVM: Fix an STI shadow on VMRUN bug

[PATCH v2 0/2] KVM: SVM: Fix an STI shadow on VMRUN bug

[Sean Christopherson]

Fix a bug where KVM puts VMRUN in an STI shadow, which AMD CPUs bleed
into guest state if a #VMEXIT occurs before completing the VMRUN, e.g.
if vectoring an injected exception triggers an exit.

v2: Use "raw" variants to avoid tracing in noinstr code. [kernel test robot]

v1: https://lore.kernel.org/all/20250215010946.1201353-1-seanjc@google.com

Sean Christopherson (2):
  KVM: SVM: Set RFLAGS.IF=1 in C code, to get VMRUN out of the STI
    shadow
  KVM: selftests: Assert that STI blocking isn't set after event
    injection

 arch/x86/kvm/svm/svm.c                             | 14 ++++++++++++++
 arch/x86/kvm/svm/vmenter.S                         | 10 +---------
 .../selftests/kvm/x86/nested_exceptions_test.c     |  2 ++
 3 files changed, 17 insertions(+), 9 deletions(-)


base-commit: fed48e2967f402f561d80075a20c5c9e16866e53
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v2 1/2] KVM: SVM: Set RFLAGS.IF=1 in C code, to get VMRUN out of the STI shadow

[Sean Christopherson]

Enable/disable local IRQs, i.e. set/clear RFLAGS.IF, in the common
svm_vcpu_enter_exit() just after/before guest_state_{enter,exit}_irqoff()
so that VMRUN is not executed in an STI shadow.  AMD CPUs have a quirk
(some would say "bug"), where the STI shadow bleeds into the guest's
intr_state field if a #VMEXIT occurs during injection of an event, i.e. if
the VMRUN doesn't complete before the subsequent #VMEXIT.

The spurious "interrupts masked" state is relatively benign, as it only
occurs during event injection and is transient.  Because KVM is already
injecting an event, the guest can't be in HLT, and if KVM is querying IRQ
blocking for injection, then KVM would need to force an immediate exit
anyways since injecting multiple events is impossible.

However, because KVM copies int_state verbatim from vmcb02 to vmcb12, the
spurious STI shadow is visible to L1 when running a nested VM, which can
trip sanity checks, e.g. in VMware's VMM.

Hoist the STI+CLI all the way to C code, as the aforementioned calls to
guest_state_{enter,exit}_irqoff() already inform lockdep that IRQs are
enabled/disabled, and taking a fault on VMRUN with RFLAGS.IF=1 is already
possible.  I.e. if there's kernel code that is confused by running with
RFLAGS.IF=1, then it's already a problem.  In practice, since GIF=0 also
blocks NMIs, the only change in exposure to non-KVM code (relative to
surrounding VMRUN with STI+CLI) is exception handling code, and except for
the kvm_rebooting=1 case, all exception in the core VM-Enter/VM-Exit path
are fatal.

Use the "raw" variants to enable/disable IRQs to avoid tracing in the
"no instrumentation" code; the guest state helpers also take care of
tracing IRQ state.

Oppurtunstically document why KVM needs to do STI in the first place.

Reported-by: Doug Covelli <doug.covelli@broadcom.com>
Closes: https://lore.kernel.org/all/CADH9ctBs1YPmE4aCfGPNBwA10cA8RuAk2gO7542DjMZgs4uzJQ@mail.gmail.com
Fixes: f14eec0a3203 ("KVM: SVM: move more vmentry code to assembly")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c     | 14 ++++++++++++++
 arch/x86/kvm/svm/vmenter.S | 10 +---------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b8aa0f36850f..df5c23613b95 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4189,6 +4189,18 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu, bool spec_ctrl_in
 
 	guest_state_enter_irqoff();
 
+	/*
+	 * Set RFLAGS.IF prior to VMRUN, as the host's RFLAGS.IF at the time of
+	 * VMRUN controls whether or not physical IRQs are masked (KVM always
+	 * runs with V_INTR_MASKING_MASK).  Toggle RFLAGS.IF here to avoid the
+	 * temptation to do STI+VMRUN+CLI, as AMD CPUs bleed the STI shadow
+	 * into guest state if delivery of an event during VMRUN triggers a
+	 * #VMEXIT, and the guest_state transitions already tell lockdep that
+	 * IRQs are being enabled/disabled.  Note!  GIF=0 for the entirety of
+	 * this path, so IRQs aren't actually unmasked while running host code.
+	 */
+	raw_local_irq_enable();
+
 	amd_clear_divider();
 
 	if (sev_es_guest(vcpu->kvm))
@@ -4197,6 +4209,8 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu, bool spec_ctrl_in
 	else
 		__svm_vcpu_run(svm, spec_ctrl_intercepted);
 
+	raw_local_irq_disable();
+
 	guest_state_exit_irqoff();
 }
 
diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
index 2ed80aea3bb1..0c61153b275f 100644
--- a/arch/x86/kvm/svm/vmenter.S
+++ b/arch/x86/kvm/svm/vmenter.S
@@ -170,12 +170,8 @@ SYM_FUNC_START(__svm_vcpu_run)
 	mov VCPU_RDI(%_ASM_DI), %_ASM_DI
 
 	/* Enter guest mode */
-	sti
-
 3:	vmrun %_ASM_AX
 4:
-	cli
-
 	/* Pop @svm to RAX while it's the only available register. */
 	pop %_ASM_AX
 
@@ -340,12 +336,8 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
 	mov KVM_VMCB_pa(%rax), %rax
 
 	/* Enter guest mode */
-	sti
-
 1:	vmrun %rax
-
-2:	cli
-
+2:
 	/* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */
 	FILL_RETURN_BUFFER %rax, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT
 
-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH v2 1/2] KVM: SVM: Set RFLAGS.IF=1 in C code, to get VMRUN out of the STI shadow

[Jim Mattson]

On Mon, Feb 24, 2025 at 8:55 AM Sean Christopherson <seanjc@google.com> wrote:
>
> Enable/disable local IRQs, i.e. set/clear RFLAGS.IF, in the common
> svm_vcpu_enter_exit() just after/before guest_state_{enter,exit}_irqoff()
> so that VMRUN is not executed in an STI shadow.  AMD CPUs have a quirk
> (some would say "bug"), where the STI shadow bleeds into the guest's
> intr_state field if a #VMEXIT occurs during injection of an event, i.e. if
> the VMRUN doesn't complete before the subsequent #VMEXIT.
>
> The spurious "interrupts masked" state is relatively benign, as it only
> occurs during event injection and is transient.  Because KVM is already
> injecting an event, the guest can't be in HLT, and if KVM is querying IRQ
> blocking for injection, then KVM would need to force an immediate exit
> anyways since injecting multiple events is impossible.
>
> However, because KVM copies int_state verbatim from vmcb02 to vmcb12, the
> spurious STI shadow is visible to L1 when running a nested VM, which can
> trip sanity checks, e.g. in VMware's VMM.
>
> Hoist the STI+CLI all the way to C code, as the aforementioned calls to
> guest_state_{enter,exit}_irqoff() already inform lockdep that IRQs are
> enabled/disabled, and taking a fault on VMRUN with RFLAGS.IF=1 is already
> possible.  I.e. if there's kernel code that is confused by running with
> RFLAGS.IF=1, then it's already a problem.  In practice, since GIF=0 also
> blocks NMIs, the only change in exposure to non-KVM code (relative to
> surrounding VMRUN with STI+CLI) is exception handling code, and except for
> the kvm_rebooting=1 case, all exception in the core VM-Enter/VM-Exit path
> are fatal.
>
> Use the "raw" variants to enable/disable IRQs to avoid tracing in the
> "no instrumentation" code; the guest state helpers also take care of
> tracing IRQ state.
>
> Oppurtunstically document why KVM needs to do STI in the first place.
>
> Reported-by: Doug Covelli <doug.covelli@broadcom.com>
> Closes: https://lore.kernel.org/all/CADH9ctBs1YPmE4aCfGPNBwA10cA8RuAk2gO7542DjMZgs4uzJQ@mail.gmail.com
> Fixes: f14eec0a3203 ("KVM: SVM: move more vmentry code to assembly")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Jim Mattson <jmattson@google.com>

[PATCH v2 2/2] KVM: selftests: Assert that STI blocking isn't set after event injection

[Sean Christopherson]

Add an L1 (guest) assert to the nested exceptions test to verify that KVM
doesn't put VMRUN in an STI shadow (AMD CPUs bleed the shadow into the
guest's int_state if a #VMEXIT occurs before VMRUN fully completes).

Add a similar assert to the VMX side as well, because why not.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/x86/nested_exceptions_test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86/nested_exceptions_test.c b/tools/testing/selftests/kvm/x86/nested_exceptions_test.c
index 3eb0313ffa39..3641a42934ac 100644
--- a/tools/testing/selftests/kvm/x86/nested_exceptions_test.c
+++ b/tools/testing/selftests/kvm/x86/nested_exceptions_test.c
@@ -85,6 +85,7 @@ static void svm_run_l2(struct svm_test_data *svm, void *l2_code, int vector,
 
 	GUEST_ASSERT_EQ(ctrl->exit_code, (SVM_EXIT_EXCP_BASE + vector));
 	GUEST_ASSERT_EQ(ctrl->exit_info_1, error_code);
+	GUEST_ASSERT(!ctrl->int_state);
 }
 
 static void l1_svm_code(struct svm_test_data *svm)
@@ -122,6 +123,7 @@ static void vmx_run_l2(void *l2_code, int vector, uint32_t error_code)
 	GUEST_ASSERT_EQ(vmreadz(VM_EXIT_REASON), EXIT_REASON_EXCEPTION_NMI);
 	GUEST_ASSERT_EQ((vmreadz(VM_EXIT_INTR_INFO) & 0xff), vector);
 	GUEST_ASSERT_EQ(vmreadz(VM_EXIT_INTR_ERROR_CODE), error_code);
+	GUEST_ASSERT(!vmreadz(GUEST_INTERRUPTIBILITY_INFO));
 }
 
 static void l1_vmx_code(struct vmx_pages *vmx)
-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH v2 2/2] KVM: selftests: Assert that STI blocking isn't set after event injection

[Jim Mattson]

On Mon, Feb 24, 2025 at 8:56 AM Sean Christopherson <seanjc@google.com> wrote:
>
> Add an L1 (guest) assert to the nested exceptions test to verify that KVM
> doesn't put VMRUN in an STI shadow (AMD CPUs bleed the shadow into the
> guest's int_state if a #VMEXIT occurs before VMRUN fully completes).
>
> Add a similar assert to the VMX side as well, because why not.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH v2 0/2] KVM: SVM: Fix an STI shadow on VMRUN bug

[Sean Christopherson]

On Mon, 24 Feb 2025 08:54:40 -0800, Sean Christopherson wrote:
> Fix a bug where KVM puts VMRUN in an STI shadow, which AMD CPUs bleed
> into guest state if a #VMEXIT occurs before completing the VMRUN, e.g.
> if vectoring an injected exception triggers an exit.
> 
> v2: Use "raw" variants to avoid tracing in noinstr code. [kernel test robot]
> 
> v1: https://lore.kernel.org/all/20250215010946.1201353-1-seanjc@google.com
> 
> [...]

Applied to kvm-x86 fixes, thanks!

[1/2] KVM: SVM: Set RFLAGS.IF=1 in C code, to get VMRUN out of the STI shadow
      https://github.com/kvm-x86/linux/commit/be45bc4eff33
[2/2] KVM: selftests: Assert that STI blocking isn't set after event injection
      https://github.com/kvm-x86/linux/commit/f3513a335e71

--
https://github.com/kvm-x86/linux/tree/next