Fix a NULL pointer dereference in gfn_to_rmap() that occurs if RSM fails,
reported by syzbot.
The immediate problem is that the MMU context's role gets out of sync
because KVM clears the SMM flag in the vCPU at the start of RSM emulation,
but only resets the MMU context if RSM succeeds. The divergence in vCPU
vs. MMU role with respect to the SMM flag causes explosions if the non-SMM
memslots have gfn ranges that are not present in the SMM memslots, because
the MMU expects that the memslot for a shadow page cannot magically
disappear.
The other obvious problem is that KVM doesn't emulate triple fault on RSM
failure, e.g. it keeps running the vCPU in a frankenstate instead of
exiting to userspace. Fixing that would squash the syzbot repro, but
would not fix the underlying issue because nothing prevents userspace from
calling KVM_RUN on a vCPU that hit shutdown (yay lack of a shutdown state).
But, it's easy to fix and definitely worth doing.
Everything after the two bug fixes is cleanup.
Ben Gardon has an internal patch or two that guards against the NULL
pointer dereference in gfn_to_rmap(). I'm planning on getting that
functionality posted (needs a little massaging) so that these types of
snafus don't crash the host (this isn't the first time I've introduced a
bug that broke gfn_to_rmap(), though thankfully it's the first time such
a bug has made it upstream, knock on wood).
Amusingly, adding gfn_to_rmap() NULL memslot checks might even be a
performance improvement. Because gfn_to_rmap() doesn't check the memslot
before using it, and because the compiler can see the search_memslots()
returns NULL/0, gcc often/always generates dedicated (and hilarious) code
for NULL, e.g. this #GP was caused by an explicit load from 0:
48 8b 14 25 00 00 00 00 mov 0x0,%rdx
Sean Christopherson (9):
KVM: x86: Immediately reset the MMU context when the SMM flag is
cleared
KVM: x86: Emulate triple fault shutdown if RSM emulation fails
KVM: x86: Replace .set_hflags() with dedicated .exiting_smm() helper
KVM: x86: Invoke kvm_smm_changed() immediately after clearing SMM flag
KVM: x86: Move (most) SMM hflags modifications into kvm_smm_changed()
KVM: x86: Move "entering SMM" tracepoint into kvm_smm_changed()
KVM: x86: Rename SMM tracepoint to make it reflect reality
KVM: x86: Drop .post_leave_smm(), i.e. the manual post-RSM MMU reset
KVM: x86: Drop "pre_" from enter/leave_smm() helpers
arch/x86/include/asm/kvm-x86-ops.h | 4 +--
arch/x86/include/asm/kvm_host.h | 4 +--
arch/x86/kvm/emulate.c | 31 ++++++++++-------
arch/x86/kvm/kvm_emulate.h | 7 ++--
arch/x86/kvm/svm/svm.c | 8 ++---
arch/x86/kvm/trace.h | 2 +-
arch/x86/kvm/vmx/vmx.c | 8 ++---
arch/x86/kvm/x86.c | 53 +++++++++++++++---------------
8 files changed, 61 insertions(+), 56 deletions(-)
--
2.32.0.rc1.229.g3e70b5a671-goog
Use the recently introduced KVM_REQ_TRIPLE_FAULT to properly emulate
shutdown if RSM from SMM fails.
Note, entering shutdown after clearing the SMM flag and restoring NMI
blocking is architecturally correct with respect to AMD's APM, which KVM
also uses for SMRAM layout and RSM NMI blocking behavior. The APM says:
An RSM causes a processor shutdown if an invalid-state condition is
found in the SMRAM state-save area. Only an external reset, external
processor-initialization, or non-maskable external interrupt (NMI) can
cause the processor to leave the shutdown state.
Of note is processor-initialization (INIT) as a valid shutdown wake
event, as INIT is blocked by SMM, implying that entering shutdown also
forces the CPU out of SMM.
For recent Intel CPUs, restoring NMI blocking is technically wrong, but
so is restoring NMI blocking in the first place, and Intel's RSM
"architecture" is such a mess that just about anything is allowed and can
be justified as micro-architectural behavior.
Per the SDM:
On Pentium 4 and later processors, shutdown will inhibit INTR and A20M
but will not change any of the other inhibits. On these processors,
NMIs will be inhibited if no action is taken in the SMI handler to
uninhibit them (see Section 34.8).
where Section 34.8 says:
When the processor enters SMM while executing an NMI handler, the
processor saves the SMRAM state save map but does not save the
attribute to keep NMI interrupts disabled. Potentially, an NMI could be
latched (while in SMM or upon exit) and serviced upon exit of SMM even
though the previous NMI handler has still not completed.
I.e. RSM unconditionally unblocks NMI, but shutdown on RSM does not,
which is in direct contradiction of KVM's behavior. But, as mentioned
above, KVM follows AMD architecture and restores NMI blocking on RSM, so
that micro-architectural detail is already lost.
And for Pentium era CPUs, SMI# can break shutdown, meaning that at least
some Intel CPUs fully leave SMM when entering shutdown:
In the shutdown state, Intel processors stop executing instructions
until a RESET#, INIT# or NMI# is asserted. While Pentium family
processors recognize the SMI# signal in shutdown state, P6 family and
Intel486 processors do not.
In other words, the fact that Intel CPUs have implemented the two
extremes gives KVM carte blanche when it comes to honoring Intel's
architecture for handling shutdown during RSM.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/emulate.c | 12 +++++++-----
arch/x86/kvm/kvm_emulate.h | 1 +
arch/x86/kvm/x86.c | 6 ++++++
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 5e5de05a8fbf..0603a2c79093 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2683,7 +2683,7 @@ static int em_rsm(struct x86_emulate_ctxt *ctxt)
* state-save area.
*/
if (ctxt->ops->pre_leave_smm(ctxt, buf))
- return X86EMUL_UNHANDLEABLE;
+ goto emulate_shutdown;
#ifdef CONFIG_X86_64
if (emulator_has_longmode(ctxt))
@@ -2692,14 +2692,16 @@ static int em_rsm(struct x86_emulate_ctxt *ctxt)
#endif
ret = rsm_load_state_32(ctxt, buf);
- if (ret != X86EMUL_CONTINUE) {
- /* FIXME: should triple fault */
- return X86EMUL_UNHANDLEABLE;
- }
+ if (ret != X86EMUL_CONTINUE)
+ goto emulate_shutdown;
ctxt->ops->post_leave_smm(ctxt);
return X86EMUL_CONTINUE;
+
+emulate_shutdown:
+ ctxt->ops->triple_fault(ctxt);
+ return X86EMUL_UNHANDLEABLE;
}
static void
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 3e870bf9ca4d..9c34aa60e45f 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -233,6 +233,7 @@ struct x86_emulate_ops {
int (*pre_leave_smm)(struct x86_emulate_ctxt *ctxt,
const char *smstate);
void (*post_leave_smm)(struct x86_emulate_ctxt *ctxt);
+ void (*triple_fault)(struct x86_emulate_ctxt *ctxt);
int (*set_xcr)(struct x86_emulate_ctxt *ctxt, u32 index, u64 xcr);
};
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 54d212fe9b15..cda148cf06fa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7123,6 +7123,11 @@ static void emulator_post_leave_smm(struct x86_emulate_ctxt *ctxt)
kvm_smm_changed(emul_to_vcpu(ctxt));
}
+static void emulator_triple_fault(struct x86_emulate_ctxt *ctxt)
+{
+ kvm_make_request(KVM_REQ_TRIPLE_FAULT, emul_to_vcpu(ctxt));
+}
+
static int emulator_set_xcr(struct x86_emulate_ctxt *ctxt, u32 index, u64 xcr)
{
return __kvm_set_xcr(emul_to_vcpu(ctxt), index, xcr);
@@ -7172,6 +7177,7 @@ static const struct x86_emulate_ops emulate_ops = {
.set_hflags = emulator_set_hflags,
.pre_leave_smm = emulator_pre_leave_smm,
.post_leave_smm = emulator_post_leave_smm,
+ .triple_fault = emulator_triple_fault,
.set_xcr = emulator_set_xcr,
};
--
2.32.0.rc1.229.g3e70b5a671-goog
Move the core of SMM hflags modifications into kvm_smm_changed() and use
kvm_smm_changed() in enter_smm(). Clear HF_SMM_INSIDE_NMI_MASK for
leaving SMM but do not set it for entering SMM. If the vCPU is executing
outside of SMM, the flag should unequivocally be cleared, e.g. this
technically fixes a benign bug where the flag could be left set after
KVM_SET_VCPU_EVENTS, but the reverse is not true as NMI blocking depends
on pre-SMM state or userspace input.
Note, this adds an extra kvm_mmu_reset_context() to enter_smm(). The
extra/early reset isn't strictly necessary, and in a way can never be
necessary since the vCPU/MMU context is in a half-baked state until the
final context reset at the end of the function. But, enter_smm() is not
a hot path, and exploding on an invalid root_hpa is probably better than
having a stale SMM flag in the MMU role; it's at least no worse.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/x86.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 76ba28865824..13a33c962657 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4457,7 +4457,7 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
memset(&events->reserved, 0, sizeof(events->reserved));
}
-static void kvm_smm_changed(struct kvm_vcpu *vcpu);
+static void kvm_smm_changed(struct kvm_vcpu *vcpu, bool entering_smm);
static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
struct kvm_vcpu_events *events)
@@ -4517,13 +4517,8 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
vcpu->arch.apic->sipi_vector = events->sipi_vector;
if (events->flags & KVM_VCPUEVENT_VALID_SMM) {
- if (!!(vcpu->arch.hflags & HF_SMM_MASK) != events->smi.smm) {
- if (events->smi.smm)
- vcpu->arch.hflags |= HF_SMM_MASK;
- else
- vcpu->arch.hflags &= ~HF_SMM_MASK;
- kvm_smm_changed(vcpu);
- }
+ if (!!(vcpu->arch.hflags & HF_SMM_MASK) != events->smi.smm)
+ kvm_smm_changed(vcpu, events->smi.smm);
vcpu->arch.smi_pending = events->smi.pending;
@@ -7108,8 +7103,7 @@ static void emulator_exiting_smm(struct x86_emulate_ctxt *ctxt)
{
struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
- vcpu->arch.hflags &= ~(HF_SMM_MASK | HF_SMM_INSIDE_NMI_MASK);
- kvm_smm_changed(vcpu);
+ kvm_smm_changed(vcpu, false);
}
static int emulator_pre_leave_smm(struct x86_emulate_ctxt *ctxt,
@@ -7438,9 +7432,13 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,
static int complete_emulated_mmio(struct kvm_vcpu *vcpu);
static int complete_emulated_pio(struct kvm_vcpu *vcpu);
-static void kvm_smm_changed(struct kvm_vcpu *vcpu)
+static void kvm_smm_changed(struct kvm_vcpu *vcpu, bool entering_smm)
{
- if (!(vcpu->arch.hflags & HF_SMM_MASK)) {
+ if (entering_smm) {
+ vcpu->arch.hflags |= HF_SMM_MASK;
+ } else {
+ vcpu->arch.hflags &= ~(HF_SMM_MASK | HF_SMM_INSIDE_NMI_MASK);
+
/* This is a good place to trace that we are exiting SMM. */
trace_kvm_enter_smm(vcpu->vcpu_id, vcpu->arch.smbase, false);
@@ -8912,7 +8910,7 @@ static void enter_smm(struct kvm_vcpu *vcpu)
*/
static_call(kvm_x86_pre_enter_smm)(vcpu, buf);
- vcpu->arch.hflags |= HF_SMM_MASK;
+ kvm_smm_changed(vcpu, true);
kvm_vcpu_write_guest(vcpu, vcpu->arch.smbase + 0xfe00, buf, sizeof(buf));
if (static_call(kvm_x86_get_nmi_mask)(vcpu))
--
2.32.0.rc1.229.g3e70b5a671-goog
Invoke the "entering SMM" tracepoint from kvm_smm_changed() instead of
enter_smm(), effectively moving it from before reading vCPU state to
after reading state (but still before writing it to SMRAM!). The primary
motivation is to consolidate code, but calling the tracepoint from
kvm_smm_changed() also makes its invocation consistent with respect to
SMI and RSM, and with respect to KVM_SET_VCPU_EVENTS (which previously
only invoked the tracepoint when forcing the vCPU out of SMM).
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/x86.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 13a33c962657..11ea81c8cb82 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7434,14 +7434,13 @@ static int complete_emulated_pio(struct kvm_vcpu *vcpu);
static void kvm_smm_changed(struct kvm_vcpu *vcpu, bool entering_smm)
{
+ trace_kvm_enter_smm(vcpu->vcpu_id, vcpu->arch.smbase, entering_smm);
+
if (entering_smm) {
vcpu->arch.hflags |= HF_SMM_MASK;
} else {
vcpu->arch.hflags &= ~(HF_SMM_MASK | HF_SMM_INSIDE_NMI_MASK);
- /* This is a good place to trace that we are exiting SMM. */
- trace_kvm_enter_smm(vcpu->vcpu_id, vcpu->arch.smbase, false);
-
/* Process a latched INIT or SMI, if any. */
kvm_make_request(KVM_REQ_EVENT, vcpu);
}
@@ -8894,7 +8893,6 @@ static void enter_smm(struct kvm_vcpu *vcpu)
char buf[512];
u32 cr0;
- trace_kvm_enter_smm(vcpu->vcpu_id, vcpu->arch.smbase, true);
memset(buf, 0, 512);
#ifdef CONFIG_X86_64
if (guest_cpuid_has(vcpu, X86_FEATURE_LM))
--
2.32.0.rc1.229.g3e70b5a671-goog
Sean Christopherson <[email protected]> writes:
> Use the recently introduced KVM_REQ_TRIPLE_FAULT to properly emulate
> shutdown if RSM from SMM fails.
>
> Note, entering shutdown after clearing the SMM flag and restoring NMI
> blocking is architecturally correct with respect to AMD's APM, which KVM
> also uses for SMRAM layout and RSM NMI blocking behavior. The APM says:
>
> An RSM causes a processor shutdown if an invalid-state condition is
> found in the SMRAM state-save area. Only an external reset, external
> processor-initialization, or non-maskable external interrupt (NMI) can
> cause the processor to leave the shutdown state.
>
> Of note is processor-initialization (INIT) as a valid shutdown wake
> event, as INIT is blocked by SMM, implying that entering shutdown also
> forces the CPU out of SMM.
>
> For recent Intel CPUs, restoring NMI blocking is technically wrong, but
> so is restoring NMI blocking in the first place, and Intel's RSM
> "architecture" is such a mess that just about anything is allowed and can
> be justified as micro-architectural behavior.
>
> Per the SDM:
>
> On Pentium 4 and later processors, shutdown will inhibit INTR and A20M
> but will not change any of the other inhibits. On these processors,
> NMIs will be inhibited if no action is taken in the SMI handler to
> uninhibit them (see Section 34.8).
>
> where Section 34.8 says:
>
> When the processor enters SMM while executing an NMI handler, the
> processor saves the SMRAM state save map but does not save the
> attribute to keep NMI interrupts disabled. Potentially, an NMI could be
> latched (while in SMM or upon exit) and serviced upon exit of SMM even
> though the previous NMI handler has still not completed.
>
> I.e. RSM unconditionally unblocks NMI, but shutdown on RSM does not,
> which is in direct contradiction of KVM's behavior. But, as mentioned
> above, KVM follows AMD architecture and restores NMI blocking on RSM, so
> that micro-architectural detail is already lost.
>
> And for Pentium era CPUs, SMI# can break shutdown, meaning that at least
> some Intel CPUs fully leave SMM when entering shutdown:
>
> In the shutdown state, Intel processors stop executing instructions
> until a RESET#, INIT# or NMI# is asserted. While Pentium family
> processors recognize the SMI# signal in shutdown state, P6 family and
> Intel486 processors do not.
>
> In other words, the fact that Intel CPUs have implemented the two
> extremes gives KVM carte blanche when it comes to honoring Intel's
> architecture for handling shutdown during RSM.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/emulate.c | 12 +++++++-----
> arch/x86/kvm/kvm_emulate.h | 1 +
> arch/x86/kvm/x86.c | 6 ++++++
> 3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 5e5de05a8fbf..0603a2c79093 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2683,7 +2683,7 @@ static int em_rsm(struct x86_emulate_ctxt *ctxt)
> * state-save area.
> */
> if (ctxt->ops->pre_leave_smm(ctxt, buf))
> - return X86EMUL_UNHANDLEABLE;
> + goto emulate_shutdown;
>
> #ifdef CONFIG_X86_64
> if (emulator_has_longmode(ctxt))
> @@ -2692,14 +2692,16 @@ static int em_rsm(struct x86_emulate_ctxt *ctxt)
> #endif
> ret = rsm_load_state_32(ctxt, buf);
>
> - if (ret != X86EMUL_CONTINUE) {
> - /* FIXME: should triple fault */
> - return X86EMUL_UNHANDLEABLE;
> - }
> + if (ret != X86EMUL_CONTINUE)
> + goto emulate_shutdown;
>
> ctxt->ops->post_leave_smm(ctxt);
>
> return X86EMUL_CONTINUE;
> +
> +emulate_shutdown:
> + ctxt->ops->triple_fault(ctxt);
> + return X86EMUL_UNHANDLEABLE;
I'm probably missing something, but what's the desired effect of both
raising KVM_REQ_TRIPLE_FAULT and returning X86EMUL_UNHANDLEABLE here?
I've modified smm selftest to see what's happening:
diff --git a/tools/testing/selftests/kvm/x86_64/smm_test.c b/tools/testing/selftests/kvm/x86_64/smm_test.c
index 613c42c5a9b8..cf215cd2c6e2 100644
--- a/tools/testing/selftests/kvm/x86_64/smm_test.c
+++ b/tools/testing/selftests/kvm/x86_64/smm_test.c
@@ -147,6 +147,11 @@ int main(int argc, char *argv[])
"Unexpected stage: #%x, got %x",
stage, stage_reported);
+ if (stage_reported == SMRAM_STAGE) {
+ /* corrupt smram */
+ memset(addr_gpa2hva(vm, SMRAM_GPA) + 0xfe00, 0xff, 512);
+ }
+
state = vcpu_save_state(vm, VCPU_ID);
kvm_vm_release(vm);
kvm_vm_restart(vm, O_RDWR);
What I see is:
smm_test-7600 [002] 4497.073918: kvm_exit: reason EXIT_RSM rip 0x8004 info 0 0
smm_test-7600 [002] 4497.073921: kvm_emulate_insn: 1000000:8004: 0f aa
smm_test-7600 [002] 4497.073924: kvm_smm_transition: vcpu 1: leaving SMM, smbase 0x1000000
smm_test-7600 [002] 4497.073928: kvm_emulate_insn: 0:8004: 0f aa FAIL
smm_test-7600 [002] 4497.073929: kvm_fpu: unload
smm_test-7600 [002] 4497.073930: kvm_userspace_exit: reason KVM_EXIT_INTERNAL_ERROR (17)
If I change X86EMUL_UNHANDLEABLE to X86EMUL_CONTINUE tripple fault is
happening indeed (why don't we have triple fault printed in trace by
default BTW???):
smm_test-16810 [006] 5117.007220: kvm_exit: reason EXIT_RSM rip 0x8004 info 0 0
smm_test-16810 [006] 5117.007222: kvm_emulate_insn: 1000000:8004: 0f aa
smm_test-16810 [006] 5117.007225: kvm_smm_transition: vcpu 1: leaving SMM, smbase 0x1000000
smm_test-16810 [006] 5117.007229: bputs: vcpu_enter_guest: KVM_REQ_TRIPLE_FAULT
smm_test-16810 [006] 5117.007230: kvm_fpu: unload
smm_test-16810 [006] 5117.007230: kvm_userspace_exit: reason KVM_EXIT_SHUTDOWN (8)
So should we actually have X86EMUL_CONTINUE when we queue
KVM_REQ_TRIPLE_FAULT here?
(Initially, my comment was supposed to be 'why don't you add
TRIPLE_FAULT to smm selftest?' but the above overshadows it)
> }
>
> static void
> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> index 3e870bf9ca4d..9c34aa60e45f 100644
> --- a/arch/x86/kvm/kvm_emulate.h
> +++ b/arch/x86/kvm/kvm_emulate.h
> @@ -233,6 +233,7 @@ struct x86_emulate_ops {
> int (*pre_leave_smm)(struct x86_emulate_ctxt *ctxt,
> const char *smstate);
> void (*post_leave_smm)(struct x86_emulate_ctxt *ctxt);
> + void (*triple_fault)(struct x86_emulate_ctxt *ctxt);
> int (*set_xcr)(struct x86_emulate_ctxt *ctxt, u32 index, u64 xcr);
> };
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 54d212fe9b15..cda148cf06fa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7123,6 +7123,11 @@ static void emulator_post_leave_smm(struct x86_emulate_ctxt *ctxt)
> kvm_smm_changed(emul_to_vcpu(ctxt));
> }
>
> +static void emulator_triple_fault(struct x86_emulate_ctxt *ctxt)
> +{
> + kvm_make_request(KVM_REQ_TRIPLE_FAULT, emul_to_vcpu(ctxt));
> +}
> +
> static int emulator_set_xcr(struct x86_emulate_ctxt *ctxt, u32 index, u64 xcr)
> {
> return __kvm_set_xcr(emul_to_vcpu(ctxt), index, xcr);
> @@ -7172,6 +7177,7 @@ static const struct x86_emulate_ops emulate_ops = {
> .set_hflags = emulator_set_hflags,
> .pre_leave_smm = emulator_pre_leave_smm,
> .post_leave_smm = emulator_post_leave_smm,
> + .triple_fault = emulator_triple_fault,
> .set_xcr = emulator_set_xcr,
> };
--
Vitaly
On 09/06/21 20:56, Sean Christopherson wrote:
> For recent Intel CPUs, restoring NMI blocking is technically wrong, but
> so is restoring NMI blocking in the first place, and Intel's RSM
> "architecture" is such a mess that just about anything is allowed and can
> be justified as micro-architectural behavior.
The Intel manual is an absolute mess with respect to NMI blocking, and
for once AMD followed suit.
Some versions of the AMD BIOS and Kernel Developer Manual provide the
offset of the "NMI masked" flag in the SMM state save area, but
unfortunately that was discovered too late.
Paolo
On 10/06/21 10:26, Vitaly Kuznetsov wrote:
> So should we actually have X86EMUL_CONTINUE when we queue
> KVM_REQ_TRIPLE_FAULT here?
Yes...
> (Initially, my comment was supposed to be 'why don't you add
> TRIPLE_FAULT to smm selftest?' but the above overshadows it)
... and a tenth patch to add a selftest would be nice to have indeed.
Paolo
On 09/06/21 20:56, Sean Christopherson wrote:
> Fix a NULL pointer dereference in gfn_to_rmap() that occurs if RSM fails,
> reported by syzbot.
>
> The immediate problem is that the MMU context's role gets out of sync
> because KVM clears the SMM flag in the vCPU at the start of RSM emulation,
> but only resets the MMU context if RSM succeeds. The divergence in vCPU
> vs. MMU role with respect to the SMM flag causes explosions if the non-SMM
> memslots have gfn ranges that are not present in the SMM memslots, because
> the MMU expects that the memslot for a shadow page cannot magically
> disappear.
>
> The other obvious problem is that KVM doesn't emulate triple fault on RSM
> failure, e.g. it keeps running the vCPU in a frankenstate instead of
> exiting to userspace. Fixing that would squash the syzbot repro, but
> would not fix the underlying issue because nothing prevents userspace from
> calling KVM_RUN on a vCPU that hit shutdown (yay lack of a shutdown state).
> But, it's easy to fix and definitely worth doing.
>
> Everything after the two bug fixes is cleanup.
>
> Ben Gardon has an internal patch or two that guards against the NULL
> pointer dereference in gfn_to_rmap(). I'm planning on getting that
> functionality posted (needs a little massaging) so that these types of
> snafus don't crash the host (this isn't the first time I've introduced a
> bug that broke gfn_to_rmap(), though thankfully it's the first time such
> a bug has made it upstream, knock on wood).
>
> Amusingly, adding gfn_to_rmap() NULL memslot checks might even be a
> performance improvement. Because gfn_to_rmap() doesn't check the memslot
> before using it, and because the compiler can see the search_memslots()
> returns NULL/0, gcc often/always generates dedicated (and hilarious) code
> for NULL, e.g. this #GP was caused by an explicit load from 0:
>
> 48 8b 14 25 00 00 00 00 mov 0x0,%rdx
>
>
> Sean Christopherson (9):
> KVM: x86: Immediately reset the MMU context when the SMM flag is
> cleared
> KVM: x86: Emulate triple fault shutdown if RSM emulation fails
> KVM: x86: Replace .set_hflags() with dedicated .exiting_smm() helper
> KVM: x86: Invoke kvm_smm_changed() immediately after clearing SMM flag
> KVM: x86: Move (most) SMM hflags modifications into kvm_smm_changed()
> KVM: x86: Move "entering SMM" tracepoint into kvm_smm_changed()
> KVM: x86: Rename SMM tracepoint to make it reflect reality
> KVM: x86: Drop .post_leave_smm(), i.e. the manual post-RSM MMU reset
> KVM: x86: Drop "pre_" from enter/leave_smm() helpers
>
> arch/x86/include/asm/kvm-x86-ops.h | 4 +--
> arch/x86/include/asm/kvm_host.h | 4 +--
> arch/x86/kvm/emulate.c | 31 ++++++++++-------
> arch/x86/kvm/kvm_emulate.h | 7 ++--
> arch/x86/kvm/svm/svm.c | 8 ++---
> arch/x86/kvm/trace.h | 2 +-
> arch/x86/kvm/vmx/vmx.c | 8 ++---
> arch/x86/kvm/x86.c | 53 +++++++++++++++---------------
> 8 files changed, 61 insertions(+), 56 deletions(-)
>
Queued 2-9 too for 5.14, with Vitaly's suggested change for patch 2.
Paolo
On Thu, Jun 10, 2021, Paolo Bonzini wrote:
> On 10/06/21 10:26, Vitaly Kuznetsov wrote:
> > So should we actually have X86EMUL_CONTINUE when we queue
> > KVM_REQ_TRIPLE_FAULT here?
>
> Yes...
>
> > (Initially, my comment was supposed to be 'why don't you add
> > TRIPLE_FAULT to smm selftest?' but the above overshadows it)
>
> ... and a tenth patch to add a selftest would be nice to have indeed.
Yes, I've been remiss in writing/modifying tests, I'll prioritize that once I've
dug myself out of the rabbit holes I wandered into via the vCPU RESET/INIT series.
On Thu, Jun 10, 2021, Vitaly Kuznetsov wrote:
> why don't we have triple fault printed in trace by default BTW???
Or maybe trace_kvm_make_request()? Not sure how much pain that would inflict on
the binaries though.
Sean Christopherson <[email protected]> writes:
> On Thu, Jun 10, 2021, Vitaly Kuznetsov wrote:
>> why don't we have triple fault printed in trace by default BTW???
>
> Or maybe trace_kvm_make_request()?
... or maybe just trace_kvm_check_request()?
$ git grep 'kvm_make_request(' arch/x86/kvm/ | wc -l
115
$ git grep 'kvm_check_request(' arch/x86/kvm/ | wc -l
41
--
Vitaly