| Message ID | 20180524155056.710-1-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 24, 2018 at 05:50:56PM +0200, Radim Krčmář wrote: > If the hypercall was called from userspace or real mode, KVM injects #UD > and then advances RIP, so it looks like #UD was caused by the following > instruction. This probably won't cause more than confusion, but could > give an unexpected access to guest OS' instruction emulator. What does the SDM say about this? Or does the HyperV specification give a firm statement that this is the expectation ? > > Also, refactor the code to count hv hypercalls that were handled by the > virt userspace. > > Fixes: 6356ee0c9602 ("x86: Delay skip of emulated hypercall instruction") > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > arch/x86/kvm/hyperv.c | 19 +++++++++++-------- > arch/x86/kvm/x86.c | 12 ++++-------- > 2 files changed, 15 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index 5708e951a5c6..46ff64da44ca 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -1260,12 +1260,16 @@ static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result) > } > } > > +static int kvm_hv_hypercall_complete(struct kvm_vcpu *vcpu, u64 result) > +{ > + kvm_hv_hypercall_set_result(vcpu, result); > + ++vcpu->stat.hypercalls; > + return kvm_skip_emulated_instruction(vcpu); > +} > + > static int kvm_hv_hypercall_complete_userspace(struct kvm_vcpu *vcpu) > { > - struct kvm_run *run = vcpu->run; > - > - kvm_hv_hypercall_set_result(vcpu, run->hyperv.u.hcall.result); > - return kvm_skip_emulated_instruction(vcpu); > + return kvm_hv_hypercall_complete(vcpu, vcpu->run->hyperv.u.hcall.result); > } > > static u16 kvm_hvcall_signal_event(struct kvm_vcpu *vcpu, bool fast, u64 param) > @@ -1350,7 +1354,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) > /* Hypercall continuation is not supported yet */ > if (rep_cnt || rep_idx) { > ret = HV_STATUS_INVALID_HYPERCALL_CODE; > - goto set_result; > + goto out; > } > > switch (code) { > @@ -1381,9 +1385,8 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) > break; > } > > -set_result: > - kvm_hv_hypercall_set_result(vcpu, ret); > - return 1; > +out: > + return kvm_hv_hypercall_complete(vcpu, ret); > } > > void kvm_hv_init_vm(struct kvm *kvm) > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 37dd9a9d050a..35a20fcfb661 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6676,11 +6676,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > unsigned long nr, a0, a1, a2, a3, ret; > int op_64_bit; > > - if (kvm_hv_hypercall_enabled(vcpu->kvm)) { > - if (!kvm_hv_hypercall(vcpu)) > - return 0; > - goto out; > - } > + if (kvm_hv_hypercall_enabled(vcpu->kvm)) > + return kvm_hv_hypercall(vcpu); > > nr = kvm_register_read(vcpu, VCPU_REGS_RAX); > a0 = kvm_register_read(vcpu, VCPU_REGS_RBX); > @@ -6701,7 +6698,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > > if (kvm_x86_ops->get_cpl(vcpu) != 0) { > ret = -KVM_EPERM; > - goto out_error; > + goto out; > } > > switch (nr) { > @@ -6721,12 +6718,11 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > ret = -KVM_ENOSYS; > break; > } > -out_error: > +out: > if (!op_64_bit) > ret = (u32)ret; > kvm_register_write(vcpu, VCPU_REGS_RAX, ret); > > -out: > ++vcpu->stat.hypercalls; > return kvm_skip_emulated_instruction(vcpu); > } > -- > 2.17.0 >
On 24/05/2018 18:08, Konrad Rzeszutek Wilk wrote: > On Thu, May 24, 2018 at 05:50:56PM +0200, Radim Krčmář wrote: >> If the hypercall was called from userspace or real mode, KVM injects #UD >> and then advances RIP, so it looks like #UD was caused by the following >> instruction. This probably won't cause more than confusion, but could >> give an unexpected access to guest OS' instruction emulator. > > What does the SDM say about this? Or does the HyperV specification give > a firm statement that this is the expectation ? #UD is a fault and its RIP always points to the instruction that caused the exception. But you of course know this already, so I'm not sure I understand the question. Paolo
On 24/05/2018 17:50, Radim Krčmář wrote: > If the hypercall was called from userspace or real mode, KVM injects #UD > and then advances RIP, so it looks like #UD was caused by the following > instruction. This probably won't cause more than confusion, but could > give an unexpected access to guest OS' instruction emulator. > > Also, refactor the code to count hv hypercalls that were handled by the > virt userspace. > > Fixes: 6356ee0c9602 ("x86: Delay skip of emulated hypercall instruction") > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > arch/x86/kvm/hyperv.c | 19 +++++++++++-------- > arch/x86/kvm/x86.c | 12 ++++-------- > 2 files changed, 15 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index 5708e951a5c6..46ff64da44ca 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -1260,12 +1260,16 @@ static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result) > } > } > > +static int kvm_hv_hypercall_complete(struct kvm_vcpu *vcpu, u64 result) > +{ > + kvm_hv_hypercall_set_result(vcpu, result); > + ++vcpu->stat.hypercalls; > + return kvm_skip_emulated_instruction(vcpu); > +} > + > static int kvm_hv_hypercall_complete_userspace(struct kvm_vcpu *vcpu) > { > - struct kvm_run *run = vcpu->run; > - > - kvm_hv_hypercall_set_result(vcpu, run->hyperv.u.hcall.result); > - return kvm_skip_emulated_instruction(vcpu); > + return kvm_hv_hypercall_complete(vcpu, vcpu->run->hyperv.u.hcall.result); > } > > static u16 kvm_hvcall_signal_event(struct kvm_vcpu *vcpu, bool fast, u64 param) > @@ -1350,7 +1354,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) > /* Hypercall continuation is not supported yet */ > if (rep_cnt || rep_idx) { > ret = HV_STATUS_INVALID_HYPERCALL_CODE; > - goto set_result; > + goto out; > } > > switch (code) { > @@ -1381,9 +1385,8 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) > break; > } > > -set_result: > - kvm_hv_hypercall_set_result(vcpu, ret); > - return 1; > +out: > + return kvm_hv_hypercall_complete(vcpu, ret); > } > > void kvm_hv_init_vm(struct kvm *kvm) > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 37dd9a9d050a..35a20fcfb661 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6676,11 +6676,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > unsigned long nr, a0, a1, a2, a3, ret; > int op_64_bit; > > - if (kvm_hv_hypercall_enabled(vcpu->kvm)) { > - if (!kvm_hv_hypercall(vcpu)) > - return 0; > - goto out; > - } > + if (kvm_hv_hypercall_enabled(vcpu->kvm)) > + return kvm_hv_hypercall(vcpu); > > nr = kvm_register_read(vcpu, VCPU_REGS_RAX); > a0 = kvm_register_read(vcpu, VCPU_REGS_RBX); > @@ -6701,7 +6698,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > > if (kvm_x86_ops->get_cpl(vcpu) != 0) { > ret = -KVM_EPERM; > - goto out_error; > + goto out; > } > > switch (nr) { > @@ -6721,12 +6718,11 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > ret = -KVM_ENOSYS; > break; > } > -out_error: > +out: > if (!op_64_bit) > ret = (u32)ret; > kvm_register_write(vcpu, VCPU_REGS_RAX, ret); > > -out: > ++vcpu->stat.hypercalls; > return kvm_skip_emulated_instruction(vcpu); > } > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
2018-05-24 18:22+0200, Paolo Bonzini: > On 24/05/2018 18:08, Konrad Rzeszutek Wilk wrote: > > On Thu, May 24, 2018 at 05:50:56PM +0200, Radim Krčmář wrote: > >> If the hypercall was called from userspace or real mode, KVM injects #UD > >> and then advances RIP, so it looks like #UD was caused by the following > >> instruction. This probably won't cause more than confusion, but could > >> give an unexpected access to guest OS' instruction emulator. > > > > What does the SDM say about this? Or does the HyperV specification give > > a firm statement that this is the expectation ? > > #UD is a fault and its RIP always points to the instruction that caused > the exception. But you of course know this already, so I'm not sure I > understand the question. That was my reasoning. And for completeness, TLFS only says this: Hypercalls can be invoked only from the most privileged guest processor mode. In the case of x64, this means protected mode with a current privilege level (CPL) of zero. Although real-mode code runs with an effective CPL of zero, hypercalls are not allowed in real mode. An attempt to invoke a hypercall within an illegal processor mode will generate a #UD (undefined operation) exception.
On Thu, May 24, 2018 at 07:01:04PM +0200, Radim Krčmář wrote: > 2018-05-24 18:22+0200, Paolo Bonzini: > > On 24/05/2018 18:08, Konrad Rzeszutek Wilk wrote: > > > On Thu, May 24, 2018 at 05:50:56PM +0200, Radim Krčmář wrote: > > >> If the hypercall was called from userspace or real mode, KVM injects #UD > > >> and then advances RIP, so it looks like #UD was caused by the following > > >> instruction. This probably won't cause more than confusion, but could > > >> give an unexpected access to guest OS' instruction emulator. > > > > > > What does the SDM say about this? Or does the HyperV specification give > > > a firm statement that this is the expectation ? > > > > #UD is a fault and its RIP always points to the instruction that caused > > the exception. But you of course know this already, so I'm not sure I > > understand the question. > > That was my reasoning. And for completeness, TLFS only says this: > > Hypercalls can be invoked only from the most privileged guest processor > mode. In the case of x64, this means protected mode with a current > privilege level (CPL) of zero. Although real-mode code runs with an > effective CPL of zero, hypercalls are not allowed in real mode. An > attempt to invoke a hypercall within an illegal processor mode will > generate a #UD (undefined operation) exception. Which is weird b/c I remember seeing an bug where the opposite was requested - that is VMCALL was allowed from ring 3 in Windows .. or maybe a similar bug where ring3 did a vmcall and something was not handled correctly. Either way the patch seems right.
2018-05-24 18:22+0200, Paolo Bonzini:
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Thanks, applied.
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 5708e951a5c6..46ff64da44ca 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -1260,12 +1260,16 @@ static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result) } } +static int kvm_hv_hypercall_complete(struct kvm_vcpu *vcpu, u64 result) +{ + kvm_hv_hypercall_set_result(vcpu, result); + ++vcpu->stat.hypercalls; + return kvm_skip_emulated_instruction(vcpu); +} + static int kvm_hv_hypercall_complete_userspace(struct kvm_vcpu *vcpu) { - struct kvm_run *run = vcpu->run; - - kvm_hv_hypercall_set_result(vcpu, run->hyperv.u.hcall.result); - return kvm_skip_emulated_instruction(vcpu); + return kvm_hv_hypercall_complete(vcpu, vcpu->run->hyperv.u.hcall.result); } static u16 kvm_hvcall_signal_event(struct kvm_vcpu *vcpu, bool fast, u64 param) @@ -1350,7 +1354,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) /* Hypercall continuation is not supported yet */ if (rep_cnt || rep_idx) { ret = HV_STATUS_INVALID_HYPERCALL_CODE; - goto set_result; + goto out; } switch (code) { @@ -1381,9 +1385,8 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) break; } -set_result: - kvm_hv_hypercall_set_result(vcpu, ret); - return 1; +out: + return kvm_hv_hypercall_complete(vcpu, ret); } void kvm_hv_init_vm(struct kvm *kvm) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 37dd9a9d050a..35a20fcfb661 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6676,11 +6676,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) unsigned long nr, a0, a1, a2, a3, ret; int op_64_bit; - if (kvm_hv_hypercall_enabled(vcpu->kvm)) { - if (!kvm_hv_hypercall(vcpu)) - return 0; - goto out; - } + if (kvm_hv_hypercall_enabled(vcpu->kvm)) + return kvm_hv_hypercall(vcpu); nr = kvm_register_read(vcpu, VCPU_REGS_RAX); a0 = kvm_register_read(vcpu, VCPU_REGS_RBX); @@ -6701,7 +6698,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) if (kvm_x86_ops->get_cpl(vcpu) != 0) { ret = -KVM_EPERM; - goto out_error; + goto out; } switch (nr) { @@ -6721,12 +6718,11 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) ret = -KVM_ENOSYS; break; } -out_error: +out: if (!op_64_bit) ret = (u32)ret; kvm_register_write(vcpu, VCPU_REGS_RAX, ret); -out: ++vcpu->stat.hypercalls; return kvm_skip_emulated_instruction(vcpu); }
If the hypercall was called from userspace or real mode, KVM injects #UD and then advances RIP, so it looks like #UD was caused by the following instruction. This probably won't cause more than confusion, but could give an unexpected access to guest OS' instruction emulator. Also, refactor the code to count hv hypercalls that were handled by the virt userspace. Fixes: 6356ee0c9602 ("x86: Delay skip of emulated hypercall instruction") Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- arch/x86/kvm/hyperv.c | 19 +++++++++++-------- arch/x86/kvm/x86.c | 12 ++++-------- 2 files changed, 15 insertions(+), 16 deletions(-)