[PATCH 0/2] KVM: SVM: Fix unexpected #UD on INT3 in SEV guests

[PATCH 0/2] KVM: SVM: Fix unexpected #UD on INT3 in SEV guests

[Sean Christopherson]

Fix a bug where KVM injects a bogus #UD for SEV guests when trying to skip
an INT3 as part of re-injecting the associated #BP that got kinda sorta
intercepted due to a #NPF occuring while vectoring/delivering the #BP.

I haven't actually confirmed that patch 1 fixes the bug, as it's a
different change than what I originally proposed.  I'm 99% certain it will
work, but I definitely need verification that it fixes the problem

Patch 2 is a tangentially related cleanup to make NRIPS a requirement for
enabling SEV, e.g. so that we don't ever get "bug" reports of SEV guests
not working when NRIPS is disabled.

Sean Christopherson (2):
  KVM: SVM: Don't inject #UD if KVM attempts emulation of SEV guest w/o
    insn
  KVM: SVM: Require nrips support for SEV guests (and beyond)

 arch/x86/kvm/svm/sev.c |  2 +-
 arch/x86/kvm/svm/svm.c | 37 ++++++++++++++++++++-----------------
 arch/x86/kvm/svm/svm.h |  1 +
 3 files changed, 22 insertions(+), 18 deletions(-)


base-commit: 240f736891887939571854bd6d734b6c9291f22e
-- 
2.41.0.694.ge786442a9b-goog

[PATCH 1/2] KVM: SVM: Don't inject #UD if KVM attempts emulation of SEV guest w/o insn

[Sean Christopherson]

