| Message ID | 20241217181458.68690-5-iorlov@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance event delivery error handling | expand |
On Tue, Dec 17, 2024, Ivan Orlov wrote: > Move unhandleable vmexit during vectoring error detection > into check_emulate_instruction. Implement the function which prohibits > the emulation if EMULTYPE_PF is set when vectoring, otherwise such a > situation may occur: I definitely think it's worth explaining that moving the detection covers new emulation cases, and also calling out that handle_ept_misconfig() consults vmx_check_emulate_instruction(), i.e. that moving the detection shouldn't affect KVM's overall handlng of EPT Misconfig. -- Move handling of emulation during event vectoring, which KVM doesn't support, into VMX's check_emulate_instruction(), so that KVM detects all unsupported emulation, not just cached emulated MMIO (EPT misconfig). E.g. on emulated MMIO that isn't cached (EPT Violation) or occurs with legacy shadow paging (#PF). Rejecting emulation on other sources of emulation also fixes a largely theoretical flaw (thanks to the "unprotect and retry" logic), where KVM could incorrectly inject a #DF: 1. CPU executes an instruction and hits a #GP 2. While vectoring the #GP, a shadow #PF occurs 3. On the #PF VM-Exit, KVM re-injects #GP 4. KVM emulates because of the write-protected page 5. KVM "successfully" emulates and also detects the #GP 6. KVM synthesizes a #GP, and since #GP has already been injected, incorrectly escalates to a #DF. Fix the comment about EMULTYPE_PF as this flag doesn't necessarily mean MMIO anymore: it can also be set due to the write protection violation. Note, handle_ept_misconfig() checks vmx_check_emulate_instruction() before attempting emulation of any kind.
On 12/18/24 18:39, Sean Christopherson wrote: > I definitely think it's worth explaining that moving the detection covers new > emulation cases, and also calling out that handle_ept_misconfig() consults > vmx_check_emulate_instruction(), i.e. that moving the detection shouldn't > affect KVM's overall handlng of EPT Misconfig. > > -- > > Move handling of emulation during event vectoring, which KVM doesn't > support, into VMX's check_emulate_instruction(), so that KVM detects > all unsupported emulation, not just cached emulated MMIO (EPT misconfig). > E.g. on emulated MMIO that isn't cached (EPT Violation) or occurs with > legacy shadow paging (#PF). > > Rejecting emulation on other sources of emulation also fixes a largely > theoretical flaw (thanks to the "unprotect and retry" logic), where KVM > could incorrectly inject a #DF: > > 1. CPU executes an instruction and hits a #GP > 2. While vectoring the #GP, a shadow #PF occurs > 3. On the #PF VM-Exit, KVM re-injects #GP > 4. KVM emulates because of the write-protected page > 5. KVM "successfully" emulates and also detects the #GP > 6. KVM synthesizes a #GP, and since #GP has already been injected, > incorrectly escalates to a #DF. > > Fix the comment about EMULTYPE_PF as this flag doesn't necessarily > mean MMIO anymore: it can also be set due to the write protection > violation. > > Note, handle_ept_misconfig() checks vmx_check_emulate_instruction() before > attempting emulation of any kind. > Yeah, I thought that covering the change in non-cacheable MMIO / shadow paged #PF handling, but forgot to include it into the commit message :( Could you please fix the message when applying? The message you suggested looks good to me. Thanks!
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index de8fb1ab230c..f3a1d050e1d6 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -2019,8 +2019,8 @@ u64 vcpu_tsc_khz(struct kvm_vcpu *vcpu); * VMware backdoor emulation handles select instructions * and reinjects the #GP for all other cases. * - * EMULTYPE_PF - Set when emulating MMIO by way of an intercepted #PF, in which - * case the CR2/GPA value pass on the stack is valid. + * EMULTYPE_PF - Set when an intercepted #PF triggers the emulation, in which case + * the CR2/GPA value pass on the stack is valid. * * EMULTYPE_COMPLETE_USER_EXIT - Set when the emulator should update interruptibility * state and inject single-step #DBs after skipping @@ -2055,6 +2055,11 @@ u64 vcpu_tsc_khz(struct kvm_vcpu *vcpu); #define EMULTYPE_COMPLETE_USER_EXIT (1 << 7) #define EMULTYPE_WRITE_PF_TO_SP (1 << 8) +static inline bool kvm_can_emulate_event_vectoring(int emul_type) +{ + return !(emul_type & EMULTYPE_PF); +} + int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type); int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu, void *insn, int insn_len); diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index acc2f0e0a339..89ddbe1175c7 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1705,6 +1705,12 @@ int vmx_check_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type, kvm_queue_exception(vcpu, UD_VECTOR); return X86EMUL_PROPAGATE_FAULT; } + + /* Check that emulation is possible during event vectoring */ + if ((to_vmx(vcpu)->idt_vectoring_info & VECTORING_INFO_VALID_MASK) && + !kvm_can_emulate_event_vectoring(emul_type)) + return X86EMUL_UNHANDLEABLE_VECTORING; + return X86EMUL_CONTINUE; } @@ -6543,26 +6549,15 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath) return 0; } - /* - * Note: - * Do not try to fix EXIT_REASON_EPT_MISCONFIG if it caused by - * delivery event since it indicates guest is accessing MMIO. - * The vm-exit can be triggered again after return to guest that - * will cause infinite loop. - */ if ((vectoring_info & VECTORING_INFO_VALID_MASK) && (exit_reason.basic != EXIT_REASON_EXCEPTION_NMI && exit_reason.basic != EXIT_REASON_EPT_VIOLATION && exit_reason.basic != EXIT_REASON_PML_FULL && exit_reason.basic != EXIT_REASON_APIC_ACCESS && exit_reason.basic != EXIT_REASON_TASK_SWITCH && - exit_reason.basic != EXIT_REASON_NOTIFY)) { - gpa_t gpa = INVALID_GPA; - - if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG) - gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS); - - kvm_prepare_event_vectoring_exit(vcpu, gpa); + exit_reason.basic != EXIT_REASON_NOTIFY && + exit_reason.basic != EXIT_REASON_EPT_MISCONFIG)) { + kvm_prepare_event_vectoring_exit(vcpu, INVALID_GPA); return 0; }
Move unhandleable vmexit during vectoring error detection into check_emulate_instruction. Implement the function which prohibits the emulation if EMULTYPE_PF is set when vectoring, otherwise such a situation may occur: 1. CPU executes an instruction and hits a #GP 2. While vectoring the #GP, a shadow #PF occurs 3. On vmexit, KVM re-injects #GP 4. KVM emulates because of the write-protected page 5. KVM "successfully" emulates and also detects the #GP 6. KVM synthesizes a #GP, and since #GP has already been injected, incorrectly escalates to a #DF. Fix the comment about EMULTYPE_PF as this flag doesn't necessarily mean MMIO anymore: it can also be set due to the write protection violation. Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Ivan Orlov <iorlov@amazon.com> --- V1 -> V2: - Detect the unhandleable vectoring error in vmx_check_emulate_instruction instead of handling it in the common MMU code (which is specific for cached MMIO) V2 -> V3: - Prohibit any emulation during vectoring if it happens due to an intercepted #PF. arch/x86/include/asm/kvm_host.h | 9 +++++++-- arch/x86/kvm/vmx/vmx.c | 23 +++++++++-------------- 2 files changed, 16 insertions(+), 16 deletions(-)