Re: [Xen-devel] [PATCH] x86/monitor: add support for descriptor access events
On Tue, 21 Mar 2017 12:04:02 + Andrew Cooper <andrew.coop...@citrix.com> wrote: > On 10/03/17 15:50, Vlad Ioan Topan wrote: > > Adds monitor support for descriptor access events (reads & writes of > > IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM). > > > > Signed-off-by: Vlad Ioan Topan <ito...@bitdefender.com> > > How much extra overhead does this typically give? (I am curious, more > than anything else) Without a monitor attached, none at all; with a monitor attached and with exits activated, there are probably a few hundred exits during the OS startup process; during normal OS runtime there should be no descriptor register access. The other suggestions will be incorporated by my colleagues who will take over the patch(es). Thank you, -- Vlad-Ioan TOPAN Linux Kernel Development Lead Bitdefender ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/monitor: add support for descriptor access events
nclude/public/domctl.h
> > @@ -1087,6 +1087,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
> > #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
>
> ... the sequence here.
Ok.
> > @@ -259,6 +261,24 @@ struct vm_event_mov_to_msr {
> > uint64_t value;
> > };
> >
> > +#define VM_EVENT_DESC_INVALID0
>
> What is this good for?
The default (uninitialized) value is given a semantic of "invalid" to
make potential problems due to incorrectly / incompletely initialized
or corrupted data more obvious (I assume the .pad* fields are checked to
be 0 for the same reason).
> Jan
Thank you for the review; pending further feedback on the
X86EMUL_UNHANDLEABLE case I'll post an updated patch.
--
Vlad-Ioan TOPAN
Linux Kernel Development Lead
Bitdefender
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/monitor: add support for descriptor access events
On Tue, 14 Mar 2017 09:15:04 -0400 Boris Ostrovsky <boris.ostrov...@oracle.com> wrote: > > > On 03/14/2017 08:50 AM, Razvan Cojocaru wrote: > > On 03/14/2017 02:15 PM, Vlad-Ioan TOPAN wrote: > >>>> @@ -2642,6 +2660,38 @@ void svm_vmexit_handler(struct cpu_user_regs > >>>> *regs) > >>>> case VMEXIT_PAUSE: > >>>> svm_vmexit_do_pause(regs); > >>>> break; > >>>> + > >>>> +case VMEXIT_IDTR_READ: > >>>> +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > >>>> VM_EVENT_DESC_IDTR, 0); > >>>> +break; > >>>> + > >>>> +case VMEXIT_GDTR_READ: > >>>> +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > >>>> VM_EVENT_DESC_GDTR, 0); > >>>> +break; > >>>> + > >>>> +case VMEXIT_LDTR_READ: > >>>> +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > >>>> VM_EVENT_DESC_LDTR, 0); > >>>> +break; > >>>> + > >>>> +case VMEXIT_TR_READ: > >>>> +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > >>>> VM_EVENT_DESC_TR, 0); > >>>> +break; > >>>> + > >>>> +case VMEXIT_IDTR_WRITE: > >>>> +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > >>>> VM_EVENT_DESC_IDTR, 1); > >>>> +break; > >>>> + > >>>> +case VMEXIT_GDTR_WRITE: > >>>> +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > >>>> VM_EVENT_DESC_GDTR, 1); > >>>> +break; > >>>> + > >>>> +case VMEXIT_LDTR_WRITE: > >>>> +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > >>>> VM_EVENT_DESC_LDTR, 1); > >>>> +break; > >>>> + > >>>> +case VMEXIT_TR_WRITE: > >>>> +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > >>>> VM_EVENT_DESC_TR, 1); > >>>> +break; > >>> > >>> I think this can be halved in size by having > >>> 'hvm_descriptor_access_intercept(..., exit_reason_write?1:0)' > >>> > >>> And maybe even collapse completely by having a lookup table mapping exit > >>> reason to event. > >> > >> The problem with both ideas is that they depend on assumptions about the > >> values of the VMEXIT_* constants to make the code shorter and still > >> keep it readable, which in my opinion would be bad. Although they will > >> most likely stay sequential and keep their current numeric values, it's > >> not something I'd hardcode. Without those assumptions, it's either > >> another switch or a very long if, which would mean roughly the same > >> amount of code, but less readable (it's the way I've written it > >> initally before coming to this version). > > > > I'm reading Boris' suggestion to mean: > > > > case VMEXIT_IDTR_READ: > > case VMEXIT_IDTR_WRITE: > > hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > > VM_EVENT_DESC_IDTR, exit_reason == VMEXIT_IDTR_WRITE); > > break; > > > > I could be wrong. > > Right, that's exactly what I meant, thanks. That's the "roughly same amount of code, but less readable" option, but it works for me if that's the consensus. > As for getting rid of all but one cases --- yes, it may be a a bit > tricky to do it in a reasonably compact manner. > > -boris Okay then, I'll post v2 of the patch with the two suggested changes. -- Vlad-Ioan TOPAN Linux Kernel Development Lead Bitdefender ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/monitor: add support for descriptor access events
> > @@ -2642,6 +2660,38 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > > case VMEXIT_PAUSE: > > svm_vmexit_do_pause(regs); > > break; > > + > > +case VMEXIT_IDTR_READ: > > +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > > VM_EVENT_DESC_IDTR, 0); > > +break; > > + > > +case VMEXIT_GDTR_READ: > > +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > > VM_EVENT_DESC_GDTR, 0); > > +break; > > + > > +case VMEXIT_LDTR_READ: > > +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > > VM_EVENT_DESC_LDTR, 0); > > +break; > > + > > +case VMEXIT_TR_READ: > > +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > > VM_EVENT_DESC_TR, 0); > > +break; > > + > > +case VMEXIT_IDTR_WRITE: > > +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > > VM_EVENT_DESC_IDTR, 1); > > +break; > > + > > +case VMEXIT_GDTR_WRITE: > > +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > > VM_EVENT_DESC_GDTR, 1); > > +break; > > + > > +case VMEXIT_LDTR_WRITE: > > +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > > VM_EVENT_DESC_LDTR, 1); > > +break; > > + > > +case VMEXIT_TR_WRITE: > > +hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, > > VM_EVENT_DESC_TR, 1); > > +break; > > I think this can be halved in size by having > 'hvm_descriptor_access_intercept(..., exit_reason_write?1:0)' > > And maybe even collapse completely by having a lookup table mapping exit > reason to event. The problem with both ideas is that they depend on assumptions about the values of the VMEXIT_* constants to make the code shorter and still keep it readable, which in my opinion would be bad. Although they will most likely stay sequential and keep their current numeric values, it's not something I'd hardcode. Without those assumptions, it's either another switch or a very long if, which would mean roughly the same amount of code, but less readable (it's the way I've written it initally before coming to this version). -- Vlad-Ioan TOPAN Linux Kernel Development Lead Bitdefender ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/monitor: add support for descriptor access events
> > +struct vm_event_desc_access {
> > +union {
> > +uint32_t vmx_instr_info;/* VMX: VMCS Instruction-Information
> > Field */
> > +uint64_t svm_exitinfo; /* SVM: VMCB EXITINFO Field */
> > +uint64_t info;
> > +} vmexit;
> > +uint64_t exit_qualification;/* VMX only: VMCS Exit Qualification
> > Field */
>
> IMHO it might be a bit cleaner if we had two sub-structs here, one for
> vmx and one for svm, and have them in a union. The vmx struct would
> have vmx_instr_info and exit_qualification, while the svm would only
> have svm_exitinfo.
Makes sense, will do.
--
Vlad-Ioan TOPAN
Linux Kernel Development Lead
Bitdefender
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH] x86/monitor: add support for descriptor access events
Adds monitor support for descriptor access events (reads & writes of
IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM).
Signed-off-by: Vlad Ioan Topan <ito...@bitdefender.com>
---
tools/libxc/include/xenctrl.h | 2 ++
tools/libxc/xc_monitor.c| 14 +++
tools/tests/xen-access/xen-access.c | 29 -
xen/arch/x86/hvm/hvm.c | 35 ++
xen/arch/x86/hvm/monitor.c | 16
xen/arch/x86/hvm/svm/svm.c | 50 +
xen/arch/x86/hvm/vmx/vmx.c | 45 +
xen/arch/x86/monitor.c | 18 +
xen/arch/x86/vm_event.c | 6 +
xen/include/asm-x86/domain.h| 1 +
xen/include/asm-x86/hvm/hvm.h | 2 ++
xen/include/asm-x86/hvm/monitor.h | 3 +++
xen/include/asm-x86/hvm/support.h | 2 ++
xen/include/asm-x86/monitor.h | 1 +
xen/include/public/domctl.h | 1 +
xen/include/public/vm_event.h | 21
16 files changed, 245 insertions(+), 1 deletion(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index a48981a..2de6c61 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1984,6 +1984,8 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t
domain_id, uint32_t msr,
int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
bool enable);
+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_debug_exceptions(xc_interface *xch, domid_t domain_id,
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 15a7c32..f99b6e3 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -129,6 +129,20 @@ int xc_monitor_singlestep(xc_interface *xch, domid_t
domain_id,
return do_domctl(xch, );
}
+int xc_monitor_descriptor_access(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_DESC_ACCESS;
+
+return do_domctl(xch, );
+}
+
int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
bool sync)
{
diff --git a/tools/tests/xen-access/xen-access.c
b/tools/tests/xen-access/xen-access.c
index 9d4f957..c567cc0 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -337,7 +337,7 @@ void usage(char* progname)
{
fprintf(stderr, "Usage: %s [-m] write|exec", progname);
#if defined(__i386__) || defined(__x86_64__)
-fprintf(stderr,
"|breakpoint|altp2m_write|altp2m_exec|debug|cpuid");
+fprintf(stderr,
"|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access");
#elif defined(__arm__) || defined(__aarch64__)
fprintf(stderr, "|privcall");
#endif
@@ -368,6 +368,7 @@ int main(int argc, char *argv[])
int altp2m = 0;
int debug = 0;
int cpuid = 0;
+int desc_access = 0;
uint16_t altp2m_view_id = 0;
char* progname = argv[0];
@@ -434,6 +435,10 @@ int main(int argc, char *argv[])
{
cpuid = 1;
}
+else if ( !strcmp(argv[0], "desc_access") )
+{
+desc_access = 1;
+}
#elif defined(__arm__) || defined(__aarch64__)
else if ( !strcmp(argv[0], "privcall") )
{
@@ -570,6 +575,16 @@ int main(int argc, char *argv[])
goto exit;
}
}
+
+if ( desc_access )
+{
+rc = xc_monitor_descriptor_access(xch, domain_id, 1);
+if ( rc < 0 )
+{
+ERROR("Error %d setting descriptor access listener with
vm_event\n", rc);
+goto exit;
+}
+}
if ( privcall )
{
@@ -595,6 +610,8 @@ int main(int argc, char *argv[])
rc = xc_monitor_debug_exceptions(xch, domain_id, 0, 0);
if ( cpuid )
rc = xc_monitor_cpuid(xch, domain_id, 0);
+if ( desc_access )
+rc = xc_monitor_descriptor_access(xch, domain_id, 0);
if ( privcall )
rc = xc_monitor_privileged_call(xch, domain_id, 0);
@@ -779,6 +796,16 @@ int main(int argc, char *argv[])
rsp.data = req.data;
rsp.data.regs.x86.rip += req.u.cpuid.insn_length;
break;
+case VM_E
Re: [Xen-devel] Enabling #VE for a domain from dom0
> > Is there any reason for the other check I've mentioned, performed when > > setting the "suppres #VE" bit in PTEs? Unsuppressing #VEs for a page > > will only do anything if the guest has already enabled #VE, so the > > previous issue doesn't apply in this case. > > suppress #VE has a negative meaning. Once #VE is enabled, all frames > need SVE for Xen to continue getting EPT_VIOLATION vmexits per usual. > It is only whitelisted frames which should have SVE cleared on them. > > Having said that, by the time you have cooperation between several > domains, I don't see a reason for excluding a remote domain updating SVE. Sorry for following up so late on this; what would be an acceptable approach to allow a remote domain to update the "Suppress VE" bit for a given domain? The current page permission code flow via do_altp2m_op() / HVMOP_altp2m_set_mem_access only allows setting actual page permissions, and all the other APIs it calls along the way (p2m_set_mem_access(), set_mem_access() and p2m_set_altp2m_mem_access() in mem_access.c) don't have a "suppress VE" parameter (up until the ept_set_entry() call, which gets sve as "current->domain != d" from p2m_set_altp2m_mem_access()). At that level a separate parameter for sVE does not make sense, since the code is shared with the ARM arch. Would it be acceptable to add an HVMOP_altp2m_set_suppres_ve op? Thank you, -- Vlad-Ioan TOPAN Linux Kernel Development Lead Bitdefender ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Enabling #VE for a domain from dom0
> #VE, by design, raises an exception in non-root context, without > breaking out to the hypervisor. > > The vcpu in question needs to set up a suitable #VE handler, so it is > not safe for an external entity to chose when a vcpu should start > receiving #VE's. The problem is that from a security solution standpoint, it isn't feasible in a Windows guest to use libxc to enable #VE. As it is implemented, libxc is required to allow sharing a structure between the guest and the host; the structure only contains the gfn of the #VE page and the domain id/vcpu id, which are useless since it can only be enabled on the current VCPU. Would a patch providing a simpler VMCALL (without sharing structures, only passing the gfn) to enable #VE be acceptable? Is there any reason for the other check I've mentioned, performed when setting the "suppres #VE" bit in PTEs? Unsuppressing #VEs for a page will only do anything if the guest has already enabled #VE, so the previous issue doesn't apply in this case. Thank you for the prompt answer! -- Vlad-Ioan TOPAN ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] Enabling #VE for a domain from dom0
Hello, We are trying to use the #VE support in Xen to monitor memory accesses to certain pages from a kernel module in Windows. As it is written now, the #VE-enabling code appears to enforce being called by a domain for itself (by each VCPU for itself, actually), which severely limits its usefulness. Is there an architectural reasoning for this or some other factor I'm missing? The process of enabling #VE as I understand it starts from xc_altp2m_set_vcpu_enable_notify(); further along, when the HVMOP_altp2m_vcpu_enable_notify message is handled, several checks are made to ensure it's called for the current VCPU. Another check is in p2m.c in the function p2m_set_altp2m_mem_access(), which clears the "suppress #VE" bit only for pages on the same domain: return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, (current->domain != d)); The "(current->domain != d)" bit there is the sve parameter. Is there any reason not to allow cross-domain enabling of #VE? Thank you, -- Vlad-Ioan TOPAN Linux Kernel Development Lead Bitdefender ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
