On Fri, Aug 4, 2017 at 5:32 AM, Alexandru Isaila <aisa...@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 <aisa...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