Don't inject a #UD if KVM attempts to emulate an instruction for an SEV
guest without a prefilled buffer, and instead resume the guest and hope
that it can make forward progress.  When commit 04c40f344def ("KVM: SVM:
Inject #UD on attempted emulation for SEV guest w/o insn buffer") added
the completely arbitrary #UD behavior, there were no known scenarios where
a well-behaved guest would induce a VM-Exit that triggered emulation, i.e.
it was thought that injecting #UD would be helpful.

However, now that KVM (correctly) attempts to re-inject INT3/INTO, e.g. if
a #NPF is encountered when attempting to deliver the INT3/INTO, an SEV
guest can trigger emulation without a buffer, through no fault of its own.
Resuming the guest and retrying the INT3/INTO is architecturally wrong,
e.g. the vCPU will incorrectly re-hit code #DBs, but for SEV guests there
is literally no other option that has a chance of making forward progress.

Drop the #UD injection for all flavors of emulation, even though that
means that a *misbehaving* guest will effectively end up in an infinite
loop instead of getting a #UD.  There's no evidence that suggests that an
unexpected #UD is actually better than hanging the vCPU, e.g. a soft-hung
vCPU can still respond to IRQs and NMIs to generate a backtrace.

Reported-by: Wu Zongyo <wuzongyo@mail.ustc.edu.cn>
Closes: https://lore.kernel.org/all/8eb933fd-2cf3-d7a9-32fe-2a1d82eac42a@mail.ustc.edu.cn
Fixes: 6ef88d6e36c2 ("KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction")
Cc: stable@vger.kernel.org
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 212706d18c62..581958c9dd4d 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4725,18 +4725,24 @@ static bool svm_can_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
 	 * and cannot be decrypted by KVM, i.e. KVM would read cyphertext and
 	 * decode garbage.
 	 *
-	 * Inject #UD if KVM reached this point without an instruction buffer.
-	 * In practice, this path should never be hit by a well-behaved guest,
-	 * e.g. KVM doesn't intercept #UD or #GP for SEV guests, but this path
-	 * is still theoretically reachable, e.g. via unaccelerated fault-like
-	 * AVIC access, and needs to be handled by KVM to avoid putting the
-	 * guest into an infinite loop.   Injecting #UD is somewhat arbitrary,
-	 * but its the least awful option given lack of insight into the guest.
+	 * Resume the guest if KVM reached this point without an instruction
+	 * buffer.  This path should *almost* never be hit by a well-behaved
+	 * guest, e.g. KVM doesn't intercept #UD or #GP for SEV guests.  But if
+	 * a #NPF occurs while the guest is vectoring an INT3/INTO, then KVM
+	 * will attempt to re-inject the INT3/INTO and skip the instruction.
+	 * In that scenario, retrying the INT3/INTO and hoping the guest will
+	 * make forward progress is the only option that has a chance of
+	 * success (and in practice it will work the vast majority of the time).
+	 *
+	 * This path is also theoretically reachable if the guest is doing
+	 * something odd, e.g. if the guest is triggering unaccelerated fault-
+	 * like AVIC access.  Resuming the guest will put it into an infinite
+	 * loop of sorts, but there's no hope of forward progress and injecting
+	 * an exception will at best yield confusing behavior, not to mention
+	 * break the INT3/INTO+#NPF case above.
 	 */
-	if (unlikely(!insn)) {
-		kvm_queue_exception(vcpu, UD_VECTOR);
+	if (unlikely(!insn))
 		return false;
-	}
 
 	/*
 	 * Emulate for SEV guests if the insn buffer is not empty.  The buffer
-- 
2.41.0.694.ge786442a9b-goog

[PATCH 2/2] KVM: SVM: Require nrips support for SEV guests (and beyond)

[Sean Christopherson]

Disallow SEV (and beyond) if nrips is disabled via module param, as KVM
can't read guest memory to partially emulate and skip an instruction.  All
CPUs that support SEV support NRIPS, i.e. this is purely stopping the user
from shooting themselves in the foot.

Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c |  2 +-
 arch/x86/kvm/svm/svm.c | 11 ++++-------
 arch/x86/kvm/svm/svm.h |  1 +
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 2cd15783dfb9..8ce9ffc8709e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2185,7 +2185,7 @@ void __init sev_hardware_setup(void)
 	bool sev_es_supported = false;
 	bool sev_supported = false;
 
-	if (!sev_enabled || !npt_enabled)
+	if (!sev_enabled || !npt_enabled || !nrips)
 		goto out;
 
 	/*
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 581958c9dd4d..7cb5ef5835c2 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -202,7 +202,7 @@ static int nested = true;
 module_param(nested, int, S_IRUGO);
 
 /* enable/disable Next RIP Save */
-static int nrips = true;
+int nrips = true;
 module_param(nrips, int, 0444);
 
 /* enable/disable Virtual VMLOAD VMSAVE */
@@ -5191,9 +5191,11 @@ static __init int svm_hardware_setup(void)
 
 	svm_adjust_mmio_mask();
 
+	nrips = nrips && boot_cpu_has(X86_FEATURE_NRIPS);
+
 	/*
 	 * Note, SEV setup consumes npt_enabled and enable_mmio_caching (which
-	 * may be modified by svm_adjust_mmio_mask()).
+	 * may be modified by svm_adjust_mmio_mask()), as well as nrips.
 	 */
 	sev_hardware_setup();
 
@@ -5205,11 +5207,6 @@ static __init int svm_hardware_setup(void)
 			goto err;
 	}
 
-	if (nrips) {
-		if (!boot_cpu_has(X86_FEATURE_NRIPS))
-			nrips = false;
-	}
-
 	enable_apicv = avic = avic && avic_hardware_setup();
 
 	if (!enable_apicv) {
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 2237230aad98..860511276087 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -34,6 +34,7 @@
 #define MSRPM_OFFSETS	32
 extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
 extern bool npt_enabled;
+extern int nrips;
 extern int vgif;
 extern bool intercept_smi;
 extern bool x2avic_enabled;
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH 0/2] KVM: SVM: Fix unexpected #UD on INT3 in SEV guests

[Tom Lendacky]

On 8/10/23 18:49, Sean Christopherson wrote:
> Fix a bug where KVM injects a bogus #UD for SEV guests when trying to skip
> an INT3 as part of re-injecting the associated #BP that got kinda sorta
> intercepted due to a #NPF occuring while vectoring/delivering the #BP.
> 
> I haven't actually confirmed that patch 1 fixes the bug, as it's a
> different change than what I originally proposed.  I'm 99% certain it will
> work, but I definitely need verification that it fixes the problem
> 
> Patch 2 is a tangentially related cleanup to make NRIPS a requirement for
> enabling SEV, e.g. so that we don't ever get "bug" reports of SEV guests
> not working when NRIPS is disabled.
> 
> Sean Christopherson (2):
>    KVM: SVM: Don't inject #UD if KVM attempts emulation of SEV guest w/o
>      insn
>    KVM: SVM: Require nrips support for SEV guests (and beyond)
> 
>   arch/x86/kvm/svm/sev.c |  2 +-
>   arch/x86/kvm/svm/svm.c | 37 ++++++++++++++++++++-----------------
>   arch/x86/kvm/svm/svm.h |  1 +
>   3 files changed, 22 insertions(+), 18 deletions(-)

We ran some stress tests against a version of the kernel without this fix 
and we're able to reproduce the issue, but not reliably, after a few 
hours. With this patch, it has not reproduced after running for a week.

Not as reliable a scenario as the original reporter, but this looks like 
it resolves the issue.

So, for the series:

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> 
> 
> base-commit: 240f736891887939571854bd6d734b6c9291f22e

Re: [PATCH 0/2] KVM: SVM: Fix unexpected #UD on INT3 in SEV guests

[Sean Christopherson]

On Tue, Aug 22, 2023, Tom Lendacky wrote:
> On 8/10/23 18:49, Sean Christopherson wrote:
> > Fix a bug where KVM injects a bogus #UD for SEV guests when trying to skip
> > an INT3 as part of re-injecting the associated #BP that got kinda sorta
> > intercepted due to a #NPF occuring while vectoring/delivering the #BP.
> > 
> > I haven't actually confirmed that patch 1 fixes the bug, as it's a
> > different change than what I originally proposed.  I'm 99% certain it will
> > work, but I definitely need verification that it fixes the problem
> > 
> > Patch 2 is a tangentially related cleanup to make NRIPS a requirement for
> > enabling SEV, e.g. so that we don't ever get "bug" reports of SEV guests
> > not working when NRIPS is disabled.
> > 
> > Sean Christopherson (2):
> >    KVM: SVM: Don't inject #UD if KVM attempts emulation of SEV guest w/o
> >      insn
> >    KVM: SVM: Require nrips support for SEV guests (and beyond)
> > 
> >   arch/x86/kvm/svm/sev.c |  2 +-
> >   arch/x86/kvm/svm/svm.c | 37 ++++++++++++++++++++-----------------
> >   arch/x86/kvm/svm/svm.h |  1 +
> >   3 files changed, 22 insertions(+), 18 deletions(-)
> 
> We ran some stress tests against a version of the kernel without this fix
> and we're able to reproduce the issue, but not reliably, after a few hours.
> With this patch, it has not reproduced after running for a week.
> 
> Not as reliable a scenario as the original reporter, but this looks like it
> resolves the issue.

Thanks Tom!  I'll apply this for v6.6, that'll give us plenty of time to change
course if necessary.

Re: [PATCH 0/2] KVM: SVM: Fix unexpected #UD on INT3 in SEV guests

[Tom Lendacky]

On 8/22/23 10:14, Sean Christopherson wrote:
> On Tue, Aug 22, 2023, Tom Lendacky wrote:
>> On 8/10/23 18:49, Sean Christopherson wrote:
>>> Fix a bug where KVM injects a bogus #UD for SEV guests when trying to skip
>>> an INT3 as part of re-injecting the associated #BP that got kinda sorta
>>> intercepted due to a #NPF occuring while vectoring/delivering the #BP.
>>>
>>> I haven't actually confirmed that patch 1 fixes the bug, as it's a
>>> different change than what I originally proposed.  I'm 99% certain it will
>>> work, but I definitely need verification that it fixes the problem
>>>
>>> Patch 2 is a tangentially related cleanup to make NRIPS a requirement for
>>> enabling SEV, e.g. so that we don't ever get "bug" reports of SEV guests
>>> not working when NRIPS is disabled.
>>>
>>> Sean Christopherson (2):
>>>     KVM: SVM: Don't inject #UD if KVM attempts emulation of SEV guest w/o
>>>       insn
>>>     KVM: SVM: Require nrips support for SEV guests (and beyond)
>>>
>>>    arch/x86/kvm/svm/sev.c |  2 +-
>>>    arch/x86/kvm/svm/svm.c | 37 ++++++++++++++++++++-----------------
>>>    arch/x86/kvm/svm/svm.h |  1 +
>>>    3 files changed, 22 insertions(+), 18 deletions(-)
>>
>> We ran some stress tests against a version of the kernel without this fix
>> and we're able to reproduce the issue, but not reliably, after a few hours.
>> With this patch, it has not reproduced after running for a week.
>>
>> Not as reliable a scenario as the original reporter, but this looks like it
>> resolves the issue.
> 
> Thanks Tom!  I'll apply this for v6.6, that'll give us plenty of time to change
> course if necessary.

I may have spoke to soon...  When the #UD was triggered it was here:

[    0.118524] Spectre V2 : Enabling Restricted Speculation for firmware calls
[    0.118524] Spectre V2 : mitigation: Enabling conditional Indirect Branch Prediction Barrier
[    0.118524] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl
[    0.118524] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[    0.118524] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.2.2-amdsos-build50-ubuntu-20.04+ #1
[    0.118524] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[    0.118524] RIP: 0010:int3_selftest_ip+0x0/0x60
[    0.118524] Code: b9 25 05 00 00 48 c7 c2 e8 7c 80 b0 48 c7 c6 fe 1c d3 b0 48 c7 c7 f0 7d da b0 e8 4c 2c 0b ff e8 75 da 15 ff 0f 0b 48 8d 7d f4 <cc> 90 90 90 90 83 7d f4 01 74 2f 80 3d 39 7f a8 00 00 74 24 b9 34


Now (after about a week) we've encountered a hang here:

[    0.106216] Spectre V2 : Enabling Restricted Speculation for firmware calls
[    0.106216] Spectre V2 : mitigation: Enabling conditional Indirect Branch Prediction Barrier
[    0.106216] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl

It is in the very same spot and so I wonder if the return false (without
queuing a #UD) is causing an infinite loop here that appears as a guest
hang. Whereas, we have some systems running the first patch that you
created that have not hit this hang.

But I'm not sure why or how this patch could cause the guest hang. I
would think that the retry of the instruction would resolve everything
and the guest would continue. Unfortunately, the guest was killed, so I'll
try to reproduce and get a dump or trace points of the VM to see what is
going on.

Thanks,
Tom

Re: [PATCH 0/2] KVM: SVM: Fix unexpected #UD on INT3 in SEV guests

[Sean Christopherson]

On Wed, Aug 23, 2023, Tom Lendacky wrote:
> On 8/22/23 10:14, Sean Christopherson wrote:
> > On Tue, Aug 22, 2023, Tom Lendacky wrote:
> > > On 8/10/23 18:49, Sean Christopherson wrote:
> > > > Fix a bug where KVM injects a bogus #UD for SEV guests when trying to skip
> > > > an INT3 as part of re-injecting the associated #BP that got kinda sorta
> > > > intercepted due to a #NPF occuring while vectoring/delivering the #BP.
> > > > 
> > > > I haven't actually confirmed that patch 1 fixes the bug, as it's a
> > > > different change than what I originally proposed.  I'm 99% certain it will
> > > > work, but I definitely need verification that it fixes the problem
> > > > 
> > > > Patch 2 is a tangentially related cleanup to make NRIPS a requirement for
> > > > enabling SEV, e.g. so that we don't ever get "bug" reports of SEV guests
> > > > not working when NRIPS is disabled.
> > > > 
> > > > Sean Christopherson (2):
> > > >     KVM: SVM: Don't inject #UD if KVM attempts emulation of SEV guest w/o
> > > >       insn
> > > >     KVM: SVM: Require nrips support for SEV guests (and beyond)
> > > > 
> > > >    arch/x86/kvm/svm/sev.c |  2 +-
> > > >    arch/x86/kvm/svm/svm.c | 37 ++++++++++++++++++++-----------------
> > > >    arch/x86/kvm/svm/svm.h |  1 +
> > > >    3 files changed, 22 insertions(+), 18 deletions(-)
> > > 
> > > We ran some stress tests against a version of the kernel without this fix
> > > and we're able to reproduce the issue, but not reliably, after a few hours.
> > > With this patch, it has not reproduced after running for a week.
> > > 
> > > Not as reliable a scenario as the original reporter, but this looks like it
> > > resolves the issue.
> > 
> > Thanks Tom!  I'll apply this for v6.6, that'll give us plenty of time to change
> > course if necessary.
> 
> I may have spoke to soon...  When the #UD was triggered it was here:
> 
> [    0.118524] Spectre V2 : Enabling Restricted Speculation for firmware calls
> [    0.118524] Spectre V2 : mitigation: Enabling conditional Indirect Branch Prediction Barrier
> [    0.118524] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl
> [    0.118524] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [    0.118524] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.2.2-amdsos-build50-ubuntu-20.04+ #1
> [    0.118524] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> [    0.118524] RIP: 0010:int3_selftest_ip+0x0/0x60
> [    0.118524] Code: b9 25 05 00 00 48 c7 c2 e8 7c 80 b0 48 c7 c6 fe 1c d3 b0 48 c7 c7 f0 7d da b0 e8 4c 2c 0b ff e8 75 da 15 ff 0f 0b 48 8d 7d f4 <cc> 90 90 90 90 83 7d f4 01 74 2f 80 3d 39 7f a8 00 00 74 24 b9 34
> 
> 
> Now (after about a week) we've encountered a hang here:
> 
> [    0.106216] Spectre V2 : Enabling Restricted Speculation for firmware calls
> [    0.106216] Spectre V2 : mitigation: Enabling conditional Indirect Branch Prediction Barrier
> [    0.106216] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl
> 
> It is in the very same spot and so I wonder if the return false (without
> queuing a #UD) is causing an infinite loop here that appears as a guest
> hang. Whereas, we have some systems running the first patch that you
> created that have not hit this hang.
> 
> But I'm not sure why or how this patch could cause the guest hang. I
> would think that the retry of the instruction would resolve everything
> and the guest would continue. Unfortunately, the guest was killed, so I'll
> try to reproduce and get a dump or trace points of the VM to see what is
> going on.

Gah, it's because x86_emulate_instruction() returns '1' and not '0' when
svm_can_emulate_instruction() returns false.  svm_update_soft_interrupt_rip()
would then continue with the injection, i.e. inject #BP on the INT3 RIP, not on
the RIP following the INT3, which would cause this check to fail

	if (regs->ip - INT3_INSN_SIZE != selftest)
		return NOTIFY_DONE;

and eventually send do_trap_no_signal() to die().

I distinctly remember seeing the return value problem when writing the patch, but
missed that it would result in KVM injecting the unexpected #BP.

I punted on trying to properly fix this by having can_emulate_instruction()
differentiate between "retry insn" and "inject exception", because that change
is painfully invasive and I though I could get away with the simple fix.  Drat.

I think the best option is to add a "temporary" patch so that the fix for @stable
is short, sweet, and safe, and then do the can_emulate_instruction() cleanup that
I was avoiding.

E.g. this as patch 2/4 (or maybe 2/5) of this series:

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7cb5ef5835c2..8457a36b44c1 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -364,6 +364,8 @@ static void svm_set_interrupt_shadow(struct kvm_vcpu *vcpu, int mask)
                svm->vmcb->control.int_state |= SVM_INTERRUPT_SHADOW_MASK;
 
 }
+static bool svm_can_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
+                                       void *insn, int insn_len);
 
 static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
                                           bool commit_side_effects)
