[PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Alexandru Isaila]

In some introspection usecases, an in-guest agent needs to communicate
with the external introspection agent.  An existing mechanism is
HVMOP_guest_request_vm_event, but this is restricted to kernel usecases
like all other hypercalls.

Introduce a mechanism whereby the introspection agent can whitelist the
use of HVMOP_guest_request_vm_event directly from userspace.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

---
Changes since V5:
	- Added the bool allow_userspace to the xc_monitor_guest_request
	 function
---
 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_monitor.c      |  3 ++-
 xen/arch/x86/hvm/hypercall.c  |  5 +++++
 xen/common/monitor.c          |  1 +
 xen/include/asm-x86/domain.h  | 19 ++++++++++---------
 xen/include/public/domctl.h   |  1 +
 6 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index bde8313..a3d0929 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2021,7 +2021,7 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
 int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
                                  bool enable);
 int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
-                             bool enable, bool sync);
+                             bool enable, bool sync, bool allow_userspace);
 int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
                                 bool enable, bool sync);
 int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index b44ce93..a677820 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -147,7 +147,7 @@ int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
 }
 
 int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
-                             bool sync)
+                             bool sync, bool allow_userspace)
 {
     DECLARE_DOMCTL;
 
@@ -157,6 +157,7 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
                                     : XEN_DOMCTL_MONITOR_OP_DISABLE;
     domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST;
     domctl.u.monitor_op.u.guest_request.sync = sync;
+    domctl.u.monitor_op.u.guest_request.allow_userspace = enable ? allow_userspace : false;
 
     return do_domctl(xch, &domctl);
 }
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index e7238ce..5742dd1 100644
--- 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;
+
         if ( unlikely(hvm_get_cpl(curr)) )
         {
     default:
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index 451f42f..20463e0 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
         domain_pause(d);
         d->monitor.guest_request_sync = mop->u.guest_request.sync;
         d->monitor.guest_request_enabled = requested_status;
+        d->arch.monitor.guest_request_userspace_enabled = mop->u.guest_request.allow_userspace;
         domain_unpause(d);
         break;
     }
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index c10522b..de02507 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -396,15 +396,16 @@ struct arch_domain
 
     /* Arch-specific monitor options */
     struct {
-        unsigned int write_ctrlreg_enabled       : 4;
-        unsigned int write_ctrlreg_sync          : 4;
-        unsigned int write_ctrlreg_onchangeonly  : 4;
-        unsigned int singlestep_enabled          : 1;
-        unsigned int software_breakpoint_enabled : 1;
-        unsigned int debug_exception_enabled     : 1;
-        unsigned int debug_exception_sync        : 1;
-        unsigned int cpuid_enabled               : 1;
-        unsigned int descriptor_access_enabled   : 1;
+        unsigned int write_ctrlreg_enabled                                 : 4;
+        unsigned int write_ctrlreg_sync                                    : 4;
+        unsigned int write_ctrlreg_onchangeonly                            : 4;
+        unsigned int singlestep_enabled                                    : 1;
+        unsigned int software_breakpoint_enabled                           : 1;
+        unsigned int debug_exception_enabled                               : 1;
+        unsigned int debug_exception_sync                                  : 1;
+        unsigned int cpuid_enabled                                         : 1;
+        unsigned int descriptor_access_enabled                             : 1;
+        unsigned int guest_request_userspace_enabled                       : 1;
         struct monitor_msr_bitmap *msr_bitmap;
         uint64_t write_ctrlreg_mask[4];
     } monitor;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index ff39762..5997c52 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1124,6 +1124,7 @@ struct xen_domctl_monitor_op {
         struct {
             /* Pause vCPU until response */
             uint8_t sync;
+            uint8_t allow_userspace;
         } guest_request;
 
         struct {
-- 
2.7.4


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

Re: [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Thu, Aug 17, 2017 at 5:50 AM, Alexandru Isaila
<aisaila@bitdefender.com> wrote:
> In some introspection usecases, an in-guest agent needs to communicate
> with the external introspection agent.  An existing mechanism is
> HVMOP_guest_request_vm_event, but this is restricted to kernel usecases
> like all other hypercalls.
>
> Introduce a mechanism whereby the introspection agent can whitelist the
> use of HVMOP_guest_request_vm_event directly from userspace.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

>
> ---
> Changes since V5:
>         - Added the bool allow_userspace to the xc_monitor_guest_request
>          function
> ---
>  tools/libxc/include/xenctrl.h |  2 +-
>  tools/libxc/xc_monitor.c      |  3 ++-
>  xen/arch/x86/hvm/hypercall.c  |  5 +++++
>  xen/common/monitor.c          |  1 +
>  xen/include/asm-x86/domain.h  | 19 ++++++++++---------
>  xen/include/public/domctl.h   |  1 +
>  6 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index bde8313..a3d0929 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2021,7 +2021,7 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
>  int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
>                                   bool enable);
>  int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
> -                             bool enable, bool sync);
> +                             bool enable, bool sync, bool allow_userspace);
>  int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
>                                  bool enable, bool sync);
>  int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index b44ce93..a677820 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -147,7 +147,7 @@ int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
>  }
>
>  int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
> -                             bool sync)
> +                             bool sync, bool allow_userspace)
>  {
>      DECLARE_DOMCTL;
>
> @@ -157,6 +157,7 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
>                                      : XEN_DOMCTL_MONITOR_OP_DISABLE;
>      domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST;
>      domctl.u.monitor_op.u.guest_request.sync = sync;
> +    domctl.u.monitor_op.u.guest_request.allow_userspace = enable ? allow_userspace : false;
>
>      return do_domctl(xch, &domctl);
>  }
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index e7238ce..5742dd1 100644
> --- 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;
> +
>          if ( unlikely(hvm_get_cpl(curr)) )
>          {
>      default:
> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> index 451f42f..20463e0 100644
> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>          domain_pause(d);
>          d->monitor.guest_request_sync = mop->u.guest_request.sync;
>          d->monitor.guest_request_enabled = requested_status;
> +        d->arch.monitor.guest_request_userspace_enabled = mop->u.guest_request.allow_userspace;
>          domain_unpause(d);
>          break;
>      }
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index c10522b..de02507 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -396,15 +396,16 @@ struct arch_domain
>
>      /* Arch-specific monitor options */
>      struct {
> -        unsigned int write_ctrlreg_enabled       : 4;
> -        unsigned int write_ctrlreg_sync          : 4;
> -        unsigned int write_ctrlreg_onchangeonly  : 4;
> -        unsigned int singlestep_enabled          : 1;
> -        unsigned int software_breakpoint_enabled : 1;
> -        unsigned int debug_exception_enabled     : 1;
> -        unsigned int debug_exception_sync        : 1;
> -        unsigned int cpuid_enabled               : 1;
> -        unsigned int descriptor_access_enabled   : 1;
> +        unsigned int write_ctrlreg_enabled                                 : 4;
> +        unsigned int write_ctrlreg_sync                                    : 4;
> +        unsigned int write_ctrlreg_onchangeonly                            : 4;
> +        unsigned int singlestep_enabled                                    : 1;
> +        unsigned int software_breakpoint_enabled                           : 1;
> +        unsigned int debug_exception_enabled                               : 1;
> +        unsigned int debug_exception_sync                                  : 1;
> +        unsigned int cpuid_enabled                                         : 1;
> +        unsigned int descriptor_access_enabled                             : 1;
> +        unsigned int guest_request_userspace_enabled                       : 1;
>          struct monitor_msr_bitmap *msr_bitmap;
>          uint64_t write_ctrlreg_mask[4];
>      } monitor;
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index ff39762..5997c52 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1124,6 +1124,7 @@ struct xen_domctl_monitor_op {
>          struct {
>              /* Pause vCPU until response */
>              uint8_t sync;
> +            uint8_t allow_userspace;
>          } guest_request;
>
>          struct {
> --
> 2.7.4

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

Re: [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Wei Liu]

On Thu, Aug 17, 2017 at 02:50:19PM +0300, Alexandru Isaila wrote:
> In some introspection usecases, an in-guest agent needs to communicate
> with the external introspection agent.  An existing mechanism is
> HVMOP_guest_request_vm_event, but this is restricted to kernel usecases
> like all other hypercalls.
> 
> Introduce a mechanism whereby the introspection agent can whitelist the
> use of HVMOP_guest_request_vm_event directly from userspace.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> 
> ---
> Changes since V5:
> 	- Added the bool allow_userspace to the xc_monitor_guest_request
> 	 function
> ---
>  tools/libxc/include/xenctrl.h |  2 +-
>  tools/libxc/xc_monitor.c      |  3 ++-

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Jan Beulich]

>>> On 17.08.17 at 13:50, <aisaila@bitdefender.com> wrote:
> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>          domain_pause(d);
>          d->monitor.guest_request_sync = mop->u.guest_request.sync;
>          d->monitor.guest_request_enabled = requested_status;
> +        d->arch.monitor.guest_request_userspace_enabled = mop->u.guest_request.allow_userspace;

This breaks the build on ARM.

Jan


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

Re: [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Alexandru Stefan ISAILA]

On Vi, 2017-08-25 at 06:13 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 17.08.17 at 13:50, <aisaila@bitdefender.com> wrote:
> > --- a/xen/common/monitor.c
> > +++ b/xen/common/monitor.c
> > @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct
> > xen_domctl_monitor_op *mop)
> >          domain_pause(d);
> >          d->monitor.guest_request_sync = mop->u.guest_request.sync;
> >          d->monitor.guest_request_enabled = requested_status;
> > +        d->arch.monitor.guest_request_userspace_enabled = mop-
> > >u.guest_request.allow_userspace;
> This breaks the build on ARM.
There are 2 solutions, I can move the case in x86/monitor.c in
the arch_monitor_domctl_event function or I can make a arch specific
function that does the assignment in the x86 case and does nothing in
the arm case. What approach do you prefer?
>
> Jan
>
>
> ________________________
> This email was scanned by Bitdefender

