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

[Alexandru Stefan ISAILA <aisaila@bitdefender.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8162 bytes --]

​

________________________________
From: Tamas K Lengyel <tamas@tklengyel.com>
Sent: Saturday, August 5, 2017 4:32 AM
To: Alexandru Stefan ISAILA
Cc: Xen-devel; wei.liu2@citrix.com; Tim Deegan; Stefano Stabellini; Konrad Rzeszutek Wilk; Jan Beulich; Ian Jackson; George Dunlap; Andrew Cooper; Razvan Cojocaru
Subject: Re: [PATCH v4] x86/hvm: Allow guest_request vm_events coming from userspace



On Fri, Aug 4, 2017 at 5:32 AM, Alexandru Isaila <aisaila@bitdefender.com<mailto: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<mailto:aisaila@bitdefender.com>>

---
Changes since V3:
        - Changed commit message
        - Added new lines
        - Indent the maximum space on the defines
        - Chaned the name of the define/function name/struct member
          from vmcall to event
---
 tools/libxc/include/xenctrl.h |  1 +
 tools/libxc/xc_monitor.c      | 14 ++++++++++++++
 xen/arch/x86/hvm/hypercall.c  |  5 +++++
 xen/common/monitor.c          | 14 ++++++++++++++
 xen/include/public/domctl.h   | 21 +++++++++++----------
 xen/include/xen/sched.h       |  5 +++--
 6 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index bde8313..90a056f 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2022,6 +2022,7 @@ 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);
+int xc_allow_guest_userspace_event(xc_interface *xch, domid_t domain_id, bool enable);
 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..6064c39 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -161,6 +161,20 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
     return do_domctl(xch, &domctl);
 }

+int xc_allow_guest_userspace_event(xc_interface *xch, domid_t domain_id, bool enable)

This function should be prefixed with "xc_monitor_" like all the rest of the functions here.

+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_monitor_op;
+    domctl.domain = domain_id;
+    domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
+    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_EVENT;
+
+    return do_domctl(xch, &domctl);
+}
+
+
 int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
                                 bool enable)
 {
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index e7238ce..8eb5f49 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->monitor.guest_request_userspace_event &&
+            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..21a1457 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -79,6 +79,20 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
         break;
     }

+    case XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_EVENT:
+    {
+        bool_t old_status = d->monitor.guest_request_enabled;

You are checking guest_request enabled here while later setting guest_request_userspace_event. These are two separate monitor options, adjust accordingly.

+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        d->monitor.guest_request_sync = mop->u.guest_request.sync;

You are setting guest_request_sync here which is a setting belonging to guest_request_enabled. If you need sync/async option for the userspace guest request it should be a separate bit. Since the toolstack side you add in this patch never sets the sync option I assume that is not the case, so remove this.

+        d->monitor.guest_request_userspace_event = requested_status;
+        domain_unpause(d);
+        break;
+    }
+
     default:
         /* Give arch-side the chance to handle this event */
         return arch_monitor_domctl_event(d, mop);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index ff39762..870495c 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1073,16 +1073,17 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES  2
 #define XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP  3

-#define XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG         0
-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR            1
-#define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            2
-#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   3
-#define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST         4
-#define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION       5
-#define XEN_DOMCTL_MONITOR_EVENT_CPUID                 6
-#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL       7
-#define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT             8
-#define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS           9
+#define XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG                                0
+#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR                                   1
+#define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP                                   2
+#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT                          3
+#define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST                                4
+#define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION                              5
+#define XEN_DOMCTL_MONITOR_EVENT_CPUID                                        6
+#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL                              7
+#define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT                                    8
+#define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS                                  9
+#define XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_EVENT                       10

 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 6673b27..898a132 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -480,8 +480,9 @@ struct domain

     /* Common monitor options */
     struct {
-        unsigned int guest_request_enabled       : 1;
-        unsigned int guest_request_sync          : 1;
+        unsigned int guest_request_enabled                                 : 1;
+        unsigned int guest_request_sync                                    : 1;
+        unsigned int guest_request_userspace_event                         : 1;

This should be "guest_request_userspace_enabled". It also should not be in xen/sched.h as on ARM there is no instruction trapping from userspace directly to the hypervisor (AFAIK).

Is everyone ok with moving the bit in a x86 specific region?

     } monitor;
 };

--
2.7.4

Tamas

Regards,
Alex

________________________________
This email was scanned by Bitdefender

[-- Attachment #1.2: Type: text/html, Size: 14733 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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