@@ -384,6 +386,14 @@ static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
        }
 
        if (!svm->next_rip) {
+               /*
+                * FIXME: Drop this when kvm_emulate_instruction() does the
+                * right thing and treats "can't emulate" as outright failure
+                * for EMULTYPE_SKIP.
+                */
+               if (!svm_can_emulate_instruction(vcpu, EMULTYPE_SKIP, NULL, 0))
+                       return 0;
+
                if (unlikely(!commit_side_effects))
                        old_rflags = svm->vmcb->save.rflags;

Re: [PATCH 0/2] KVM: SVM: Fix unexpected #UD on INT3 in SEV guests

[Tom Lendacky]

On 8/23/23 15:03, Sean Christopherson wrote:
> On Wed, Aug 23, 2023, Tom Lendacky wrote:
>> On 8/22/23 10:14, Sean Christopherson wrote:
>>> On Tue, Aug 22, 2023, Tom Lendacky wrote:
>>>> On 8/10/23 18:49, Sean Christopherson wrote:
>>>>> Fix a bug where KVM injects a bogus #UD for SEV guests when trying to skip
>>>>> an INT3 as part of re-injecting the associated #BP that got kinda sorta
>>>>> intercepted due to a #NPF occuring while vectoring/delivering the #BP.
>>>>>
>>>>> I haven't actually confirmed that patch 1 fixes the bug, as it's a
>>>>> different change than what I originally proposed.  I'm 99% certain it will
>>>>> work, but I definitely need verification that it fixes the problem
>>>>>
>>>>> Patch 2 is a tangentially related cleanup to make NRIPS a requirement for
>>>>> enabling SEV, e.g. so that we don't ever get "bug" reports of SEV guests
>>>>> not working when NRIPS is disabled.
>>>>>
>>>>> Sean Christopherson (2):
>>>>>      KVM: SVM: Don't inject #UD if KVM attempts emulation of SEV guest w/o
>>>>>        insn
>>>>>      KVM: SVM: Require nrips support for SEV guests (and beyond)
>>>>>
>>>>>     arch/x86/kvm/svm/sev.c |  2 +-
>>>>>     arch/x86/kvm/svm/svm.c | 37 ++++++++++++++++++++-----------------
>>>>>     arch/x86/kvm/svm/svm.h |  1 +
>>>>>     3 files changed, 22 insertions(+), 18 deletions(-)
>>>>
>>>> We ran some stress tests against a version of the kernel without this fix
>>>> and we're able to reproduce the issue, but not reliably, after a few hours.
>>>> With this patch, it has not reproduced after running for a week.
>>>>
>>>> Not as reliable a scenario as the original reporter, but this looks like it
>>>> resolves the issue.
>>>
>>> Thanks Tom!  I'll apply this for v6.6, that'll give us plenty of time to change
>>> course if necessary.
>>
>> I may have spoke to soon...  When the #UD was triggered it was here:
>>
>> [    0.118524] Spectre V2 : Enabling Restricted Speculation for firmware calls
>> [    0.118524] Spectre V2 : mitigation: Enabling conditional Indirect Branch Prediction Barrier
>> [    0.118524] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl
>> [    0.118524] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
>> [    0.118524] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.2.2-amdsos-build50-ubuntu-20.04+ #1
>> [    0.118524] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
>> [    0.118524] RIP: 0010:int3_selftest_ip+0x0/0x60
>> [    0.118524] Code: b9 25 05 00 00 48 c7 c2 e8 7c 80 b0 48 c7 c6 fe 1c d3 b0 48 c7 c7 f0 7d da b0 e8 4c 2c 0b ff e8 75 da 15 ff 0f 0b 48 8d 7d f4 <cc> 90 90 90 90 83 7d f4 01 74 2f 80 3d 39 7f a8 00 00 74 24 b9 34
>>
>>
>> Now (after about a week) we've encountered a hang here:
>>
>> [    0.106216] Spectre V2 : Enabling Restricted Speculation for firmware calls
>> [    0.106216] Spectre V2 : mitigation: Enabling conditional Indirect Branch Prediction Barrier
>> [    0.106216] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl
>>
>> It is in the very same spot and so I wonder if the return false (without
>> queuing a #UD) is causing an infinite loop here that appears as a guest
>> hang. Whereas, we have some systems running the first patch that you
>> created that have not hit this hang.
>>
>> But I'm not sure why or how this patch could cause the guest hang. I
>> would think that the retry of the instruction would resolve everything
>> and the guest would continue. Unfortunately, the guest was killed, so I'll
>> try to reproduce and get a dump or trace points of the VM to see what is
>> going on.
> 
> Gah, it's because x86_emulate_instruction() returns '1' and not '0' when
> svm_can_emulate_instruction() returns false.  svm_update_soft_interrupt_rip()
> would then continue with the injection, i.e. inject #BP on the INT3 RIP, not on
> the RIP following the INT3, which would cause this check to fail
> 
> 	if (regs->ip - INT3_INSN_SIZE != selftest)
> 		return NOTIFY_DONE;
> 
> and eventually send do_trap_no_signal() to die().

Thanks for figuring that out so quickly!

> 
> I distinctly remember seeing the return value problem when writing the patch, but
> missed that it would result in KVM injecting the unexpected #BP.
> 
> I punted on trying to properly fix this by having can_emulate_instruction()
> differentiate between "retry insn" and "inject exception", because that change
> is painfully invasive and I though I could get away with the simple fix.  Drat.
> 
> I think the best option is to add a "temporary" patch so that the fix for @stable
> is short, sweet, and safe, and then do the can_emulate_instruction() cleanup that
> I was avoiding.
> 
> E.g. this as patch 2/4 (or maybe 2/5) of this series:

2/4 or 2/5? Do you mean 2/3 since there were only 2 patches in the series?

I'll apply the below patch in between patches 1 and 2 and re-test. Should 
have results in a week :)

Thanks,
Tom

> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7cb5ef5835c2..8457a36b44c1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -364,6 +364,8 @@ static void svm_set_interrupt_shadow(struct kvm_vcpu *vcpu, int mask)
>                  svm->vmcb->control.int_state |= SVM_INTERRUPT_SHADOW_MASK;
>   
>   }
> +static bool svm_can_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
> +                                       void *insn, int insn_len);
>   
>   static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
>                                             bool commit_side_effects)
> @@ -384,6 +386,14 @@ static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
>          }
>   
>          if (!svm->next_rip) {
> +               /*
> +                * FIXME: Drop this when kvm_emulate_instruction() does the
> +                * right thing and treats "can't emulate" as outright failure
> +                * for EMULTYPE_SKIP.
> +                */
> +               if (!svm_can_emulate_instruction(vcpu, EMULTYPE_SKIP, NULL, 0))
> +                       return 0;
> +
>                  if (unlikely(!commit_side_effects))
>                          old_rflags = svm->vmcb->save.rflags;

Re: [PATCH 0/2] KVM: SVM: Fix unexpected #UD on INT3 in SEV guests

[Sean Christopherson]

On Wed, Aug 23, 2023, Tom Lendacky wrote:
> On 8/23/23 15:03, Sean Christopherson wrote:
> > I think the best option is to add a "temporary" patch so that the fix for @stable
> > is short, sweet, and safe, and then do the can_emulate_instruction() cleanup that
> > I was avoiding.
> > 
> > E.g. this as patch 2/4 (or maybe 2/5) of this series:
> 
> 2/4 or 2/5? Do you mean 2/3 since there were only 2 patches in the series?

I am planning on adding more patches in v2 to cleanup the hack-a-fix. But after
writing the code, I think it's best to sqaush the hack-a-fix in with patch 1
(new full patch at the bottom, though it should be the same result as the earlier
delta patch).

> I'll apply the below patch in between patches 1 and 2 and re-test. Should
> have results in a week :)

Heh, maybe if we send enough emails we can get Wu's feedback before then :-)

One idea to make the original bug repro on every run would be to constantly
toggle nx_huge_pages between "off" and "force" while the guest is booting.  Toggling
nx_huge_pages should force KVM to rebuild the SPTEs and all but guarantee trying
to deliver the #BP will hit a #NPF.

--
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 8 Aug 2023 17:18:42 -0700
Subject: [PATCH 1/4] KVM: SVM: Don't inject #UD if KVM attempts to skip SEV
 guest insn

Don't inject a #UD if KVM attempts to "emulate" to skip an instruction
for an SEV guest, and instead resume the guest and hope that it can make
forward progress.  When commit 04c40f344def ("KVM: SVM: Inject #UD on
attempted emulation for SEV guest w/o insn buffer") added the completely
arbitrary #UD behavior, there were no known scenarios where a well-behaved
guest would induce a VM-Exit that triggered emulation, i.e. it was thought
that injecting #UD would be helpful.

However, now that KVM (correctly) attempts to re-inject INT3/INTO, e.g. if
a #NPF is encountered when attempting to deliver the INT3/INTO, an SEV
guest can trigger emulation without a buffer, through no fault of its own.
Resuming the guest and retrying the INT3/INTO is architecturally wrong,
e.g. the vCPU will incorrectly re-hit code #DBs, but for SEV guests there
is literally no other option that has a chance of making forward progress.

Drop the #UD injection for all "skip" emulation, not just those related to
INT3/INTO, even though that means that the guest will likely end up in an
infinite loop instead of getting a #UD (the vCPU may also crash, e.g. if
KVM emulated everything about an instruction except for advancing RIP).
There's no evidence that suggests that an unexpected #UD is actually
better than hanging the vCPU, e.g. a soft-hung vCPU can still respond to
IRQs and NMIs to generate a backtrace.

