Re: [PATCH V2 2/3] xen/vm_event: Support for guest-requested events

["Jan Beulich" <JBeulich@suse.com>]

>>> On 30.06.15 at 16:23, <rcojocaru@bitdefender.com> wrote:
> On 06/26/2015 10:02 AM, Jan Beulich wrote:
>>>>> On 15.06.15 at 11:03, <rcojocaru@bitdefender.com> wrote:
>>> Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,
>>> sent via HVMOP_request_vm_event. The guest can request that a
>>> generic vm_event (containing only the vm_event-filled guest registers
>>> as information) be sent to userspace by setting up the correct
>>> registers and doing a VMCALL. For example, for a 64-bit guest, this
>>> means: EAX = 34 (hvmop), EBX = 24 (HVMOP_request_vm_event).
>> 
>> I suppose you mean a 32-bit guest here? Also I'm not sure it's a good
>> idea to explicitly define a guest exposed hypercall to omit one of the
>> arguments normally required for it (the interface structure pointer):
>> Should there ever be a reason to allow the guest to control further
>> aspects of the operation by passing a structure, you'd then have to
>> define a new sub-op instead of being able to re-use the current one.
>> I.e. I'd strongly recommend requiring NULL to be passed here, and
>> checking this in the implementation of the handler.
> 
> Would something like this do?
> 
> 6391     case HVMOP_guest_request_vm_event:
> 6392         if ( !guest_handle_is_null(arg) )
> 6393             rc = -EINVAL;
> 6394         else
> 6395             hvm_event_guest_request();
> 6396         break;

Yes, except that I'd recommend inverting the condition and
swapping the branches.

>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -6373,6 +6373,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>          break;
>>>      }
>>>  
>>> +    case HVMOP_request_vm_event:
>>> +        hvm_event_requested();
>>> +        break;
>> 
>> No XSM check here or in the handler? Shouldn't the admin controlling
>> guest properties from the host perspective be permitted control here?
>> Cc-ing Daniel for his input ...
> 
> Thinking more about this, the goal here is to be able to monitor
> non-privileged guests from a privileged domain. Being able to subscribe
> to these events is subject to XSM checks (so an application in dom0
> would be able to receive them), but if XSM checks are needed for the
> guest as well, then, at least for the purpose the code is intended for
> now, the default would need to be to allow this to happen.

Daniel?

Jan