Thanks,
Alex

________________________
This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Jan Beulich]

>>> On 25.08.17 at 15:00, <aisaila@bitdefender.com> wrote:
> On Vi, 2017-08-25 at 06:13 -0600, Jan Beulich wrote:
>> >
>> > >
>> > > >
>> > > > On 17.08.17 at 13:50, <aisaila@bitdefender.com> wrote:
>> > --- a/xen/common/monitor.c
>> > +++ b/xen/common/monitor.c
>> > @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct
>> > xen_domctl_monitor_op *mop)
>> >          domain_pause(d);
>> >          d->monitor.guest_request_sync = mop->u.guest_request.sync;
>> >          d->monitor.guest_request_enabled = requested_status;
>> > +        d->arch.monitor.guest_request_userspace_enabled = mop-
>> > >u.guest_request.allow_userspace;
>> This breaks the build on ARM.
> There are 2 solutions, I can move the case in x86/monitor.c in
> the arch_monitor_domctl_event function or I can make a arch specific
> function that does the assignment in the x86 case and does nothing in
> the arm case. What approach do you prefer?

That's a question to the maintainers of that code. What I care
about is that patches touching common code please are at least
build-checked on the other architecture before submission.

Jan


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

Re: [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Fri, Aug 25, 2017 at 7:44 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 25.08.17 at 15:00, <aisaila@bitdefender.com> wrote:
>> On Vi, 2017-08-25 at 06:13 -0600, Jan Beulich wrote:
>>> >
>>> > >
>>> > > >
>>> > > > On 17.08.17 at 13:50, <aisaila@bitdefender.com> wrote:
>>> > --- a/xen/common/monitor.c
>>> > +++ b/xen/common/monitor.c
>>> > @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct
>>> > xen_domctl_monitor_op *mop)
>>> >          domain_pause(d);
>>> >          d->monitor.guest_request_sync = mop->u.guest_request.sync;
>>> >          d->monitor.guest_request_enabled = requested_status;
>>> > +        d->arch.monitor.guest_request_userspace_enabled = mop-
>>> > >u.guest_request.allow_userspace;
>>> This breaks the build on ARM.
>> There are 2 solutions, I can move the case in x86/monitor.c in
>> the arch_monitor_domctl_event function or I can make a arch specific
>> function that does the assignment in the x86 case and does nothing in
>> the arm case. What approach do you prefer?
>
> That's a question to the maintainers of that code. What I care
> about is that patches touching common code please are at least
> build-checked on the other architecture before submission.
>

