| Message ID | 1501846324-4683-1-git-send-email-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/08/17 12:32, 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> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one note to whoever commits it... > 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; bool. ~Andrew
On Fri, Aug 4, 2017 at 5:32 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> > > --- > 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). > } monitor; > }; > > -- > 2.7.4 > Tamas
On 08/05/2017 04:32 AM, Tamas K Lengyel wrote: > > > 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. That one was my suggestion, as I thought xc_monitor_-prefixed functions are meant to toggle monitoring somehow, whereas this function only toggles userspace use of guest request VMCALLs. But re-adding the prefix is fine - since toggling it only influences in the end how a vm_event is sent, it is after all monitor-related. Thanks for the review, Razvan
On Sat, Aug 5, 2017 at 2:18 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 08/05/2017 04:32 AM, Tamas K Lengyel wrote: > > > > > > 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. > That one was my suggestion, as I thought xc_monitor_-prefixed functions > are meant to toggle monitoring somehow, whereas this function only > toggles userspace use of guest request VMCALLs. > So it wasn't exactly clear whether this is just an option on the pre-existing guest request monitor like sync or a completely new, separate monitor option on its own. It looks to me like it is a separate option so let's treat it as such. Tamas
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) +{ + 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; + + if ( unlikely(old_status == requested_status) ) + return -EEXIST; + + domain_pause(d); + d->monitor.guest_request_sync = mop->u.guest_request.sync; + 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; } monitor; };
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 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(-)