Re: [RFC PATCH v6 74/92] kvm: x86: do not unconditionally patch the hypercall instruction during emulation
On 14/08/19 14:07, Adalbert Lazăr wrote: > On Tue, 13 Aug 2019 11:20:45 +0200, Paolo Bonzini wrote: >> On 09/08/19 18:00, Adalbert Lazăr wrote: >>> From: Mihai Donțu >>> >>> It can happened for us to end up emulating the VMCALL instruction as a >>> result of the handling of an EPT write fault. In this situation, the >>> emulator will try to unconditionally patch the correct hypercall opcode >>> bytes using emulator_write_emulated(). However, this last call uses the >>> fault GPA (if available) or walks the guest page tables at RIP, >>> otherwise. The trouble begins when using KVMI, when we forbid the use of >>> the fault GPA and fallback to the guest pt walk: in Windows (8.1 and >>> newer) the page that we try to write into is marked read-execute and as >>> such emulator_write_emulated() fails and we inject a write #PF, leading >>> to a guest crash. >>> >>> The fix is rather simple: check the existing instruction bytes before >>> doing the patching. This does not change the normal KVM behaviour, but >>> does help when using KVMI as we no longer inject a write #PF. >> >> Fixing the hypercall is just an optimization. Can we just hush and >> return to the guest if emulator_write_emulated returns >> X86EMUL_PROPAGATE_FAULT? >> >> Paolo > > Something like this? > > err = emulator_write_emulated(...); > if (err == X86EMUL_PROPAGATE_FAULT) > err = X86EMUL_CONTINUE; > return err; Yes. The only difference will be that you'll keep getting #UD vmexits instead of hypercall vmexits. It's also safer, we want to obey those r-x permissions because PatchGuard would crash the system if it noticed the rewriting for whatever reason. Paolo >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index 04b1d2916a0a..965c4f0108eb 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -7363,16 +7363,33 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) >>> } >>> EXPORT_SYMBOL_GPL(kvm_emulate_hypercall); >>> >>> +#define KVM_HYPERCALL_INSN_LEN 3 >>> + >>> static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt) >>> { >>> + int err; >>> struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); >>> - char instruction[3]; >>> + char buf[KVM_HYPERCALL_INSN_LEN]; >>> + char instruction[KVM_HYPERCALL_INSN_LEN]; >>> unsigned long rip = kvm_rip_read(vcpu); >>> >>> + err = emulator_read_emulated(ctxt, rip, buf, sizeof(buf), >>> +>exception); >>> + if (err != X86EMUL_CONTINUE) >>> + return err; >>> + >>> kvm_x86_ops->patch_hypercall(vcpu, instruction); >>> + if (!memcmp(instruction, buf, sizeof(instruction))) >>> + /* >>> +* The hypercall instruction is the correct one. Retry >>> +* its execution maybe we got here as a result of an >>> +* event other than #UD which has been resolved in the >>> +* mean time. >>> +*/ >>> + return X86EMUL_CONTINUE; >>> >>> - return emulator_write_emulated(ctxt, rip, instruction, 3, >>> - >exception); >>> + return emulator_write_emulated(ctxt, rip, instruction, >>> + sizeof(instruction), >exception); >>> } ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v6 74/92] kvm: x86: do not unconditionally patch the hypercall instruction during emulation
On Tue, 13 Aug 2019 11:20:45 +0200, Paolo Bonzini wrote: > On 09/08/19 18:00, Adalbert Lazăr wrote: > > From: Mihai Donțu > > > > It can happened for us to end up emulating the VMCALL instruction as a > > result of the handling of an EPT write fault. In this situation, the > > emulator will try to unconditionally patch the correct hypercall opcode > > bytes using emulator_write_emulated(). However, this last call uses the > > fault GPA (if available) or walks the guest page tables at RIP, > > otherwise. The trouble begins when using KVMI, when we forbid the use of > > the fault GPA and fallback to the guest pt walk: in Windows (8.1 and > > newer) the page that we try to write into is marked read-execute and as > > such emulator_write_emulated() fails and we inject a write #PF, leading > > to a guest crash. > > > > The fix is rather simple: check the existing instruction bytes before > > doing the patching. This does not change the normal KVM behaviour, but > > does help when using KVMI as we no longer inject a write #PF. > > Fixing the hypercall is just an optimization. Can we just hush and > return to the guest if emulator_write_emulated returns > X86EMUL_PROPAGATE_FAULT? > > Paolo Something like this? err = emulator_write_emulated(...); if (err == X86EMUL_PROPAGATE_FAULT) err = X86EMUL_CONTINUE; return err; > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 04b1d2916a0a..965c4f0108eb 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -7363,16 +7363,33 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > > } > > EXPORT_SYMBOL_GPL(kvm_emulate_hypercall); > > > > +#define KVM_HYPERCALL_INSN_LEN 3 > > + > > static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt) > > { > > + int err; > > struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); > > - char instruction[3]; > > + char buf[KVM_HYPERCALL_INSN_LEN]; > > + char instruction[KVM_HYPERCALL_INSN_LEN]; > > unsigned long rip = kvm_rip_read(vcpu); > > > > + err = emulator_read_emulated(ctxt, rip, buf, sizeof(buf), > > +>exception); > > + if (err != X86EMUL_CONTINUE) > > + return err; > > + > > kvm_x86_ops->patch_hypercall(vcpu, instruction); > > + if (!memcmp(instruction, buf, sizeof(instruction))) > > + /* > > +* The hypercall instruction is the correct one. Retry > > +* its execution maybe we got here as a result of an > > +* event other than #UD which has been resolved in the > > +* mean time. > > +*/ > > + return X86EMUL_CONTINUE; > > > > - return emulator_write_emulated(ctxt, rip, instruction, 3, > > - >exception); > > + return emulator_write_emulated(ctxt, rip, instruction, > > + sizeof(instruction), >exception); > > } ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v6 74/92] kvm: x86: do not unconditionally patch the hypercall instruction during emulation
On 09/08/19 18:00, Adalbert Lazăr wrote: > From: Mihai Donțu > > It can happened for us to end up emulating the VMCALL instruction as a > result of the handling of an EPT write fault. In this situation, the > emulator will try to unconditionally patch the correct hypercall opcode > bytes using emulator_write_emulated(). However, this last call uses the > fault GPA (if available) or walks the guest page tables at RIP, > otherwise. The trouble begins when using KVMI, when we forbid the use of > the fault GPA and fallback to the guest pt walk: in Windows (8.1 and > newer) the page that we try to write into is marked read-execute and as > such emulator_write_emulated() fails and we inject a write #PF, leading > to a guest crash. > > The fix is rather simple: check the existing instruction bytes before > doing the patching. This does not change the normal KVM behaviour, but > does help when using KVMI as we no longer inject a write #PF. Fixing the hypercall is just an optimization. Can we just hush and return to the guest if emulator_write_emulated returns X86EMUL_PROPAGATE_FAULT? Paolo > CC: Joerg Roedel > Signed-off-by: Mihai Donțu > Signed-off-by: Adalbert Lazăr > --- > arch/x86/kvm/x86.c | 23 --- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 04b1d2916a0a..965c4f0108eb 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -7363,16 +7363,33 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > } > EXPORT_SYMBOL_GPL(kvm_emulate_hypercall); > > +#define KVM_HYPERCALL_INSN_LEN 3 > + > static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt) > { > + int err; > struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); > - char instruction[3]; > + char buf[KVM_HYPERCALL_INSN_LEN]; > + char instruction[KVM_HYPERCALL_INSN_LEN]; > unsigned long rip = kvm_rip_read(vcpu); > > + err = emulator_read_emulated(ctxt, rip, buf, sizeof(buf), > + >exception); > + if (err != X86EMUL_CONTINUE) > + return err; > + > kvm_x86_ops->patch_hypercall(vcpu, instruction); > + if (!memcmp(instruction, buf, sizeof(instruction))) > + /* > + * The hypercall instruction is the correct one. Retry > + * its execution maybe we got here as a result of an > + * event other than #UD which has been resolved in the > + * mean time. > + */ > + return X86EMUL_CONTINUE; > > - return emulator_write_emulated(ctxt, rip, instruction, 3, > - >exception); > + return emulator_write_emulated(ctxt, rip, instruction, > +sizeof(instruction), >exception); > } > > static int dm_request_for_irq_injection(struct kvm_vcpu *vcpu) > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH v6 74/92] kvm: x86: do not unconditionally patch the hypercall instruction during emulation
From: Mihai Donțu It can happened for us to end up emulating the VMCALL instruction as a result of the handling of an EPT write fault. In this situation, the emulator will try to unconditionally patch the correct hypercall opcode bytes using emulator_write_emulated(). However, this last call uses the fault GPA (if available) or walks the guest page tables at RIP, otherwise. The trouble begins when using KVMI, when we forbid the use of the fault GPA and fallback to the guest pt walk: in Windows (8.1 and newer) the page that we try to write into is marked read-execute and as such emulator_write_emulated() fails and we inject a write #PF, leading to a guest crash. The fix is rather simple: check the existing instruction bytes before doing the patching. This does not change the normal KVM behaviour, but does help when using KVMI as we no longer inject a write #PF. CC: Joerg Roedel Signed-off-by: Mihai Donțu Signed-off-by: Adalbert Lazăr --- arch/x86/kvm/x86.c | 23 --- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 04b1d2916a0a..965c4f0108eb 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -7363,16 +7363,33 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) } EXPORT_SYMBOL_GPL(kvm_emulate_hypercall); +#define KVM_HYPERCALL_INSN_LEN 3 + static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt) { + int err; struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); - char instruction[3]; + char buf[KVM_HYPERCALL_INSN_LEN]; + char instruction[KVM_HYPERCALL_INSN_LEN]; unsigned long rip = kvm_rip_read(vcpu); + err = emulator_read_emulated(ctxt, rip, buf, sizeof(buf), +>exception); + if (err != X86EMUL_CONTINUE) + return err; + kvm_x86_ops->patch_hypercall(vcpu, instruction); + if (!memcmp(instruction, buf, sizeof(instruction))) + /* +* The hypercall instruction is the correct one. Retry +* its execution maybe we got here as a result of an +* event other than #UD which has been resolved in the +* mean time. +*/ + return X86EMUL_CONTINUE; - return emulator_write_emulated(ctxt, rip, instruction, 3, - >exception); + return emulator_write_emulated(ctxt, rip, instruction, + sizeof(instruction), >exception); } static int dm_request_for_irq_injection(struct kvm_vcpu *vcpu) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization