Message ID | 848bc23b-56a7-8088-067d-09a7c19ecb8a@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 08/03/16 23:44, Andrew Cooper wrote: > On 03/08/2016 21:37, Bread Cutter wrote: >> Hello all, >> >> I'm writing an executable that runs inside of a guest, and I planned >> to use vmcall to talk to a tool running in Dom0, using the vm_event >> API. It didn't work, and looking through the code, the first thing >> hvm_do_hypercall() does is check if the guest is in ring0. If not, it >> returns EPERM and exits. >> >> In the case of HVMOP_guest_request_vm_event, I'd rather it be up to my >> code if a call can be made from CPL>0. Is this done intentionally? > > In general, allowing hypercalls from user context is unsafe, and the > subject of several arguments in the past. > > However, in this specific case there are plenty of ways for userspace to > get the attention of an introspection agent, although in inefficient > ways. As such, blocking access is pointless. In XenServer, we have > whitelisted that specific hypercall. > > You want something like: > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index c1b8392..c7a2cdf 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -5194,8 +5194,14 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) > switch ( mode ) > { > case 8: > + if ( eax == __HYPERVISOR_hvm_op && > + regs->rdi == HVMOP_guest_request_vm_event ) > + break; > case 4: > case 2: > + if ( eax == __HYPERVISOR_hvm_op && > + regs->ebx == HVMOP_guest_request_vm_event ) > + break; > hvm_get_segment_register(curr, x86_seg_ss, &sreg); > if ( unlikely(sreg.attr.fields.dpl) ) > { Indeed, if everyone agrees that the patch is acceptable I'm happy to send it to xen-devel. It'd obviously be great if this ends up upstream. Thanks, Razvan
On 03/08/2016 22:00, Razvan Cojocaru wrote: > On 08/03/16 23:44, Andrew Cooper wrote: >> On 03/08/2016 21:37, Bread Cutter wrote: >>> Hello all, >>> >>> I'm writing an executable that runs inside of a guest, and I planned >>> to use vmcall to talk to a tool running in Dom0, using the vm_event >>> API. It didn't work, and looking through the code, the first thing >>> hvm_do_hypercall() does is check if the guest is in ring0. If not, it >>> returns EPERM and exits. >>> >>> In the case of HVMOP_guest_request_vm_event, I'd rather it be up to my >>> code if a call can be made from CPL>0. Is this done intentionally? >> In general, allowing hypercalls from user context is unsafe, and the >> subject of several arguments in the past. >> >> However, in this specific case there are plenty of ways for userspace to >> get the attention of an introspection agent, although in inefficient >> ways. As such, blocking access is pointless. In XenServer, we have >> whitelisted that specific hypercall. >> >> You want something like: >> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> index c1b8392..c7a2cdf 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -5194,8 +5194,14 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) >> switch ( mode ) >> { >> case 8: >> + if ( eax == __HYPERVISOR_hvm_op && >> + regs->rdi == HVMOP_guest_request_vm_event ) >> + break; >> case 4: >> case 2: >> + if ( eax == __HYPERVISOR_hvm_op && >> + regs->ebx == HVMOP_guest_request_vm_event ) >> + break; >> hvm_get_segment_register(curr, x86_seg_ss, &sreg); >> if ( unlikely(sreg.attr.fields.dpl) ) >> { > Indeed, if everyone agrees that the patch is acceptable I'm happy to > send it to xen-devel. It'd obviously be great if this ends up upstream. A +1 from me, but there is substantial resistance from others, which is what stopped my previous attempt to allow a kernel to opt in to allowing userspace hypercalls. ~Andrew
>>> On 03.08.16 at 22:44, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -5194,8 +5194,14 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) > switch ( mode ) > { > case 8: > + if ( eax == __HYPERVISOR_hvm_op && > + regs->rdi == HVMOP_guest_request_vm_event ) > + break; > case 4: > case 2: > + if ( eax == __HYPERVISOR_hvm_op && > + regs->ebx == HVMOP_guest_request_vm_event ) I think you really mean _ebx here. Jan
>>> On 03.08.16 at 23:00, <rcojocaru@bitdefender.com> wrote: > On 08/03/16 23:44, Andrew Cooper wrote: >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -5194,8 +5194,14 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) >> switch ( mode ) >> { >> case 8: >> + if ( eax == __HYPERVISOR_hvm_op && >> + regs->rdi == HVMOP_guest_request_vm_event ) >> + break; >> case 4: >> case 2: >> + if ( eax == __HYPERVISOR_hvm_op && >> + regs->ebx == HVMOP_guest_request_vm_event ) >> + break; >> hvm_get_segment_register(curr, x86_seg_ss, &sreg); >> if ( unlikely(sreg.attr.fields.dpl) ) >> { > > Indeed, if everyone agrees that the patch is acceptable I'm happy to > send it to xen-devel. It'd obviously be great if this ends up upstream. Well, I'm not convinced special casing like this is a good idea. And I'd really like to get a reference to previous discussions (as mentioned by Andrew). Jan
On 04/08/16 08:13, Jan Beulich wrote: >>>> On 03.08.16 at 22:44, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -5194,8 +5194,14 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) >> switch ( mode ) >> { >> case 8: >> + if ( eax == __HYPERVISOR_hvm_op && >> + regs->rdi == HVMOP_guest_request_vm_event ) >> + break; >> case 4: >> case 2: >> + if ( eax == __HYPERVISOR_hvm_op && >> + regs->ebx == HVMOP_guest_request_vm_event ) > I think you really mean _ebx here. True, but it is very hard in 32 or 16bit code to have upper bits set in the 64bit register. ~Andrew
>>> On 04.08.16 at 11:24, <andrew.cooper3@citrix.com> wrote: > On 04/08/16 08:13, Jan Beulich wrote: >>>>> On 03.08.16 at 22:44, <andrew.cooper3@citrix.com> wrote: >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -5194,8 +5194,14 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) >>> switch ( mode ) >>> { >>> case 8: >>> + if ( eax == __HYPERVISOR_hvm_op && >>> + regs->rdi == HVMOP_guest_request_vm_event ) >>> + break; >>> case 4: >>> case 2: >>> + if ( eax == __HYPERVISOR_hvm_op && >>> + regs->ebx == HVMOP_guest_request_vm_event ) >> I think you really mean _ebx here. > > True, but it is very hard in 32 or 16bit code to have upper bits set in > the 64bit register. But it's not impossible. Jan
On 04/08/16 08:23, Jan Beulich wrote: >>>> On 03.08.16 at 23:00, <rcojocaru@bitdefender.com> wrote: >> On 08/03/16 23:44, Andrew Cooper wrote: >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -5194,8 +5194,14 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) >>> switch ( mode ) >>> { >>> case 8: >>> + if ( eax == __HYPERVISOR_hvm_op && >>> + regs->rdi == HVMOP_guest_request_vm_event ) >>> + break; >>> case 4: >>> case 2: >>> + if ( eax == __HYPERVISOR_hvm_op && >>> + regs->ebx == HVMOP_guest_request_vm_event ) >>> + break; >>> hvm_get_segment_register(curr, x86_seg_ss, &sreg); >>> if ( unlikely(sreg.attr.fields.dpl) ) >>> { >> Indeed, if everyone agrees that the patch is acceptable I'm happy to >> send it to xen-devel. It'd obviously be great if this ends up upstream. > Well, I'm not convinced special casing like this is a good idea. Why? Userspace can very easily make this action happen, but only in inefficient ways involving unnecessary emulation and likely pretending to be malware. Blocking access only increases the overhead of the communication channel. It doesn't in any way prevent it. > And I'd really like to get a reference to previous discussions (as mentioned by Andrew). You mean the userspace hypercalls discussion? "[Xen-devel] RFC Userspace hypercalls" ~Andrew
>>> On 04.08.16 at 11:32, <andrew.cooper3@citrix.com> wrote: > On 04/08/16 08:23, Jan Beulich wrote: >>>>> On 03.08.16 at 23:00, <rcojocaru@bitdefender.com> wrote: >>> On 08/03/16 23:44, Andrew Cooper wrote: >>>> --- a/xen/arch/x86/hvm/hvm.c >>>> +++ b/xen/arch/x86/hvm/hvm.c >>>> @@ -5194,8 +5194,14 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) >>>> switch ( mode ) >>>> { >>>> case 8: >>>> + if ( eax == __HYPERVISOR_hvm_op && >>>> + regs->rdi == HVMOP_guest_request_vm_event ) >>>> + break; >>>> case 4: >>>> case 2: >>>> + if ( eax == __HYPERVISOR_hvm_op && >>>> + regs->ebx == HVMOP_guest_request_vm_event ) >>>> + break; >>>> hvm_get_segment_register(curr, x86_seg_ss, &sreg); >>>> if ( unlikely(sreg.attr.fields.dpl) ) >>>> { >>> Indeed, if everyone agrees that the patch is acceptable I'm happy to >>> send it to xen-devel. It'd obviously be great if this ends up upstream. >> Well, I'm not convinced special casing like this is a good idea. > > Why? Primarily because such special casing doesn't scale well - what if a 2nd, 3rd, and 4th one want passing through. The more that such a request being embedded in a multicall would then need special casing too. Jan
My workaround for now is just something like int mode = hvm_guest_x86_mode(curr); uint32_t eax = regs->eax; + if(eax == 0xFACE) { + hvm_event_guest_request(); + return 1; + } + switch ( mode ) { case 8: This way I don't have to worry about if it's a 32 or 64 bit guest. As long as I set EAX to 0xFACE before VMCALL, I can define my own calling convention for the rest of the registers. Might not be what you'd want, but just thought I'd share.
On 08/04/16 22:03, Bread Cutter wrote: > My workaround for now is just something like > > int mode = hvm_guest_x86_mode(curr); > > uint32_t eax = regs->eax; > > > + if(eax == 0xFACE) { > + hvm_event_guest_request(); > + return 1; > + } > + > > switch ( mode ) > { > case 8: > > This way I don't have to worry about if it's a 32 or 64 bit guest. As > long as I set EAX to 0xFACE before VMCALL, I can define my own calling > convention for the rest of the registers. > > Might not be what you'd want, but just thought I'd share. Thanks for sharing. Since there appear to be many people needing / doing these workarounds, this would IMHO justify at least considering the official patch for upstream. Unless someone strongly objects, I'll redo the patch with the modified comparison for 32-bit guests, as suggested by Jan, and submit it tomorrow. Whether it's accepted or not is of course up to the maintainers, but it might be nice to just have an archived patch suggesting how interested parties can achieve this. Thanks, Razvan
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index c1b8392..c7a2cdf 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -5194,8 +5194,14 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) switch ( mode ) { case 8: + if ( eax == __HYPERVISOR_hvm_op && + regs->rdi == HVMOP_guest_request_vm_event ) + break; case 4: case 2: + if ( eax == __HYPERVISOR_hvm_op && + regs->ebx == HVMOP_guest_request_vm_event ) + break; hvm_get_segment_register(curr, x86_seg_ss, &sreg); if ( unlikely(sreg.attr.fields.dpl) ) {