Re: [Xen-devel] [PATCH v5] x86/hvm: Allow guest_request vm_events coming from userspace

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 08/16/2017 02:16 AM, Tamas K Lengyel wrote:
> On Tue, Aug 15, 2017 at 2:06 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 14.08.17 at 17:53, <tamas@xxxxxxxxxxxxx> wrote:
>>> On Tue, Aug 8, 2017 at 2:27 AM, Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx> 
>>> wrote:
>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>> @@ -155,6 +155,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>>>>          /* Fallthrough to permission check. */
>>>>      case 4:
>>>>      case 2:
>>>> +        if ( currd->arch.monitor.guest_request_userspace_enabled &&
>>>> +            eax == __HYPERVISOR_hvm_op &&
>>>> +            (mode == 8 ? regs->rdi : regs->ebx) == 
>>>> HVMOP_guest_request_vm_event )
>>>> +            break;
>>>> +
>>>
>>> So the CPL check happens after the monitor check, which means this
>>> will trigger regardless if the hypercall is coming from userspace or
>>> kernelspace. Since the monitor option specifically says userspace,
>>> this should probably get moved into the block where CPL was checked.
>>
>> What difference would this make? For CPL0 the hypercall is
>> permitted anyway, and for CPL > 0 we specifically want to bypass
>> the CPL check. Or are you saying you want to restrict the new
>> check to just CPL3?
>>
> 
> Yes, according to the name of this monitor option this should only
> trigger a vm_event when the hypercall is coming from CPL3. However,
> the way it is implemented right now I see that this monitor option
> actually requires the other one to be enabled too. By itself this
> monitor option will not work. So I would also like to ask that the
> check in xen/common/monitor.c, if ( d->monitor.guest_request_enabled
> ), to be extended to be: if ( d->monitor.guest_request_enabled ||
> d->monitor.guest_request_userspace_enabled )

The option does not trigger anything. Its job is to allow guest requests
coming from userspace (via VMCALLs). And not to _only_ allow these for
userspace, but to allow them coming from userspace _as_well_.

The current version of the patch, if I've not missed something, does not
require d->monitor.guest_request_enabled to be true to work (the options
can be toggled independently).

The new function is meant to be called at any time, independent of
enabling / disabling the guest request vm_event (i.e. it only controls
its behaviour once it's enabled). So guest_request_userspace_enabled
should not be used as synonym for guest_request_enabled.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel