| Message ID | 1502180855-7500-1-git-send-email-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 08, 2017 at 11:27:35AM +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 V4: > - Changed function mane from xc_allow_guest_userspace_event > to xc_monitor_guest_userspace_event > - Fixed guest_request_enabled check > - Delete the guest_request_sync > - Changed guest_request_userspace_event to > guest_request_userspace_enabled > - Moved guest_request_userspace_enabled flag from sched.h to > domain.h > --- > tools/libxc/include/xenctrl.h | 1 + > tools/libxc/xc_monitor.c | 14 ++++++++++++++ > xen/arch/x86/hvm/hypercall.c | 5 +++++ > xen/common/monitor.c | 13 +++++++++++++ > xen/include/asm-x86/domain.h | 19 ++++++++++--------- > xen/include/public/domctl.h | 21 +++++++++++---------- > 6 files changed, 54 insertions(+), 19 deletions(-) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index bde8313..c72e12d 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_monitor_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..bd8cbcf 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_monitor_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); > +} > + > + For this bit: Acked-by: Wei Liu <wei.liu2@citrix.com> Some nits below. > 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; Indentation here and below seems rather excessive.
On Ma, 2017-08-08 at 12:27 +0100, Wei Liu wrote: > On Tue, Aug 08, 2017 at 11:27:35AM +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 V4: > > - Changed function mane from xc_allow_guest_userspace_event > > to xc_monitor_guest_userspace_event > > - Fixed guest_request_enabled check > > - Delete the guest_request_sync > > - Changed guest_request_userspace_event to > > guest_request_userspace_enabled > > - Moved guest_request_userspace_enabled flag from sched.h to > > domain.h > > --- > > tools/libxc/include/xenctrl.h | 1 + > > tools/libxc/xc_monitor.c | 14 ++++++++++++++ > > xen/arch/x86/hvm/hypercall.c | 5 +++++ > > xen/common/monitor.c | 13 +++++++++++++ > > xen/include/asm-x86/domain.h | 19 ++++++++++--------- > > xen/include/public/domctl.h | 21 +++++++++++---------- > > 6 files changed, 54 insertions(+), 19 deletions(-) > > > > diff --git a/tools/libxc/include/xenctrl.h > > b/tools/libxc/include/xenctrl.h > > index bde8313..c72e12d 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_monitor_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..bd8cbcf 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_monitor_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); > > +} > > + > > + > For this bit: > > Acked-by: Wei Liu <wei.liu2@citrix.com> > > Some nits below. > > > > > 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; > Indentation here and below seems rather excessive. This indentation was a suggestion made by Jan Beulich on Patch V3. > > ________________________ > This email was scanned by Bitdefender
On Tue, Aug 8, 2017 at 2:27 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 V4: > - Changed function mane from xc_allow_guest_userspace_event > to xc_monitor_guest_userspace_event > - Fixed guest_request_enabled check > - Delete the guest_request_sync > - Changed guest_request_userspace_event to > guest_request_userspace_enabled > - Moved guest_request_userspace_enabled flag from sched.h to > domain.h > --- > tools/libxc/include/xenctrl.h | 1 + > tools/libxc/xc_monitor.c | 14 ++++++++++++++ > xen/arch/x86/hvm/hypercall.c | 5 +++++ > xen/common/monitor.c | 13 +++++++++++++ > xen/include/asm-x86/domain.h | 19 ++++++++++--------- > xen/include/public/domctl.h | 21 +++++++++++---------- > 6 files changed, 54 insertions(+), 19 deletions(-) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index bde8313..c72e12d 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_monitor_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..bd8cbcf 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_monitor_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..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; > + So the CPL check happens after the monitor check, which means this will trigger regardless if the hypercall is coming from userspace or kernelspace. Since the monitor option specifically says userspace, this should probably get moved into the block where CPL was checked. > if ( unlikely(hvm_get_cpl(curr)) ) > { > default: > diff --git a/xen/common/monitor.c b/xen/common/monitor.c > index 451f42f..080a405 100644 > --- a/xen/common/monitor.c > +++ b/xen/common/monitor.c > @@ -79,6 +79,19 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > break; > } > > + case XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_EVENT: > + { > + bool old_status = d->arch.monitor.guest_request_userspace_enabled; > + > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > + > + domain_pause(d); > + d->arch.monitor.guest_request_userspace_enabled = 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/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..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_* */ > -- > 2.7.4
>>> On 14.08.17 at 17:53, <tamas@tklengyel.com> wrote: > On Tue, Aug 8, 2017 at 2:27 AM, Alexandru Isaila <aisaila@bitdefender.com> wrote: >> --- 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; >> + > > So the CPL check happens after the monitor check, which means this > will trigger regardless if the hypercall is coming from userspace or > kernelspace. Since the monitor option specifically says userspace, > this should probably get moved into the block where CPL was checked. What difference would this make? For CPL0 the hypercall is permitted anyway, and for CPL > 0 we specifically want to bypass the CPL check. Or are you saying you want to restrict the new check to just CPL3? Jan
On Tue, Aug 15, 2017 at 2:06 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 14.08.17 at 17:53, <tamas@tklengyel.com> wrote: >> On Tue, Aug 8, 2017 at 2:27 AM, Alexandru Isaila <aisaila@bitdefender.com> wrote: >>> --- 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; >>> + >> >> So the CPL check happens after the monitor check, which means this >> will trigger regardless if the hypercall is coming from userspace or >> kernelspace. Since the monitor option specifically says userspace, >> this should probably get moved into the block where CPL was checked. > > What difference would this make? For CPL0 the hypercall is > permitted anyway, and for CPL > 0 we specifically want to bypass > the CPL check. Or are you saying you want to restrict the new > check to just CPL3? > Yes, according to the name of this monitor option this should only trigger a vm_event when the hypercall is coming from CPL3. However, the way it is implemented right now I see that this monitor option actually requires the other one to be enabled too. By itself this monitor option will not work. So I would also like to ask that the check in xen/common/monitor.c, if ( d->monitor.guest_request_enabled ), to be extended to be: if ( d->monitor.guest_request_enabled || d->monitor.guest_request_userspace_enabled ) Tamas
On 08/16/2017 02:16 AM, Tamas K Lengyel wrote: > On Tue, Aug 15, 2017 at 2:06 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 14.08.17 at 17:53, <tamas@tklengyel.com> wrote: >>> On Tue, Aug 8, 2017 at 2:27 AM, Alexandru Isaila <aisaila@bitdefender.com> wrote: >>>> --- 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; >>>> + >>> >>> So the CPL check happens after the monitor check, which means this >>> will trigger regardless if the hypercall is coming from userspace or >>> kernelspace. Since the monitor option specifically says userspace, >>> this should probably get moved into the block where CPL was checked. >> >> What difference would this make? For CPL0 the hypercall is >> permitted anyway, and for CPL > 0 we specifically want to bypass >> the CPL check. Or are you saying you want to restrict the new >> check to just CPL3? >> > > Yes, according to the name of this monitor option this should only > trigger a vm_event when the hypercall is coming from CPL3. However, > the way it is implemented right now I see that this monitor option > actually requires the other one to be enabled too. By itself this > monitor option will not work. So I would also like to ask that the > check in xen/common/monitor.c, if ( d->monitor.guest_request_enabled > ), to be extended to be: if ( d->monitor.guest_request_enabled || > d->monitor.guest_request_userspace_enabled ) The option does not trigger anything. Its job is to allow guest requests coming from userspace (via VMCALLs). And not to _only_ allow these for userspace, but to allow them coming from userspace _as_well_. The current version of the patch, if I've not missed something, does not require d->monitor.guest_request_enabled to be true to work (the options can be toggled independently). The new function is meant to be called at any time, independent of enabling / disabling the guest request vm_event (i.e. it only controls its behaviour once it's enabled). So guest_request_userspace_enabled should not be used as synonym for guest_request_enabled. Thanks, Razvan
On Wed, Aug 16, 2017 at 6:43 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 16.08.2017 15:32, Tamas K Lengyel wrote: >> >> On Wed, Aug 16, 2017 at 12:07 AM, Razvan Cojocaru >> <rcojocaru@bitdefender.com> wrote: >>> >>> On 08/16/2017 02:16 AM, Tamas K Lengyel wrote: >>>> >>>> On Tue, Aug 15, 2017 at 2:06 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> >>>>>>>> On 14.08.17 at 17:53, <tamas@tklengyel.com> wrote: >>>>>> >>>>>> On Tue, Aug 8, 2017 at 2:27 AM, Alexandru Isaila >>>>>> <aisaila@bitdefender.com> wrote: >>>>>>> >>>>>>> --- 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; >>>>>>> + >>>>>> >>>>>> >>>>>> So the CPL check happens after the monitor check, which means this >>>>>> will trigger regardless if the hypercall is coming from userspace or >>>>>> kernelspace. Since the monitor option specifically says userspace, >>>>>> this should probably get moved into the block where CPL was checked. >>>>> >>>>> >>>>> What difference would this make? For CPL0 the hypercall is >>>>> permitted anyway, and for CPL > 0 we specifically want to bypass >>>>> the CPL check. Or are you saying you want to restrict the new >>>>> check to just CPL3? >>>>> >>>> >>>> Yes, according to the name of this monitor option this should only >>>> trigger a vm_event when the hypercall is coming from CPL3. However, >>>> the way it is implemented right now I see that this monitor option >>>> actually requires the other one to be enabled too. By itself this >>>> monitor option will not work. So I would also like to ask that the >>>> check in xen/common/monitor.c, if ( d->monitor.guest_request_enabled >>>> ), to be extended to be: if ( d->monitor.guest_request_enabled || >>>> d->monitor.guest_request_userspace_enabled ) >>> >>> >>> The option does not trigger anything. Its job is to allow guest requests >>> coming from userspace (via VMCALLs). And not to _only_ allow these for >>> userspace, but to allow them coming from userspace _as_well_. >>> >>> The current version of the patch, if I've not missed something, does not >>> require d->monitor.guest_request_enabled to be true to work (the options >>> can be toggled independently). >>> >>> The new function is meant to be called at any time, independent of >>> enabling / disabling the guest request vm_event (i.e. it only controls >>> its behaviour once it's enabled). So guest_request_userspace_enabled >>> should not be used as synonym for guest_request_enabled. >>> >> >> Hi Razvan, >> so while monitor.guest_request_enabled can indeed be toggled >> independently, if it is not set, how would the vm_event actually be >> sent for monitor.guest_request_userspace_enabled? AFAICT it wouldn't >> because the code responsible for sending the vm_event is gated only on >> monitor.guest_request_enabled. > > > Hello, indeed it wouldn't. > > This new option only control what happens _if_ monitor.guest_request_enabled > is true. While monitor.guest_request_enabled is false, it has no effect. As > soon as guest_request_enabled gets toggled on again, the behaviour will be > the one that has been previously set by using > guest_request_userspace_enabled. > >> And as for this new option being an extended version of >> guest_request_enabled in the sense that it would allow both userspace >> _and_ kernelspace, we can do that, but then the name of it is >> misleading. > > > The naming of the function did get shuffled around a bit. :) > > Would xc_monitor_allow_guest_userspace_event() be more appropriate? > > Also, if having a separate function feels counter-intuitive, we could also > simply add a bool parameter to xc_monitor_guest_request(), for example: > > int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, > bool enable, bool sync, > bool allow_userspace); > > and use that to toggle guest_request_userspace_enabled. In light of the conversion here I have to say I like this option the most. > > Thanks, > Razvan > > P.S. Is replying only to me (as opposed to a "reply all") intentional? Oops, no, I've re-added xen-devel :) Tamas
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index bde8313..c72e12d 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_monitor_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..bd8cbcf 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_monitor_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..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..080a405 100644 --- a/xen/common/monitor.c +++ b/xen/common/monitor.c @@ -79,6 +79,19 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) break; } + case XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_EVENT: + { + bool old_status = d->arch.monitor.guest_request_userspace_enabled; + + if ( unlikely(old_status == requested_status) ) + return -EEXIST; + + domain_pause(d); + d->arch.monitor.guest_request_userspace_enabled = 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/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..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_* */
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 V4: - Changed function mane from xc_allow_guest_userspace_event to xc_monitor_guest_userspace_event - Fixed guest_request_enabled check - Delete the guest_request_sync - Changed guest_request_userspace_event to guest_request_userspace_enabled - Moved guest_request_userspace_enabled flag from sched.h to domain.h --- tools/libxc/include/xenctrl.h | 1 + tools/libxc/xc_monitor.c | 14 ++++++++++++++ xen/arch/x86/hvm/hypercall.c | 5 +++++ xen/common/monitor.c | 13 +++++++++++++ xen/include/asm-x86/domain.h | 19 ++++++++++--------- xen/include/public/domctl.h | 21 +++++++++++---------- 6 files changed, 54 insertions(+), 19 deletions(-)