[PATCH 0/3] KVM: SMM fixes

[PATCH 0/3] KVM: SMM fixes

[Maxim Levitsky]

Hi!

I did first round of testing of SMM by flooding the guest with SMIs,
and running nested guests in it, and I found out that SMM
breaks nested KVM due to a refactoring change
that was done in 5.12 kernel. Fix for this is in patch 1.

I also fixed another issue I noticed in this patch which is purely
theoretical but nevertheless should be fixed. This is patch 2.

I also propose to add (mostly for debug for now) a module param
that can make the KVM to avoid intercepting #SMIs on SVM.
(Intel doesn't have such intercept I think)
The default is still to intercept #SMI so nothing is changed by
default.

This allows to test the case in which SMI are not intercepted,
by L1 without running Windows (which doesn't intercept #SMI).

In addition to that I found out that on bare metal, at least
on two Zen2 machines I have, the CPU ignores SMI interception and
never VM exits when SMI is received. As I guessed earlier
this must have been done for security reasons.

Note that bug that I fixed in patch 1, should crash VMs very soon
on bare metal as well, if the CPU were to honour the SMI intercept.
as long as there are some SMIs generated while the system is running.

I tested this on bare metal by using local APIC to send SMIs
to all real CPUs, and also used ioport 0xB2 to send SMIs.
In both cases my system slowed to a crawl but didn't show
any SMI vmexits (SMI intercept was enabled).

In a VM I also used ioport 0xB2 to generate a flood of SMIs,
which allowed me to reproduce this bug (and with intercept_smi=0
module parameter I can reproduce the bug that Vitaly fixed in
his series as well while just running nested KVM).

Note that while doing nested migration I am still able to cause
severe hangs of the L1 when I run the SMI stress test in L1
and a nested VM. VM isn't fully hung but its GUI stops responding,
and I see lots of cpu lockups errors in dmesg.
This seems to happen regardless of #SMI interception in the L1
(with Vitaly's patches applied of course)

Best regards,
	Maxim Levitsky

Maxim Levitsky (3):
  KVM: SVM: #SMI interception must not skip the instruction
  KVM: SVM: remove INIT intercept handler
  KVM: SVM: add module param to control the #SMI interception

 arch/x86/kvm/svm/nested.c |  4 ++++
 arch/x86/kvm/svm/svm.c    | 18 +++++++++++++++---
 arch/x86/kvm/svm/svm.h    |  1 +
 3 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.26.3

[PATCH 1/3] KVM: SVM: #SMI interception must not skip the instruction

[Maxim Levitsky]

Commit 5ff3a351f687 ("KVM: x86: Move trivial instruction-based
exit handlers to common code"), unfortunately made a mistake of
treating nop_on_interception and nop_interception in the same way.

Former does truly nothing while the latter skips the instruction.

SMI VM exit handler should do nothing.
(SMI itself is handled by the host when we do STGI)

Fixes: 5ff3a351f687 ("KVM: x86: Move trivial instruction-based exit handlers to common code")
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/svm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 616b9679ddcc..41d0a589c578 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2127,6 +2127,11 @@ static int nmi_interception(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static int smi_interception(struct kvm_vcpu *vcpu)
+{
+	return 1;
+}
+
 static int intr_interception(struct kvm_vcpu *vcpu)
 {
 	++vcpu->stat.irq_exits;
@@ -3101,7 +3106,7 @@ static int (*const svm_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 	[SVM_EXIT_EXCP_BASE + GP_VECTOR]	= gp_interception,
 	[SVM_EXIT_INTR]				= intr_interception,
 	[SVM_EXIT_NMI]				= nmi_interception,
-	[SVM_EXIT_SMI]				= kvm_emulate_as_nop,
+	[SVM_EXIT_SMI]				= smi_interception,
 	[SVM_EXIT_INIT]				= kvm_emulate_as_nop,
 	[SVM_EXIT_VINTR]			= interrupt_window_interception,
 	[SVM_EXIT_RDPMC]			= kvm_emulate_rdpmc,
-- 
2.26.3

[PATCH 2/3] KVM: SVM: remove INIT intercept handler

[Maxim Levitsky]

Kernel never sends real INIT even to CPUs, other than on boot.

Thus INIT interception is an error which should be caught
by a check for an unknown VMexit reason.

On top of that, the current INIT VM exit handler skips
the current instruction which is wrong.
That was added in commit 5ff3a351f687 ("KVM: x86: Move trivial
instruction-based exit handlers to common code").

Fixes: 5ff3a351f687 ("KVM: x86: Move trivial instruction-based exit handlers to common code")
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/svm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 41d0a589c578..a3aad97fa427 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3107,7 +3107,6 @@ static int (*const svm_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 	[SVM_EXIT_INTR]				= intr_interception,
 	[SVM_EXIT_NMI]				= nmi_interception,
 	[SVM_EXIT_SMI]				= smi_interception,
-	[SVM_EXIT_INIT]				= kvm_emulate_as_nop,
 	[SVM_EXIT_VINTR]			= interrupt_window_interception,
 	[SVM_EXIT_RDPMC]			= kvm_emulate_rdpmc,
 	[SVM_EXIT_CPUID]			= kvm_emulate_cpuid,
-- 
2.26.3

[PATCH 3/3] KVM: SVM: add module param to control the #SMI interception

[Maxim Levitsky]

In theory there are no side effects of not intercepting #SMI,
because then #SMI becomes transparent to the OS and the KVM.

Plus an observation on recent Zen2 CPUs reveals that these
CPUs ignore #SMI interception and never deliver #SMI VMexits.

This is also useful to test nested KVM to see that L1
handles #SMIs correctly in case when L1 doesn't intercept #SMI.

Finally the default remains the same, the SMI are intercepted
by default thus this patch doesn't have any effect unless
non default module param value is used.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/nested.c |  4 ++++
 arch/x86/kvm/svm/svm.c    | 10 +++++++++-
 arch/x86/kvm/svm/svm.h    |  1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 21d03e3a5dfd..2884c54a72bb 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -154,6 +154,10 @@ void recalc_intercepts(struct vcpu_svm *svm)
 
 	for (i = 0; i < MAX_INTERCEPT; i++)
 		c->intercepts[i] |= g->intercepts[i];
+
+	/* If SMI is not intercepted, ignore guest SMI intercept as well  */
+	if (!intercept_smi)
+		vmcb_clr_intercept(c, INTERCEPT_SMI);
 }
 
 static void copy_vmcb_control_area(struct vmcb_control_area *dst,
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index a3aad97fa427..9f95e77d5ce1 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -198,6 +198,11 @@ module_param(avic, bool, 0444);
 bool __read_mostly dump_invalid_vmcb;
 module_param(dump_invalid_vmcb, bool, 0644);
 
+
+bool intercept_smi = true;
+module_param(intercept_smi, bool, 0444);
+
+
 static bool svm_gp_erratum_intercept = true;
 
 static u8 rsm_ins_bytes[] = "\x0f\xaa";
@@ -1206,7 +1211,10 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
 
 	svm_set_intercept(svm, INTERCEPT_INTR);
 	svm_set_intercept(svm, INTERCEPT_NMI);
-	svm_set_intercept(svm, INTERCEPT_SMI);
+
+	if (intercept_smi)
+		svm_set_intercept(svm, INTERCEPT_SMI);
+
 	svm_set_intercept(svm, INTERCEPT_SELECTIVE_CR0);
 	svm_set_intercept(svm, INTERCEPT_RDPMC);
 	svm_set_intercept(svm, INTERCEPT_CPUID);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index f89b623bb591..8cb3bd59c5ea 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -31,6 +31,7 @@
 #define MSRPM_OFFSETS	16
 extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
 extern bool npt_enabled;
+extern bool intercept_smi;
 
 /*
  * Clean bits in VMCB.
-- 
2.26.3

Re: [PATCH 0/3] KVM: SMM fixes

[Paolo Bonzini]

On 07/07/21 14:50, Maxim Levitsky wrote:
> Hi!
> 
> I did first round of testing of SMM by flooding the guest with SMIs,
> and running nested guests in it, and I found out that SMM
> breaks nested KVM due to a refactoring change
> that was done in 5.12 kernel. Fix for this is in patch 1.
> 
> I also fixed another issue I noticed in this patch which is purely
> theoretical but nevertheless should be fixed. This is patch 2.
> 
> I also propose to add (mostly for debug for now) a module param
> that can make the KVM to avoid intercepting #SMIs on SVM.
> (Intel doesn't have such intercept I think)
> The default is still to intercept #SMI so nothing is changed by
> default.
> 
> This allows to test the case in which SMI are not intercepted,
> by L1 without running Windows (which doesn't intercept #SMI).
> 
> In addition to that I found out that on bare metal, at least
> on two Zen2 machines I have, the CPU ignores SMI interception and
> never VM exits when SMI is received. As I guessed earlier
> this must have been done for security reasons.
> 
> Note that bug that I fixed in patch 1, should crash VMs very soon
> on bare metal as well, if the CPU were to honour the SMI intercept.
> as long as there are some SMIs generated while the system is running.
> 
> I tested this on bare metal by using local APIC to send SMIs
> to all real CPUs, and also used ioport 0xB2 to send SMIs.
> In both cases my system slowed to a crawl but didn't show
> any SMI vmexits (SMI intercept was enabled).
> 
> In a VM I also used ioport 0xB2 to generate a flood of SMIs,
> which allowed me to reproduce this bug (and with intercept_smi=0
> module parameter I can reproduce the bug that Vitaly fixed in
> his series as well while just running nested KVM).
> 
> Note that while doing nested migration I am still able to cause
> severe hangs of the L1 when I run the SMI stress test in L1
> and a nested VM. VM isn't fully hung but its GUI stops responding,
> and I see lots of cpu lockups errors in dmesg.
> This seems to happen regardless of #SMI interception in the L1
> (with Vitaly's patches applied of course)
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (3):
>    KVM: SVM: #SMI interception must not skip the instruction
>    KVM: SVM: remove INIT intercept handler
>    KVM: SVM: add module param to control the #SMI interception
> 
>   arch/x86/kvm/svm/nested.c |  4 ++++
>   arch/x86/kvm/svm/svm.c    | 18 +++++++++++++++---
>   arch/x86/kvm/svm/svm.h    |  1 +
>   3 files changed, 20 insertions(+), 3 deletions(-)
> 

Queued, thanks.

Paolo