Xen project Mailing List

From: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
Sent: Saturday, August 5, 2017 4:32 AM
To: Alexandru Stefan ISAILA
Cc: Xen-devel; wei.liu2@xxxxxxxxxx; 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>

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

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