HVMOP_guest_request_vm_event only works from guest in ring0

Commit Message

Andrew Cooper Aug. 3, 2016, 8:44 p.m. UTC

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:


~Andrew

Comments

Razvan Cojocaru Aug. 3, 2016, 9 p.m. UTC | #1

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

Andrew Cooper Aug. 3, 2016, 9:03 p.m. UTC | #2

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

Jan Beulich Aug. 4, 2016, 7:13 a.m. UTC | #3

>>> On 03.08.16 at 22:44, <[e-mail]> 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

Jan Beulich Aug. 4, 2016, 7:23 a.m. UTC | #4

>>> On 03.08.16 at 23:00, <[e-mail]> 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

Andrew Cooper Aug. 4, 2016, 9:24 a.m. UTC | #5

On 04/08/16 08:13, Jan Beulich wrote:
>>>> On 03.08.16 at 22:44, <[e-mail]> 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

Jan Beulich Aug. 4, 2016, 9:31 a.m. UTC | #6

>>> On 04.08.16 at 11:24, <[e-mail]> wrote:
> On 04/08/16 08:13, Jan Beulich wrote:
>>>>> On 03.08.16 at 22:44, <[e-mail]> 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

Andrew Cooper Aug. 4, 2016, 9:32 a.m. UTC | #7

On 04/08/16 08:23, Jan Beulich wrote:
>>>> On 03.08.16 at 23:00, <[e-mail]> 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

Jan Beulich Aug. 4, 2016, 10:02 a.m. UTC | #8

>>> On 04.08.16 at 11:32, <[e-mail]> wrote:
> On 04/08/16 08:23, Jan Beulich wrote:
>>>>> On 03.08.16 at 23:00, <[e-mail]> 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

Bread Cutter Aug. 4, 2016, 7:03 p.m. UTC | #9

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.

Razvan Cojocaru Aug. 4, 2016, 7:19 p.m. UTC | #10

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

Patch

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) )
{