kvm: nVMX: flush TLB when decoded insn != VM-exit reason
PATCH 1/1 kvm: nVMX: flush TLB when decoded insn != VM-exit reason
It is possible for the instruction emulator to decode a different instruction from what was implied by the VM-exit information provided by hardware in vmcs02. Such is the case when the TLB entry for the guest's IP is out of sync with the appropriate page-table mapping if page installation isn't followed with a TLB flush.
Currently, KVM refuses to emulate in these scenarios, instead injecting a #UD into L2. While this does address the security risk of CVE-2020-2732, it could result in spurious #UDs to the L2 guest. Fix this by instead flushing the TLB then resuming L2, allowing hardware to generate the appropriate VM-exit to be reflected into L1.
Exceptional handling is also required for RSM and RDTSCP instructions. RDTSCP could be emulated on hardware which doesn't support it, therefore hardware will not generate a RDTSCP VM-exit on L2 resume. The dual-monitor treatment of SMM is not supported in nVMX, which implies that L0 should never handle a RSM instruction. Resuming the guest will only result in another #UD. Avoid getting stuck in a loop with these instructions by injecting a #UD for RSM and the appropriate VM-exit for RDTSCP.
--- arch/x86/kvm/emulate.c | 2 ++ arch/x86/kvm/kvm_emulate.h | 1 + arch/x86/kvm/vmx/vmx.c | 68 ++++++++++++++++++++++++++++---------- arch/x86/kvm/x86.c | 2 +- 4 files changed, 55 insertions(+), 18 deletions(-)
arch/x86/kvm/emulate.c +2 -0
@@ -5812,6 +5812,8 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt) }if (rc ==X86EMUL_INTERCEPTED )return EMULATION_INTERCEPTED ; +if (rc ==X86EMUL_RETRY_INSTR ) +return EMULATION_RETRY_INSTR ;if (rc ==X86EMUL_CONTINUE )writeback_registers (ctxt);
arch/x86/kvm/kvm_emulate.h +1 -0
@@ -496,6 +496,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt);#define EMULATION_OK 0#define EMULATION_RESTART 1#define EMULATION_INTERCEPTED 2 +#define EMULATION_RETRY_INSTR 3void init_decode_cache (struct x86_emulate_ctxt *ctxt);int x86_emulate_insn (struct x86_emulate_ctxt *ctxt); int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
arch/x86/kvm/vmx/vmx.c +51 -17
113 lines not on this page · raw message
arch/x86/kvm/x86.c +1 -1
@@ -6967,7 +6967,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, r =x86_emulate_insn (ctxt); -if (r ==EMULATION_INTERCEPTED ) +if (r ==EMULATION_INTERCEPTED || r ==EMULATION_RETRY_INSTR )return 1 ;if (r ==EMULATION_FAILED ) {
Signature · 1 line
2.27.0.290.gba653c62da-goog
Quoted 196 lines · Oliver Upton
It is possible for the instruction emulator to decode a different instruction from what was implied by the VM-exit information provided by hardware in vmcs02. Such is the case when the TLB entry for the guest's IP is out of sync with the appropriate page-table mapping if page installation isn't followed with a TLB flush.
Currently, KVM refuses to emulate in these scenarios, instead injecting a #UD into L2. While this does address the security risk of CVE-2020-2732, it could result in spurious #UDs to the L2 guest. Fix this by instead flushing the TLB then resuming L2, allowing hardware to generate the appropriate VM-exit to be reflected into L1.
Exceptional handling is also required for RSM and RDTSCP instructions. RDTSCP could be emulated on hardware which doesn't support it, therefore hardware will not generate a RDTSCP VM-exit on L2 resume. The dual-monitor treatment of SMM is not supported in nVMX, which implies that L0 should never handle a RSM instruction. Resuming the guest will only result in another #UD. Avoid getting stuck in a loop with these instructions by injecting a #UD for RSM and the appropriate VM-exit for RDTSCP.
Fixes 07721feee46b ("KVM: nVMX: Don't emulate instructions in guest mode")Cc Paolo Bonzini <[email protected]>Cc Sean Christopherson <[email protected]>Signed-off-by Oliver Upton <[email protected]>Reviewed-by Jim Mattson <[email protected]>Reviewed-by Peter Shier <[email protected]>--- arch/x86/kvm/emulate.c | 2 ++ arch/x86/kvm/kvm_emulate.h | 1 + arch/x86/kvm/vmx/vmx.c | 68 ++++++++++++++++++++++++++++---------- arch/x86/kvm/x86.c | 2 +- 4 files changed, 55 insertions(+), 18 deletions(-)arch/x86/kvm/emulate.c +2
-0@@ -5812,6 +5812,8 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt) }if (rc ==X86EMUL_INTERCEPTED )return EMULATION_INTERCEPTED ; +if (rc ==X86EMUL_RETRY_INSTR ) +return EMULATION_RETRY_INSTR ;if (rc ==X86EMUL_CONTINUE )writeback_registers (ctxt);arch/x86/kvm/kvm_emulate.h +1
-0@@ -496,6 +496,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt);#define EMULATION_OK 0#define EMULATION_RESTART 1#define EMULATION_INTERCEPTED 2 +#define EMULATION_RETRY_INSTR 3void init_decode_cache (struct x86_emulate_ctxt *ctxt);int x86_emulate_insn (struct x86_emulate_ctxt *ctxt); int emulator_task_switch(struct x86_emulate_ctxt *ctxt,arch/x86/kvm/vmx/vmx.c +51
-17@@ -7329,12 +7329,11 @@ static void vmx_request_immediate_exit(struct kvm_vcpu *vcpu)to_vmx (vcpu)->req_immediate_exit = true; } -static int vmx_check_intercept_io (struct kvm_vcpu *vcpu, -struct x86_instruction_info *info) +static bool vmx_check_intercept_io (struct kvm_vcpu *vcpu, +struct x86_instruction_info *info) {struct vmcs12 *vmcs12 =get_vmcs12 (vcpu);unsigned short port; -bool intercept;int size; if (info->intercept == x86_intercept_in ||@@ -7354,13 +7353,10 @@ static int vmx_check_intercept_io(struct kvm_vcpu *vcpu,* Otherwise, IO instruction VM-exits are controlled by the IO bitmaps. */ if (!nested_cpu_has (vmcs12,CPU_BASED_USE_IO_BITMAPS )) - intercept =nested_cpu_has (vmcs12, -CPU_BASED_UNCOND_IO_EXITING ); -else - intercept =nested_vmx_check_io_bitmaps (vcpu, port, size); +return nested_cpu_has (vmcs12, +CPU_BASED_UNCOND_IO_EXITING ); -/* FIXME: produce nested vmexit and return X86EMUL_INTERCEPTED. */ -return intercept ?X86EMUL_UNHANDLEABLE :X86EMUL_CONTINUE ; +return nested_vmx_check_io_bitmaps (vcpu, port, size); } static int vmx_check_intercept(struct kvm_vcpu *vcpu,@@ -7369,6 +7365,7 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu, struct x86_exception *exception) {struct vmcs12 *vmcs12 =get_vmcs12 (vcpu); +bool intercepted;switch (info->intercept) {/* @@ -7381,13 +7378,27 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu, exception->error_code_valid = false;return X86EMUL_PROPAGATE_FAULT ; } + + intercepted =nested_cpu_has (vmcs12,CPU_BASED_RDTSC_EXITING ); + +/* +* RDTSCP could be emulated on a CPU which doesn't support it. +* As such, flushing the TLB and resuming L2 will result in +* another #UD rather than a VM-exit to reflect into L1. +* Instead, synthesize the VM-exit here. +*/ +if (intercepted) { +nested_vmx_vmexit (vcpu,EXIT_REASON_RDTSCP ,0 ,0 ); +return X86EMUL_INTERCEPTED ; + }break ;case x86_intercept_in:case x86_intercept_ins:case x86_intercept_out:case x86_intercept_outs: -return vmx_check_intercept_io (vcpu, info); + intercepted =vmx_check_intercept_io (vcpu, info); +break ;case x86_intercept_lgdt:case x86_intercept_lidt:@@ -7397,18 +7408,41 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu,case x86_intercept_sidt:case x86_intercept_sldt:case x86_intercept_str: -if (!nested_cpu_has2 (vmcs12,SECONDARY_EXEC_DESC )) -return X86EMUL_CONTINUE ; - -/* FIXME: produce nested vmexit and return X86EMUL_INTERCEPTED. */ + intercepted =nested_cpu_has2 (vmcs12,SECONDARY_EXEC_DESC );break ; -/* TODO: check more intercepts... */ +/* +* The dual-monitor treatment of SMM is not supported in nVMX. As such, +* L0 will never handle the RSM instruction nor should it retry +* instruction execution. Instead, a #UD should be injected into the +* guest for the execution of RSM outside of SMM. +*/ +case x86_intercept_rsm: + exception->vector =UD_VECTOR ; + exception->error_code_valid = false; +return X86EMUL_PROPAGATE_FAULT ; +default : -break ; + intercepted = true; } -return X86EMUL_UNHANDLEABLE ; +if (!intercepted) +return X86EMUL_CONTINUE ; + +/* +* The only uses of the emulator in VMX for instructions which may be +* intercepted are port IO instructions, descriptor-table accesses, and +* the RDTSCP instruction. As such, if the emulator has decoded an +* instruction that is different from the VM-exit provided by hardware +* it is likely that the TLB entry and page-table mapping for the +* guest's RIP are out of sync. +* +* Rather than synthesizing a VM-exit into L1 for every possible +* instruction just flush the TLB, resume L2, and let hardware generate +* the appropriate VM-exit. +*/ +vmx_flush_tlb_gva (vcpu,kvm_rip_read (vcpu)); +return X86EMUL_RETRY_INSTR ; }#ifdef CONFIG_X86_64 arch/x86/kvm/x86.c +1
-2@@ -6967,7 +6967,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, r =x86_emulate_insn (ctxt); -if (r ==EMULATION_INTERCEPTED ) +if (r ==EMULATION_INTERCEPTED || r ==EMULATION_RETRY_INSTR )return 1 ;if (r ==EMULATION_FAILED ) { --
Quoted 2 lines
2.27.0.290.gba653c62da-goog
Ping :)
Paolo Bonzini
maintainerQuoted 7 lines · Oliver Upton
arch/x86/kvm/vmx/vmx.c +1
-4-if (!nested_cpu_has2 (vmcs12,SECONDARY_EXEC_DESC )) -return X86EMUL_CONTINUE ; - -/* FIXME: produce nested vmexit and return X86EMUL_INTERCEPTED. */ + intercepted =nested_cpu_has2 (vmcs12,SECONDARY_EXEC_DESC );break ;
[...]
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7443
Quoted 12 lines
arch/x86/kvm/vmx/vmx.c +12
-0+/* +* The only uses of the emulator in VMX for instructions which may be +* intercepted are port IO instructions, descriptor-table accesses, and +* the RDTSCP instruction. As such, if the emulator has decoded an +* instruction that is different from the VM-exit provided by hardware +* it is likely that the TLB entry and page-table mapping for the +* guest's RIP are out of sync. +* +* Rather than synthesizing a VM-exit into L1 for every possible +* instruction just flush the TLB, resume L2, and let hardware generate +* the appropriate VM-exit. +*/
So you're saying that (in the SECONDARY_EXEC_DESC case above for an example) this should have been handled earlier by nested_vmx_l1_wants_exit. But what about LGDT and friends from an MMIO address? I would say we could just not care, but an infinite #UD loop is an ugly failure mode.
(Or perhaps it's just not my day for reviewing code...).
Paolo
Quoted 82 lines · Oliver Upton
It is possible for the instruction emulator to decode a different instruction from what was implied by the VM-exit information provided by hardware in vmcs02. Such is the case when the TLB entry for the guest's IP is out of sync with the appropriate page-table mapping if page installation isn't followed with a TLB flush.
Currently, KVM refuses to emulate in these scenarios, instead injecting a #UD into L2. While this does address the security risk of CVE-2020-2732, it could result in spurious #UDs to the L2 guest. Fix this by instead flushing the TLB then resuming L2, allowing hardware to generate the appropriate VM-exit to be reflected into L1.
Exceptional handling is also required for RSM and RDTSCP instructions. RDTSCP could be emulated on hardware which doesn't support it, therefore hardware will not generate a RDTSCP VM-exit on L2 resume. The dual-monitor treatment of SMM is not supported in nVMX, which implies that L0 should never handle a RSM instruction. Resuming the guest will only result in another #UD. Avoid getting stuck in a loop with these instructions by injecting a #UD for RSM and the appropriate VM-exit for RDTSCP.
Fixes 07721feee46b ("KVM: nVMX: Don't emulate instructions in guest mode")Cc Paolo Bonzini <[email protected]>Cc Sean Christopherson <[email protected]>Signed-off-by Oliver Upton <[email protected]>Reviewed-by Jim Mattson <[email protected]>Reviewed-by Peter Shier <[email protected]>--- arch/x86/kvm/emulate.c | 2 ++ arch/x86/kvm/kvm_emulate.h | 1 + arch/x86/kvm/vmx/vmx.c | 68 ++++++++++++++++++++++++++++---------- arch/x86/kvm/x86.c | 2 +- 4 files changed, 55 insertions(+), 18 deletions(-)arch/x86/kvm/emulate.c +2
-0@@ -5812,6 +5812,8 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt) }if (rc ==X86EMUL_INTERCEPTED )return EMULATION_INTERCEPTED ; +if (rc ==X86EMUL_RETRY_INSTR ) +return EMULATION_RETRY_INSTR ;if (rc ==X86EMUL_CONTINUE )writeback_registers (ctxt);arch/x86/kvm/kvm_emulate.h +1
-0@@ -496,6 +496,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt);#define EMULATION_OK 0#define EMULATION_RESTART 1#define EMULATION_INTERCEPTED 2 +#define EMULATION_RETRY_INSTR 3void init_decode_cache (struct x86_emulate_ctxt *ctxt);int x86_emulate_insn (struct x86_emulate_ctxt *ctxt); int emulator_task_switch(struct x86_emulate_ctxt *ctxt,arch/x86/kvm/vmx/vmx.c +2
-3@@ -7329,12 +7329,11 @@ static void vmx_request_immediate_exit(struct kvm_vcpu *vcpu)to_vmx (vcpu)->req_immediate_exit = true; } -static int vmx_check_intercept_io (struct kvm_vcpu *vcpu, -struct x86_instruction_info *info) +static bool vmx_check_intercept_io (struct kvm_vcpu *vcpu, +struct x86_instruction_info *info) {struct vmcs12 *vmcs12 =get_vmcs12 (vcpu);unsigned short port; -bool intercept;int size; if (info->intercept == x86_intercept_in ||@@ -7354,13 +7353,10 @@ static int vmx_check_intercept_io(struct kvm_vcpu *vcpu,* Otherwise, IO instruction VM-exits are controlled by the IO bitmaps. */ if (!nested_cpu_has (vmcs12,CPU_BASED_USE_IO_BITMAPS ))
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept_io(struct kvm_vcpu *vcpu, · line 7359
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +3
-6- intercept =nested_cpu_has (vmcs12, -CPU_BASED_UNCOND_IO_EXITING ); - else - intercept =nested_vmx_check_io_bitmaps (vcpu, port, size); +return nested_cpu_has (vmcs12, +CPU_BASED_UNCOND_IO_EXITING ); -/* FIXME: produce nested vmexit and return X86EMUL_INTERCEPTED. */ -return intercept ?X86EMUL_UNHANDLEABLE :X86EMUL_CONTINUE ; +return nested_vmx_check_io_bitmaps (vcpu, port, size);
It might be a slightly bigger patch, but I think it'll be cleaner code in the end if this section is reordered to:
/*
* If the 'use IO bitmaps' VM-execution control is 1, IO instruction
* VM-exits are controlled by the IO bitmaps, otherwise they depend
* on the 'unconditional IO exiting' VM-execution control. */
if (nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS))
return nested_vmx_check_io_bitmaps(vcpu, port, size);
return nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING);
Quoted 18 lines
arch/x86/kvm/vmx/vmx.c +4
-0} static int vmx_check_intercept(struct kvm_vcpu *vcpu,@@ -7369,6 +7365,7 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu, struct x86_exception *exception) {struct vmcs12 *vmcs12 =get_vmcs12 (vcpu); +bool intercepted;switch (info->intercept) {/* @@ -7381,13 +7378,27 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu, exception->error_code_valid = false;return X86EMUL_PROPAGATE_FAULT ; } + + intercepted =nested_cpu_has (vmcs12,CPU_BASED_RDTSC_EXITING ); +
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7393
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +10
-0+/* +* RDTSCP could be emulated on a CPU which doesn't support it. +* As such, flushing the TLB and resuming L2 will result in +* another #UD rather than a VM-exit to reflect into L1. +* Instead, synthesize the VM-exit here. +*/ +if (intercepted) { +nested_vmx_vmexit (vcpu,EXIT_REASON_RDTSCP ,0 ,0 ); +return X86EMUL_INTERCEPTED ; + }
Maybe this instead?
if (!nested_cpu_has2(vmcs12, SECONDARY_EXEC_RDTSCP)) {
exception->vector = UD_VECTOR;
exception->error_code_valid = false;
return X86EMUL_PROPAGATE_FAULT;
} else if (nested_cpu_has(vmcs12, CPU_BASED_RDTSC_EXITING)) {
/*
* RDTSCP could be emulated on a CPU which doesn't
* support it. As such, flushing the TLB and resuming
* L2 will result in another #UD rather than a VM-exit
* to reflect into L1. Instead, synthesize the VM-exit
* here.
*/
nested_vmx_vmexit(vcpu, EXIT_REASON_RDTSCP, 0, 0);
return X86EMUL_INTERCEPTED;
}
intercepted = false;Quoted 24 lines
arch/x86/kvm/vmx/vmx.c +3
-6break ;case x86_intercept_in:case x86_intercept_ins:case x86_intercept_out:case x86_intercept_outs: -return vmx_check_intercept_io (vcpu, info); + intercepted =vmx_check_intercept_io (vcpu, info); +break ;case x86_intercept_lgdt:case x86_intercept_lidt:@@ -7397,18 +7408,41 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu,case x86_intercept_sidt:case x86_intercept_sldt:case x86_intercept_str: -if (!nested_cpu_has2 (vmcs12,SECONDARY_EXEC_DESC )) -return X86EMUL_CONTINUE ; - -/* FIXME: produce nested vmexit and return X86EMUL_INTERCEPTED. */ + intercepted =nested_cpu_has2 (vmcs12,SECONDARY_EXEC_DESC );break ; -/* TODO: check more intercepts... */
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7423
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +10
-0+/* +* The dual-monitor treatment of SMM is not supported in nVMX. As such, +* L0 will never handle the RSM instruction nor should it retry +* instruction execution. Instead, a #UD should be injected into the +* guest for the execution of RSM outside of SMM. +*/ +case x86_intercept_rsm: + exception->vector =UD_VECTOR ; + exception->error_code_valid = false; +return X86EMUL_PROPAGATE_FAULT ;
Why does RSM need special treatment? Won't it just naturally #UD if we fall through to the flush-and-retry path?
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7435
Quoted 13 lines
arch/x86/kvm/vmx/vmx.c +8
-2default : -break ; + intercepted = true; } -return X86EMUL_UNHANDLEABLE ; +if (!intercepted) +return X86EMUL_CONTINUE ; + +/* +* The only uses of the emulator in VMX for instructions which may be +* intercepted are port IO instructions, descriptor-table accesses, and +* the RDTSCP instruction. As such, if the emulator has decoded an
I wouldn't list out the individual cases, it's pretty obvious what can be emulated by looking at the above code, and inevitably something will be added that requires updating this comment.
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7437
Quoted 2 lines
arch/x86/kvm/vmx/vmx.c +2
-0+* instruction that is different from the VM-exit provided by hardware +* it is likely that the TLB entry and page-table mapping for the
Probaby better to avoid talking about "page-table mapping", because it's not clear which page tables are being referenced.
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7438
Quoted 1 line
arch/x86/kvm/vmx/vmx.c +1
-0+* guest's RIP are out of sync.
Maybe something like:
/*
* There are very few instructions that KVM will emulate for L2 and can
* also be intercepted by l1. If the emulator decoded an instruction
* that is different from the VM-exit provided by hardware, the TLB
* entry for guest's RIP is likely stale. Rather than synthesizing a
* VM-exit into L1 for every possible instruction, just flush the TLB,
* resume L2, and let hardware generate the appropriate VM-exit. */
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7444
Quoted 6 lines
arch/x86/kvm/vmx/vmx.c +6
-0+* +* Rather than synthesizing a VM-exit into L1 for every possible +* instruction just flush the TLB, resume L2, and let hardware generate +* the appropriate VM-exit. +*/ +vmx_flush_tlb_gva (vcpu,kvm_rip_read (vcpu));
This is wrong, it should flush kvm_get_linear_rip(vcpu).
Quoted 20 lines
arch/x86/kvm/vmx/vmx.c +1
-0+return X86EMUL_RETRY_INSTR ; }#ifdef CONFIG_X86_64 arch/x86/kvm/x86.c +1
-1@@ -6967,7 +6967,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, r =x86_emulate_insn (ctxt); -if (r ==EMULATION_INTERCEPTED ) +if (r ==EMULATION_INTERCEPTED || r ==EMULATION_RETRY_INSTR )return 1 ;if (r ==EMULATION_FAILED ) {Signature · 1 line
2.27.0.290.gba653c62da-goog
Quoted 232 lines · Sean Christopherson
Quoted 82 lines · Oliver Upton
It is possible for the instruction emulator to decode a different instruction from what was implied by the VM-exit information provided by hardware in vmcs02. Such is the case when the TLB entry for the guest's IP is out of sync with the appropriate page-table mapping if page installation isn't followed with a TLB flush.
Currently, KVM refuses to emulate in these scenarios, instead injecting a #UD into L2. While this does address the security risk of CVE-2020-2732, it could result in spurious #UDs to the L2 guest. Fix this by instead flushing the TLB then resuming L2, allowing hardware to generate the appropriate VM-exit to be reflected into L1.
Exceptional handling is also required for RSM and RDTSCP instructions. RDTSCP could be emulated on hardware which doesn't support it, therefore hardware will not generate a RDTSCP VM-exit on L2 resume. The dual-monitor treatment of SMM is not supported in nVMX, which implies that L0 should never handle a RSM instruction. Resuming the guest will only result in another #UD. Avoid getting stuck in a loop with these instructions by injecting a #UD for RSM and the appropriate VM-exit for RDTSCP.
Fixes 07721feee46b ("KVM: nVMX: Don't emulate instructions in guest mode")Cc Paolo Bonzini <[email protected]>Cc Sean Christopherson <[email protected]>Signed-off-by Oliver Upton <[email protected]>Reviewed-by Jim Mattson <[email protected]>Reviewed-by Peter Shier <[email protected]>--- arch/x86/kvm/emulate.c | 2 ++ arch/x86/kvm/kvm_emulate.h | 1 + arch/x86/kvm/vmx/vmx.c | 68 ++++++++++++++++++++++++++++---------- arch/x86/kvm/x86.c | 2 +- 4 files changed, 55 insertions(+), 18 deletions(-)arch/x86/kvm/emulate.c +2
-0@@ -5812,6 +5812,8 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt) }if (rc ==X86EMUL_INTERCEPTED )return EMULATION_INTERCEPTED ; +if (rc ==X86EMUL_RETRY_INSTR ) +return EMULATION_RETRY_INSTR ;if (rc ==X86EMUL_CONTINUE )writeback_registers (ctxt);arch/x86/kvm/kvm_emulate.h +1
-0@@ -496,6 +496,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt);#define EMULATION_OK 0#define EMULATION_RESTART 1#define EMULATION_INTERCEPTED 2 +#define EMULATION_RETRY_INSTR 3void init_decode_cache (struct x86_emulate_ctxt *ctxt);int x86_emulate_insn (struct x86_emulate_ctxt *ctxt); int emulator_task_switch(struct x86_emulate_ctxt *ctxt,arch/x86/kvm/vmx/vmx.c +2
-3@@ -7329,12 +7329,11 @@ static void vmx_request_immediate_exit(struct kvm_vcpu *vcpu)to_vmx (vcpu)->req_immediate_exit = true; } -static int vmx_check_intercept_io (struct kvm_vcpu *vcpu, -struct x86_instruction_info *info) +static bool vmx_check_intercept_io (struct kvm_vcpu *vcpu, +struct x86_instruction_info *info) {struct vmcs12 *vmcs12 =get_vmcs12 (vcpu);unsigned short port; -bool intercept;int size; if (info->intercept == x86_intercept_in ||@@ -7354,13 +7353,10 @@ static int vmx_check_intercept_io(struct kvm_vcpu *vcpu,* Otherwise, IO instruction VM-exits are controlled by the IO bitmaps. */ if (!nested_cpu_has (vmcs12,CPU_BASED_USE_IO_BITMAPS ))arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept_io(struct kvm_vcpu *vcpu, · line 7359
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +3
-6- intercept =nested_cpu_has (vmcs12, -CPU_BASED_UNCOND_IO_EXITING ); - else - intercept =nested_vmx_check_io_bitmaps (vcpu, port, size); +return nested_cpu_has (vmcs12, +CPU_BASED_UNCOND_IO_EXITING ); -/* FIXME: produce nested vmexit and return X86EMUL_INTERCEPTED. */ -return intercept ?X86EMUL_UNHANDLEABLE :X86EMUL_CONTINUE ; +return nested_vmx_check_io_bitmaps (vcpu, port, size);It might be a slightly bigger patch, but I think it'll be cleaner code in the end if this section is reordered to:
/*
* If the 'use IO bitmaps' VM-execution control is 1, IO instruction
* VM-exits are controlled by the IO bitmaps, otherwise they depend
* on the 'unconditional IO exiting' VM-execution control. */
if (nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS))
returnnested_vmx_check_io_bitmaps(vcpu, port, size);return
nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING);Quoted 18 lines
arch/x86/kvm/vmx/vmx.c +4
-0} static int vmx_check_intercept(struct kvm_vcpu *vcpu,@@ -7369,6 +7365,7 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu, struct x86_exception *exception) {struct vmcs12 *vmcs12 =get_vmcs12 (vcpu); +bool intercepted;switch (info->intercept) {/* @@ -7381,13 +7378,27 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu, exception->error_code_valid = false;return X86EMUL_PROPAGATE_FAULT ; } + + intercepted =nested_cpu_has (vmcs12,CPU_BASED_RDTSC_EXITING ); +arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7393
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +10
-0+/* +* RDTSCP could be emulated on a CPU which doesn't support it. +* As such, flushing the TLB and resuming L2 will result in +* another #UD rather than a VM-exit to reflect into L1. +* Instead, synthesize the VM-exit here. +*/ +if (intercepted) { +nested_vmx_vmexit (vcpu,EXIT_REASON_RDTSCP ,0 ,0 ); +return X86EMUL_INTERCEPTED ; + }Maybe this instead?
if (!nested_cpu_has2(vmcs12, SECONDARY_EXEC_RDTSCP)) { exception->vector = UD_VECTOR; exception->error_code_valid = false; return X86EMUL_PROPAGATE_FAULT; } else if (nested_cpu_has(vmcs12, CPU_BASED_RDTSC_EXITING)) { /* * RDTSCP could be emulated on a CPU which doesn't * support it. As such, flushing the TLB and resuming * L2 will result in another #UD rather than a VM-exit * to reflect into L1. Instead, synthesize the VM-exit * here. */ nested_vmx_vmexit(vcpu, EXIT_REASON_RDTSCP, 0, 0); return X86EMUL_INTERCEPTED; } intercepted = false;Quoted 24 lines
arch/x86/kvm/vmx/vmx.c +3
-6break ;case x86_intercept_in:case x86_intercept_ins:case x86_intercept_out:case x86_intercept_outs: -return vmx_check_intercept_io (vcpu, info); + intercepted =vmx_check_intercept_io (vcpu, info); +break ;case x86_intercept_lgdt:case x86_intercept_lidt:@@ -7397,18 +7408,41 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu,case x86_intercept_sidt:case x86_intercept_sldt:case x86_intercept_str: -if (!nested_cpu_has2 (vmcs12,SECONDARY_EXEC_DESC )) -return X86EMUL_CONTINUE ; - -/* FIXME: produce nested vmexit and return X86EMUL_INTERCEPTED. */ + intercepted =nested_cpu_has2 (vmcs12,SECONDARY_EXEC_DESC );break ; -/* TODO: check more intercepts... */ arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7423
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +10
-0+/* +* The dual-monitor treatment of SMM is not supported in nVMX. As such, +* L0 will never handle the RSM instruction nor should it retry +* instruction execution. Instead, a #UD should be injected into the +* guest for the execution of RSM outside of SMM. +*/ +case x86_intercept_rsm: + exception->vector =UD_VECTOR ; + exception->error_code_valid = false; +return X86EMUL_PROPAGATE_FAULT ;Why does RSM need special treatment? Won't it just naturally #UD if we fall through to the flush-and-retry path?
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7435
Quoted 13 lines
arch/x86/kvm/vmx/vmx.c +8
-2default : -break ; + intercepted = true; } -return X86EMUL_UNHANDLEABLE ; +if (!intercepted) +return X86EMUL_CONTINUE ; + +/* +* The only uses of the emulator in VMX for instructions which may be +* intercepted are port IO instructions, descriptor-table accesses, and +* the RDTSCP instruction. As such, if the emulator has decoded an I wouldn't list out the individual cases, it's pretty obvious what can be emulated by looking at the above code, and inevitably something will be added that requires updating this comment.
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7437
Quoted 2 lines
arch/x86/kvm/vmx/vmx.c +2
-0+* instruction that is different from the VM-exit provided by hardware +* it is likely that the TLB entry and page-table mapping for the Probaby better to avoid talking about "page-table mapping", because it's not clear which page tables are being referenced.
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7438
Quoted 1 line
arch/x86/kvm/vmx/vmx.c +1
-0+* guest's RIP are out of sync. Maybe something like:
/*
* There are very few instructions that KVM will emulate for L2 and can
* also be intercepted by l1. If the emulator decoded an instruction
* that is different from the VM-exit provided by hardware, the TLB
* entry for guest's RIP is likely stale. Rather than synthesizing a
* VM-exit into L1 for every possible instruction, just flush the TLB,
* resume L2, and let hardware generate the appropriate VM-exit. */
Quoted 9 lines
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7444
Quoted 6 lines
arch/x86/kvm/vmx/vmx.c +6
-0+* +* Rather than synthesizing a VM-exit into L1 for every possible +* instruction just flush the TLB, resume L2, and let hardware generate +* the appropriate VM-exit. +*/ +vmx_flush_tlb_gva (vcpu,kvm_rip_read (vcpu));This is wrong, it should flush
kvm_get_linear_rip(vcpu).
I do not believe that the aim of this patch will work anymore, since:
1dbf5d68af6f ("KVM: VMX: Add guest physical address check in EPT violation and misconfig")
Since it is possible to get into the emulator on any instruction that induces an EPT violation, we'd wind up looping when we believe the instruction needs to exit to L1 (TLB flush, resume guest, hit the same EPT violation. Rinse, wash, repeat).
Quoted 20 lines
Quoted 20 lines
arch/x86/kvm/vmx/vmx.c +1
-0+return X86EMUL_RETRY_INSTR ; }#ifdef CONFIG_X86_64 arch/x86/kvm/x86.c +1
-2@@ -6967,7 +6967,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, r =x86_emulate_insn (ctxt); -if (r ==EMULATION_INTERCEPTED ) +if (r ==EMULATION_INTERCEPTED || r ==EMULATION_RETRY_INSTR )return 1 ;if (r ==EMULATION_FAILED ) { --2.27.0.290.gba653c62da-goog
Quoted 13 lines · Oliver Upton
On Thu, Aug 13, 2020 at 12:03 PM Sean Christopherson
Quoted 9 lines
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7444
Quoted 6 lines
arch/x86/kvm/vmx/vmx.c +6
-0+* +* Rather than synthesizing a VM-exit into L1 for every possible +* instruction just flush the TLB, resume L2, and let hardware generate +* the appropriate VM-exit. +*/ +vmx_flush_tlb_gva (vcpu,kvm_rip_read (vcpu));This is wrong, it should flush
kvm_get_linear_rip(vcpu).I do not believe that the aim of this patch will work anymore, since:
Quoted 7 lines
1dbf5d68af6f("KVM: VMX: Add guest physical address check in EPT violation and misconfig")Since it is possible to get into the emulator on any instruction that induces an EPT violation, we'd wind up looping when we believe the instruction needs to exit to L1 (TLB flush, resume guest, hit the same EPT violation. Rinse, wash, repeat).
kvm_get_linear_rip() doesn't walk any page tables, it simply accounts for a non-zero CS.base when !64-bit mode. If we really wanted to, this could use the emulation context's cached _eip, but I don't see any value in that since both GUEST_CS_* and GUEST_RIP will already be cached by KVM.
unsigned long kvm_get_linear_rip(struct kvm_vcpu *vcpu)
{
if (is_64_bit_mode(vcpu))
return kvm_rip_read(vcpu);
return (u32)(get_segment_base(vcpu, VCPU_SREG_CS) +
kvm_rip_read(vcpu));
}Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
arch/x86/kvm/vmx/vmx.c +1
-0+return X86EMUL_RETRY_INSTR ; }#ifdef CONFIG_X86_64 arch/x86/kvm/x86.c +1
-2@@ -6967,7 +6967,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, r =x86_emulate_insn (ctxt); -if (r ==EMULATION_INTERCEPTED ) +if (r ==EMULATION_INTERCEPTED || r ==EMULATION_RETRY_INSTR )return 1 ;if (r ==EMULATION_FAILED ) { --2.27.0.290.gba653c62da-goog
Quoted 28 lines · Sean Christopherson
Quoted 13 lines · Oliver Upton
On Thu, Aug 13, 2020 at 12:03 PM Sean Christopherson
Quoted 9 lines
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7444
Quoted 6 lines
arch/x86/kvm/vmx/vmx.c +6
-0+* +* Rather than synthesizing a VM-exit into L1 for every possible +* instruction just flush the TLB, resume L2, and let hardware generate +* the appropriate VM-exit. +*/ +vmx_flush_tlb_gva (vcpu,kvm_rip_read (vcpu));This is wrong, it should flush
kvm_get_linear_rip(vcpu).I do not believe that the aim of this patch will work anymore, since:
Quoted 7 lines
1dbf5d68af6f("KVM: VMX: Add guest physical address check in EPT violation and misconfig")Since it is possible to get into the emulator on any instruction that induces an EPT violation, we'd wind up looping when we believe the instruction needs to exit to L1 (TLB flush, resume guest, hit the same EPT violation. Rinse, wash, repeat).
kvm_get_linear_rip()doesn't walk any page tables, it simply accounts for a non-zero CS.base when !64-bit mode. If we really wanted to, this could use the emulation context's cached_eip, but I don't see any value in that since both GUEST_CS_* andGUEST_RIPwill already be cached by KVM.
Quoted 7 lines
unsigned long
kvm_get_linear_rip(struct kvm_vcpu *vcpu){ if (is_64_bit_mode(vcpu)) return kvm_rip_read(vcpu); return (u32)(get_segment_base(vcpu, VCPU_SREG_CS) + kvm_rip_read(vcpu)); }
Sorry, I was a tad imprecise. I haven't any issues with your suggestion. Rather, I believe that my overall patch is ineffective.
Suppose we had an EPT violation for a GPA that exceeded the guest's MAXPHYADDR. Let's also say that the EPT violation occurred on the memory operand of an LMSW instruction. Per the aforementioned patch, we will dive into the emulator. Since we check intercepts before reading the operand out of memory, we will fall through to the default case, set intercepted = true, flush TLB and resume.
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
arch/x86/kvm/vmx/vmx.c +1
-0+return X86EMUL_RETRY_INSTR ; }#ifdef CONFIG_X86_64 arch/x86/kvm/x86.c +1
-2@@ -6967,7 +6967,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, r =x86_emulate_insn (ctxt); -if (r ==EMULATION_INTERCEPTED ) +if (r ==EMULATION_INTERCEPTED || r ==EMULATION_RETRY_INSTR )return 1 ;if (r ==EMULATION_FAILED ) { --2.27.0.290.gba653c62da-goog
Quoted 41 lines · Oliver Upton
Quoted 28 lines · Sean Christopherson
Quoted 13 lines · Oliver Upton
On Thu, Aug 13, 2020 at 12:03 PM Sean Christopherson
Quoted 9 lines
arch/x86/kvm/vmx/vmx.c static int vmx_check_intercept(struct kvm_vcpu *vcpu, · line 7444
Quoted 6 lines
arch/x86/kvm/vmx/vmx.c +6
-0+* +* Rather than synthesizing a VM-exit into L1 for every possible +* instruction just flush the TLB, resume L2, and let hardware generate +* the appropriate VM-exit. +*/ +vmx_flush_tlb_gva (vcpu,kvm_rip_read (vcpu));This is wrong, it should flush
kvm_get_linear_rip(vcpu).I do not believe that the aim of this patch will work anymore, since:
Quoted 7 lines
1dbf5d68af6f("KVM: VMX: Add guest physical address check in EPT violation and misconfig")Since it is possible to get into the emulator on any instruction that induces an EPT violation, we'd wind up looping when we believe the instruction needs to exit to L1 (TLB flush, resume guest, hit the same EPT violation. Rinse, wash, repeat).
kvm_get_linear_rip()doesn't walk any page tables, it simply accounts for a non-zero CS.base when !64-bit mode. If we really wanted to, this could use the emulation context's cached_eip, but I don't see any value in that since both GUEST_CS_* andGUEST_RIPwill already be cached by KVM.Quoted 7 lines
unsigned long
kvm_get_linear_rip(struct kvm_vcpu *vcpu){ if (is_64_bit_mode(vcpu)) return kvm_rip_read(vcpu); return (u32)(get_segment_base(vcpu, VCPU_SREG_CS) + kvm_rip_read(vcpu)); }Sorry, I was a tad imprecise. I haven't any issues with your suggestion. Rather, I believe that my overall patch is ineffective.
Quoted 6 lines
Suppose we had an EPT violation for a GPA that exceeded the guest's MAXPHYADDR. Let's also say that the EPT violation occurred on the memory operand of an LMSW instruction. Per the aforementioned patch, we will dive into the emulator. Since we check intercepts before reading the operand out of memory, we will fall through to the default case, set intercepted = true, flush TLB and resume.
Hrm. The new invocation of kvm_emulate_instruction() feels incomplete from the perspective that it doesn't have a flag that states "this should always cause a #PF, freak out if it doesn't". Such a flag would allow keeping this approach as this interception logic could bail early if it is set, knowing that the emulator will inject a #PF or bail to userspace (or something like that).