Reported-by: Wu Zongyo <wuzongyo@mail.ustc.edu.cn>
Closes: https://lore.kernel.org/all/8eb933fd-2cf3-d7a9-32fe-2a1d82eac42a@mail.ustc.edu.cn
Fixes: 6ef88d6e36c2 ("KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction")
Cc: stable@vger.kernel.org
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 212706d18c62..f6adc43b315a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -364,6 +364,8 @@ static void svm_set_interrupt_shadow(struct kvm_vcpu *vcpu, int mask)
 		svm->vmcb->control.int_state |= SVM_INTERRUPT_SHADOW_MASK;
 
 }
+static bool svm_can_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
+					void *insn, int insn_len);
 
 static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
 					   bool commit_side_effects)
@@ -384,6 +386,14 @@ static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
 	}
 
 	if (!svm->next_rip) {
+		/*
+		 * FIXME: Drop this when kvm_emulate_instruction() does the
+		 * right thing and treats "can't emulate" as outright failure
+		 * for EMULTYPE_SKIP.
+		 */
+		if (!svm_can_emulate_instruction(vcpu, EMULTYPE_SKIP, NULL, 0))
+			return 0;
+
 		if (unlikely(!commit_side_effects))
 			old_rflags = svm->vmcb->save.rflags;
 
@@ -4725,16 +4735,25 @@ static bool svm_can_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
 	 * and cannot be decrypted by KVM, i.e. KVM would read cyphertext and
 	 * decode garbage.
 	 *
-	 * Inject #UD if KVM reached this point without an instruction buffer.
-	 * In practice, this path should never be hit by a well-behaved guest,
-	 * e.g. KVM doesn't intercept #UD or #GP for SEV guests, but this path
-	 * is still theoretically reachable, e.g. via unaccelerated fault-like
-	 * AVIC access, and needs to be handled by KVM to avoid putting the
-	 * guest into an infinite loop.   Injecting #UD is somewhat arbitrary,
-	 * but its the least awful option given lack of insight into the guest.
+	 * If KVM is NOT trying to simply skip an instruction, inject #UD if
+	 * KVM reached this point without an instruction buffer.  In practice,
+	 * this path should never be hit by a well-behaved guest, e.g. KVM
+	 * doesn't intercept #UD or #GP for SEV guests, but this path is still
+	 * theoretically reachable, e.g. via unaccelerated fault-like AVIC
+	 * access, and needs to be handled by KVM to avoid putting the guest
+	 * into an infinite loop.   Injecting #UD is somewhat arbitrary, but
+	 * its the least awful option given lack of insight into the guest.
+	 *
+	 * If KVM is trying to skip an instruction, simply resume the guest.
+	 * If a #NPF occurs while the guest is vectoring an INT3/INTO, then KVM
+	 * will attempt to re-inject the INT3/INTO and skip the instruction.
+	 * In that scenario, retrying the INT3/INTO and hoping the guest will
+	 * make forward progress is the only option that has a chance of
+	 * success (and in practice it will work the vast majority of the time).
 	 */
 	if (unlikely(!insn)) {
-		kvm_queue_exception(vcpu, UD_VECTOR);
+		if (!(emul_type & EMULTYPE_SKIP))
+			kvm_queue_exception(vcpu, UD_VECTOR);
 		return false;
 	}
 

base-commit: 240f736891887939571854bd6d734b6c9291f22e
-- 

Re: [PATCH 0/2] KVM: SVM: Fix unexpected #UD on INT3 in SEV guests

[Sean Christopherson]

On Wed, Aug 23, 2023, Sean Christopherson wrote:
> One idea to make the original bug repro on every run would be to constantly
> toggle nx_huge_pages between "off" and "force" while the guest is booting.  Toggling
> nx_huge_pages should force KVM to rebuild the SPTEs and all but guarantee trying
> to deliver the #BP will hit a #NPF.

Mwhahaha.  That, plus a delay in the guest and disabling THP, makes this 100%
reproducible.  I'll verify the fix actually works before posting v2.