Ough, yes, please build-check your code on both architectures before
sending them. As for which route to take I prefer in this case doing
the arch specific function that does the assignment when needed and is
empty when not. The function itself could probably be declared as
static inline too.

Tamas

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

Re: [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[George Dunlap]

On 08/25/2017 02:44 PM, Jan Beulich wrote:
>>>> On 25.08.17 at 15:00, <aisaila@bitdefender.com> wrote:
>> On Vi, 2017-08-25 at 06:13 -0600, Jan Beulich wrote:
>>>>
>>>>>
>>>>>>
>>>>>> On 17.08.17 at 13:50, <aisaila@bitdefender.com> wrote:
>>>> --- a/xen/common/monitor.c
>>>> +++ b/xen/common/monitor.c
>>>> @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct
>>>> xen_domctl_monitor_op *mop)
>>>>          domain_pause(d);
>>>>          d->monitor.guest_request_sync = mop->u.guest_request.sync;
>>>>          d->monitor.guest_request_enabled = requested_status;
>>>> +        d->arch.monitor.guest_request_userspace_enabled = mop-
>>>>> u.guest_request.allow_userspace;
>>> This breaks the build on ARM.
>> There are 2 solutions, I can move the case in x86/monitor.c in
>> the arch_monitor_domctl_event function or I can make a arch specific
>> function that does the assignment in the x86 case and does nothing in
>> the arm case. What approach do you prefer?
> 
> That's a question to the maintainers of that code. What I care
> about is that patches touching common code please are at least
> build-checked on the other architecture before submission.

I don't think this is a reasonable requirement.  That's what push gates
(and Travis) are for.

 -George

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

Re: [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Jan Beulich]

>>> On 25.08.17 at 17:49, <george.dunlap@citrix.com> wrote:
> On 08/25/2017 02:44 PM, Jan Beulich wrote:
>>>>> On 25.08.17 at 15:00, <aisaila@bitdefender.com> wrote:
>>> On Vi, 2017-08-25 at 06:13 -0600, Jan Beulich wrote:
>>>>>
>>>>>>
>>>>>>>
>>>>>>> On 17.08.17 at 13:50, <aisaila@bitdefender.com> wrote:
>>>>> --- a/xen/common/monitor.c
>>>>> +++ b/xen/common/monitor.c
>>>>> @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct
>>>>> xen_domctl_monitor_op *mop)
>>>>>          domain_pause(d);
>>>>>          d->monitor.guest_request_sync = mop->u.guest_request.sync;
>>>>>          d->monitor.guest_request_enabled = requested_status;
>>>>> +        d->arch.monitor.guest_request_userspace_enabled = mop-
>>>>>> u.guest_request.allow_userspace;
>>>> This breaks the build on ARM.
>>> There are 2 solutions, I can move the case in x86/monitor.c in
>>> the arch_monitor_domctl_event function or I can make a arch specific
>>> function that does the assignment in the x86 case and does nothing in
>>> the arm case. What approach do you prefer?
>> 
>> That's a question to the maintainers of that code. What I care
>> about is that patches touching common code please are at least
>> build-checked on the other architecture before submission.
> 
> I don't think this is a reasonable requirement.  That's what push gates
> (and Travis) are for.

Hmm, I can see your way of thinking, but to me (doing an ARM
build test if I don't forget it before pushing) it's a waste of time
to apply a patch only to then find it breaks the build and needs
reverting.

Jan


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