[PATCH V3 0/2] Introspection optimization helpers

[PATCH V3 0/2] Introspection optimization helpers

[Razvan Cojocaru]

Hello,

This series adds two minor patches. The first one allows finer-grained
control over the emulation behaviour of REP instructions. Previously,
once vm_event-based emulation was enabled, no optimizations were allowed.
However, this has a performance impact on the monitored guest, so I've
added a new libxc function to enable / disable this at will at any point.

The second patch addresses an older issue: in my initial series a few
years back, there was a (longish) patch that computed the length of the
current instruction, and advanced the instruction pointer past it if
it was required by the vm_event reply. However, integrating that code
has not been trivial, and so the second patch in this series simply
allows a vm_event reply to say that the instruction pointer should be
set to whatever value it sends back to the hypervisor. This way, the
computation of the instruction length is left to the userspace
application.

This version addresses comments received for the previous version,
the specific changes are described in the change log for each patch.

[PATCH V3 1/2] xen, libxc: Fine grained control of REP emulation
[PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS


Thanks,
Razvan

[PATCH V3 1/2] xen, libxc: Fine grained control of REP emulation optimizations

[Razvan Cojocaru]

Previously, if vm_event emulation support was enabled, then REP
optimizations were disabled when emulating REP-compatible
instructions. This patch allows fine-tuning of this behaviour by
providing a dedicated libxc helper function.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
Changes since V2:
 - Now only checking ->arch.mem_access_emulate_each_rep in
   hvmemul_virtual_to_linear() (previously was also checking
   ->arch.mem_access_emulate_enabled.
 - Disabling ->arch.mem_access_emulate_enabled when monitoring
   is disabled.
 - Changed the if()s in vm_event_resume() to a switch().
 - Added Ian Campbell's ack.
---
 tools/libxc/include/xenctrl.h |   12 ++++++++++++
 tools/libxc/xc_monitor.c      |   13 +++++++++++++
 xen/arch/x86/hvm/emulate.c    |    2 +-
 xen/arch/x86/monitor.c        |    7 ++++++-
 xen/arch/x86/vm_event.c       |    2 ++
 xen/include/asm-x86/domain.h  |    1 +
 xen/include/public/domctl.h   |    1 +
 7 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3482544..3bfa00b 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2428,6 +2428,18 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
 int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
                              bool enable, bool sync);
 
+/**
+ * This function enables / disables emulation for each REP for a
+ * REP-compatible instruction.
+ *
+ * @parm xch a handle to an open hypervisor interface.
+ * @parm domain_id the domain id one wants to get the node affinity of.
+ * @parm enable if 0 optimize when possible, else emulate each REP.
+ * @return 0 on success, -1 on failure.
+ */
+int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
+                                bool enable);
+
 /***
  * Memory sharing operations.
  *
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 065669c..b1705dd 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -143,3 +143,16 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
 
     return do_domctl(xch, &domctl);
 }
+
+int xc_monitor_emulate_each_rep(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 = XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP;
+    domctl.u.monitor_op.event = enable;
+
+    return do_domctl(xch, &domctl);
+}
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 5934c72..39774b7 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -514,7 +514,7 @@ static int hvmemul_virtual_to_linear(
      * being triggered for repeated writes to a whole page.
      */
     *reps = min_t(unsigned long, *reps,
-                  unlikely(current->domain->arch.mem_access_emulate_enabled)
+                  unlikely(current->domain->arch.mem_access_emulate_each_rep)
                            ? 1 : 4096);
 
     reg = hvmemul_get_seg_reg(seg, hvmemul_ctxt);
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 3d52135..7611f7b 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -73,10 +73,15 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
     if ( rc )
         return rc;
 
-    if ( mop->op == XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES )
+    switch ( mop->op )
     {
+    case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
         mop->event = capabilities;
         return 0;
+
+    case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
+        d->arch.mem_access_emulate_each_rep = !!mop->event;
+        return 0;
     }
 
     /*
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index e4e0aa4..c38d37b 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -54,6 +54,8 @@ void vm_event_cleanup_domain(struct domain *d)
         xfree(v->arch.vm_event);
         v->arch.vm_event = NULL;
     }
+
+    d->arch.mem_access_emulate_each_rep = 0;
 }
 
 void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index f0aeade..f1d7ed6 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -386,6 +386,7 @@ struct arch_domain
 
     /* Mem_access emulation control */
     bool_t mem_access_emulate_enabled;
+    bool_t mem_access_emulate_each_rep;
 } __cacheline_aligned;
 
 #define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 794d4d5..ae241f2 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1007,6 +1007,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_OP_ENABLE            0
 #define XEN_DOMCTL_MONITOR_OP_DISABLE           1
 #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
-- 
1.7.9.5

[PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Razvan Cojocaru]

A previous version of this patch dealing with support for skipping
the current instruction when a vm_event response requested it
computed the instruction length in the hypervisor, adding non-trivial
code dependencies. This patch allows a userspace vm_event client to
simply request that the guest's EIP is set to an arbitary value,
computed by the introspection application. The registers that can
now be set are EAX-EDX, ESP, EBP, ESI, EDI, R8-R15, EFLAGS, and EIP.
CR0, CR3 and CR4 are not set, as at the time of vm_event_resume()
we can't call hvm_set_cr{0,3,4}() and simply setting
v->arch.hvm_vcpu.guest_cr[{0,3,4}] is unlikely to have the desired
effect. The rest of the vm_event registers are not set because
they're not being filled by hvm_event_fill_regs(), but only by
p2m_vm_event_fill_regs().
The VCPU needs to be paused for this flag to take effect.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
Changes since V2:
 - Now setting more registers than just EIP.
 - Updated the header comment to state cleary which registers are
   now affected.
---
 xen/arch/x86/vm_event.c        |   24 ++++++++++++++++++++++++
 xen/common/vm_event.c          |    3 +++
 xen/include/asm-arm/vm_event.h |    6 ++++++
 xen/include/asm-x86/vm_event.h |    2 ++
 xen/include/public/vm_event.h  |    6 ++++++
 5 files changed, 41 insertions(+)

diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index c38d37b..9677ecc 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -97,6 +97,30 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
     }
 }
 
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
+{
+    v->arch.user_regs.eax = rsp->data.regs.x86.rax;
+    v->arch.user_regs.ebx = rsp->data.regs.x86.rbx;
+    v->arch.user_regs.ecx = rsp->data.regs.x86.rcx;
+    v->arch.user_regs.edx = rsp->data.regs.x86.rdx;
+    v->arch.user_regs.esp = rsp->data.regs.x86.rsp;
+    v->arch.user_regs.ebp = rsp->data.regs.x86.rbp;
+    v->arch.user_regs.esi = rsp->data.regs.x86.rsi;
+    v->arch.user_regs.edi = rsp->data.regs.x86.rdi;
+
+    v->arch.user_regs.r8 = rsp->data.regs.x86.r8;
+    v->arch.user_regs.r9 = rsp->data.regs.x86.r9;
+    v->arch.user_regs.r10 = rsp->data.regs.x86.r10;
+    v->arch.user_regs.r11 = rsp->data.regs.x86.r11;
+    v->arch.user_regs.r12 = rsp->data.regs.x86.r12;
+    v->arch.user_regs.r13 = rsp->data.regs.x86.r13;
+    v->arch.user_regs.r14 = rsp->data.regs.x86.r14;
+    v->arch.user_regs.r15 = rsp->data.regs.x86.r15;
+
+    v->arch.user_regs.eflags = rsp->data.regs.x86.rflags;
+    v->arch.user_regs.eip = rsp->data.regs.x86.rip;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index ef84b0f..e1f9580 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -417,6 +417,9 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
 
         if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
         {
+            if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
+                vm_event_set_registers(v, &rsp);
+
             if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
                 vm_event_toggle_singlestep(d, v);
 
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 976fdf1..4d0fbf7 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -47,4 +47,10 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
     /* Not supported on ARM. */
 }
 
+static inline
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
+{
+    /* Not supported on ARM. */
+}
+
 #endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 2ff2cab..5aff834 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -42,4 +42,6 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
 
 void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp);
 
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
+
 #endif /* __ASM_X86_VM_EVENT_H__ */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index ff2f217..3e4efad 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -89,6 +89,12 @@
  * by the altp2m_idx response field if possible.
  */
 #define VM_EVENT_FLAG_ALTERNATE_P2M      (1 << 7)
+/*
+ * Set the vCPU registers to the values in the  vm_event response.
+ * Applies to EAX-EDX, ESP, EBP, ESI, EDI, R8-R15, EFLAGS, and EIP.
+ * Requires the vCPU to be paused already (synchronous events only).
+ */
+#define VM_EVENT_FLAG_SET_REGISTERS      (1 << 8)
 
 /*
  * Reasons for the vm event request
-- 
1.7.9.5

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Andrew Cooper]

On 28/09/15 11:16, Razvan Cojocaru wrote:
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index ff2f217..3e4efad 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -89,6 +89,12 @@
>   * by the altp2m_idx response field if possible.
>   */
>  #define VM_EVENT_FLAG_ALTERNATE_P2M      (1 << 7)
> +/*
> + * Set the vCPU registers to the values in the  vm_event response.
> + * Applies to EAX-EDX, ESP, EBP, ESI, EDI, R8-R15, EFLAGS, and EIP.
> + * Requires the vCPU to be paused already (synchronous events only).
> + */

The set registers are architecture specific, or will be if/when ARM
gains support.

It is fine to list the architecture specific bits here (as there are no
more appropriate places for it to live) but do explicitly call out that
the 2nd sentence in x86 specific.

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH V3 1/2] xen, libxc: Fine grained control of REP emulation optimizations

[Andrew Cooper]

On 28/09/15 11:16, Razvan Cojocaru wrote:
> Previously, if vm_event emulation support was enabled, then REP
> optimizations were disabled when emulating REP-compatible
> instructions. This patch allows fine-tuning of this behaviour by
> providing a dedicated libxc helper function.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Razvan Cojocaru]

On 09/28/2015 01:26 PM, Andrew Cooper wrote:
> On 28/09/15 11:16, Razvan Cojocaru wrote:
>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
>> index ff2f217..3e4efad 100644
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -89,6 +89,12 @@
>>   * by the altp2m_idx response field if possible.
>>   */
>>  #define VM_EVENT_FLAG_ALTERNATE_P2M      (1 << 7)
>> +/*
>> + * Set the vCPU registers to the values in the  vm_event response.
>> + * Applies to EAX-EDX, ESP, EBP, ESI, EDI, R8-R15, EFLAGS, and EIP.
>> + * Requires the vCPU to be paused already (synchronous events only).
>> + */
> 
> The set registers are architecture specific, or will be if/when ARM
> gains support.
> 
> It is fine to list the architecture specific bits here (as there are no
> more appropriate places for it to live) but do explicitly call out that
> the 2nd sentence in x86 specific.
> 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Will do in the next version.


Thanks for the review,
Razvan

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Jan Beulich]

>>> On 28.09.15 at 12:16, <rcojocaru@bitdefender.com> wrote:
> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
> +{
> +    v->arch.user_regs.eax = rsp->data.regs.x86.rax;
> +    v->arch.user_regs.ebx = rsp->data.regs.x86.rbx;
> +    v->arch.user_regs.ecx = rsp->data.regs.x86.rcx;
> +    v->arch.user_regs.edx = rsp->data.regs.x86.rdx;
> +    v->arch.user_regs.esp = rsp->data.regs.x86.rsp;
> +    v->arch.user_regs.ebp = rsp->data.regs.x86.rbp;
> +    v->arch.user_regs.esi = rsp->data.regs.x86.rsi;
> +    v->arch.user_regs.edi = rsp->data.regs.x86.rdi;
> +
> +    v->arch.user_regs.r8 = rsp->data.regs.x86.r8;
> +    v->arch.user_regs.r9 = rsp->data.regs.x86.r9;
> +    v->arch.user_regs.r10 = rsp->data.regs.x86.r10;
> +    v->arch.user_regs.r11 = rsp->data.regs.x86.r11;
> +    v->arch.user_regs.r12 = rsp->data.regs.x86.r12;
> +    v->arch.user_regs.r13 = rsp->data.regs.x86.r13;
> +    v->arch.user_regs.r14 = rsp->data.regs.x86.r14;
> +    v->arch.user_regs.r15 = rsp->data.regs.x86.r15;
> +
> +    v->arch.user_regs.eflags = rsp->data.regs.x86.rflags;

Shouldn't you sanitize the value? I can't immediately see anything
putting Xen at risk (but it also doesn't seem impossible that I'm
overlooking something), but surely putting insane values here
can lead to hard to debug guest crashes.

> +    v->arch.user_regs.eip = rsp->data.regs.x86.rip;

Similarly here I wonder whether this shouldn't be at least
range checked.

> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -417,6 +417,9 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
>  
>          if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
>          {
> +            if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
> +                vm_event_set_registers(v, &rsp);
> +
>              if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
>                  vm_event_toggle_singlestep(d, v);
 
What meaning has setting both flags and EFLAGS.TF in the new
register values?

Jan

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Razvan Cojocaru]

On 09/28/2015 06:25 PM, Jan Beulich wrote:
>>>> On 28.09.15 at 12:16, <rcojocaru@bitdefender.com> wrote:
>> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
>> +{
>> +    v->arch.user_regs.eax = rsp->data.regs.x86.rax;
>> +    v->arch.user_regs.ebx = rsp->data.regs.x86.rbx;
>> +    v->arch.user_regs.ecx = rsp->data.regs.x86.rcx;
>> +    v->arch.user_regs.edx = rsp->data.regs.x86.rdx;
>> +    v->arch.user_regs.esp = rsp->data.regs.x86.rsp;
>> +    v->arch.user_regs.ebp = rsp->data.regs.x86.rbp;
>> +    v->arch.user_regs.esi = rsp->data.regs.x86.rsi;
>> +    v->arch.user_regs.edi = rsp->data.regs.x86.rdi;
>> +
>> +    v->arch.user_regs.r8 = rsp->data.regs.x86.r8;
>> +    v->arch.user_regs.r9 = rsp->data.regs.x86.r9;
>> +    v->arch.user_regs.r10 = rsp->data.regs.x86.r10;
>> +    v->arch.user_regs.r11 = rsp->data.regs.x86.r11;
>> +    v->arch.user_regs.r12 = rsp->data.regs.x86.r12;
>> +    v->arch.user_regs.r13 = rsp->data.regs.x86.r13;
>> +    v->arch.user_regs.r14 = rsp->data.regs.x86.r14;
>> +    v->arch.user_regs.r15 = rsp->data.regs.x86.r15;
>> +
>> +    v->arch.user_regs.eflags = rsp->data.regs.x86.rflags;
> 
> Shouldn't you sanitize the value? I can't immediately see anything
> putting Xen at risk (but it also doesn't seem impossible that I'm
> overlooking something), but surely putting insane values here
> can lead to hard to debug guest crashes.
> 
>> +    v->arch.user_regs.eip = rsp->data.regs.x86.rip;
> 
> Similarly here I wonder whether this shouldn't be at least
> range checked.

I'm assuming that the userspace vm_event client application will use the
interface wisely. A typical scenario would be:

- vm_event request received;
- reply.registers = request.registers;
- if this event requires setting registers, set only relevant ones;
- send the reply to the hypervisor (with the set registers flag).

So with this usage it shouldn't be possible to accidentally send garbage
back.

>> --- a/xen/common/vm_event.c
>> +++ b/xen/common/vm_event.c
>> @@ -417,6 +417,9 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
>>  
>>          if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
>>          {
>> +            if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
>> +                vm_event_set_registers(v, &rsp);
>> +
>>              if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
>>                  vm_event_toggle_singlestep(d, v);
>  
> What meaning has setting both flags and EFLAGS.TF in the new
> register values?

That's a good question. I'm not sure how that would affect an attached
debugger type scenario.

I'm also unsure of what a good solution to this issue would be. I could
make the flags exclusive, but that would prevent, for example, setting
EAX and singlestepping, which should not be a problem. I could also
remove the eflags assignment from vm_event_set_registers() (maybe
replace it with a comment).

Tamas, do you need eflags set? What do you think? Again, I'm happy with
just setting EIP, what scenarios are you interested in?


Thanks,
Razvan

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Andrew Cooper]

On 28/09/15 16:25, Jan Beulich wrote:
>>>> On 28.09.15 at 12:16, <rcojocaru@bitdefender.com> wrote:
>> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
>> +{
>> +    v->arch.user_regs.eax = rsp->data.regs.x86.rax;
>> +    v->arch.user_regs.ebx = rsp->data.regs.x86.rbx;
>> +    v->arch.user_regs.ecx = rsp->data.regs.x86.rcx;
>> +    v->arch.user_regs.edx = rsp->data.regs.x86.rdx;
>> +    v->arch.user_regs.esp = rsp->data.regs.x86.rsp;
>> +    v->arch.user_regs.ebp = rsp->data.regs.x86.rbp;
>> +    v->arch.user_regs.esi = rsp->data.regs.x86.rsi;
>> +    v->arch.user_regs.edi = rsp->data.regs.x86.rdi;
>> +
>> +    v->arch.user_regs.r8 = rsp->data.regs.x86.r8;
>> +    v->arch.user_regs.r9 = rsp->data.regs.x86.r9;
>> +    v->arch.user_regs.r10 = rsp->data.regs.x86.r10;
>> +    v->arch.user_regs.r11 = rsp->data.regs.x86.r11;
>> +    v->arch.user_regs.r12 = rsp->data.regs.x86.r12;
>> +    v->arch.user_regs.r13 = rsp->data.regs.x86.r13;
>> +    v->arch.user_regs.r14 = rsp->data.regs.x86.r14;
>> +    v->arch.user_regs.r15 = rsp->data.regs.x86.r15;
>> +
>> +    v->arch.user_regs.eflags = rsp->data.regs.x86.rflags;
> Shouldn't you sanitize the value? I can't immediately see anything
> putting Xen at risk (but it also doesn't seem impossible that I'm
> overlooking something), but surely putting insane values here
> can lead to hard to debug guest crashes.

I had the same thought (e.g. XSA-111), but all modifications like this
are already possible with a cunningly-crafted sethvmcontext so we are at
no more risk than before.

Furthermore, I can't think of any plausible validation which could be
done.  It is entirely possible that this interface could be used to
bounce execution into a hidden introspection agent.

~Andrew

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Jan Beulich]

>>> On 28.09.15 at 17:57, <andrew.cooper3@citrix.com> wrote:
> On 28/09/15 16:25, Jan Beulich wrote:
>>>>> On 28.09.15 at 12:16, <rcojocaru@bitdefender.com> wrote:
>>> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
>>> +{
>>> +    v->arch.user_regs.eax = rsp->data.regs.x86.rax;
>>> +    v->arch.user_regs.ebx = rsp->data.regs.x86.rbx;
>>> +    v->arch.user_regs.ecx = rsp->data.regs.x86.rcx;
>>> +    v->arch.user_regs.edx = rsp->data.regs.x86.rdx;
>>> +    v->arch.user_regs.esp = rsp->data.regs.x86.rsp;
>>> +    v->arch.user_regs.ebp = rsp->data.regs.x86.rbp;
>>> +    v->arch.user_regs.esi = rsp->data.regs.x86.rsi;
>>> +    v->arch.user_regs.edi = rsp->data.regs.x86.rdi;
>>> +
>>> +    v->arch.user_regs.r8 = rsp->data.regs.x86.r8;
>>> +    v->arch.user_regs.r9 = rsp->data.regs.x86.r9;
>>> +    v->arch.user_regs.r10 = rsp->data.regs.x86.r10;
>>> +    v->arch.user_regs.r11 = rsp->data.regs.x86.r11;
>>> +    v->arch.user_regs.r12 = rsp->data.regs.x86.r12;
>>> +    v->arch.user_regs.r13 = rsp->data.regs.x86.r13;
>>> +    v->arch.user_regs.r14 = rsp->data.regs.x86.r14;
>>> +    v->arch.user_regs.r15 = rsp->data.regs.x86.r15;
>>> +
>>> +    v->arch.user_regs.eflags = rsp->data.regs.x86.rflags;
>> Shouldn't you sanitize the value? I can't immediately see anything
>> putting Xen at risk (but it also doesn't seem impossible that I'm
>> overlooking something), but surely putting insane values here
>> can lead to hard to debug guest crashes.
> 
> I had the same thought (e.g. XSA-111), but all modifications like this
> are already possible with a cunningly-crafted sethvmcontext so we are at
> no more risk than before.

By or for HVM guests. But how about PV?

> Furthermore, I can't think of any plausible validation which could be
> done.  It is entirely possible that this interface could be used to
> bounce execution into a hidden introspection agent.

Flipping VM, AC, NT or altering IOPL would all seem bogus to me.

Jan

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 2764 bytes --]

On Mon, Sep 28, 2015 at 10:06 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 28.09.15 at 17:57, <andrew.cooper3@citrix.com> wrote:
> > On 28/09/15 16:25, Jan Beulich wrote:
> >>>>> On 28.09.15 at 12:16, <rcojocaru@bitdefender.com> wrote:
> >>> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
> >>> +{
> >>> +    v->arch.user_regs.eax = rsp->data.regs.x86.rax;
> >>> +    v->arch.user_regs.ebx = rsp->data.regs.x86.rbx;
> >>> +    v->arch.user_regs.ecx = rsp->data.regs.x86.rcx;
> >>> +    v->arch.user_regs.edx = rsp->data.regs.x86.rdx;
> >>> +    v->arch.user_regs.esp = rsp->data.regs.x86.rsp;
> >>> +    v->arch.user_regs.ebp = rsp->data.regs.x86.rbp;
> >>> +    v->arch.user_regs.esi = rsp->data.regs.x86.rsi;
> >>> +    v->arch.user_regs.edi = rsp->data.regs.x86.rdi;
> >>> +
> >>> +    v->arch.user_regs.r8 = rsp->data.regs.x86.r8;
> >>> +    v->arch.user_regs.r9 = rsp->data.regs.x86.r9;
> >>> +    v->arch.user_regs.r10 = rsp->data.regs.x86.r10;
> >>> +    v->arch.user_regs.r11 = rsp->data.regs.x86.r11;
> >>> +    v->arch.user_regs.r12 = rsp->data.regs.x86.r12;
> >>> +    v->arch.user_regs.r13 = rsp->data.regs.x86.r13;
> >>> +    v->arch.user_regs.r14 = rsp->data.regs.x86.r14;
> >>> +    v->arch.user_regs.r15 = rsp->data.regs.x86.r15;
> >>> +
> >>> +    v->arch.user_regs.eflags = rsp->data.regs.x86.rflags;
> >> Shouldn't you sanitize the value? I can't immediately see anything
> >> putting Xen at risk (but it also doesn't seem impossible that I'm
> >> overlooking something), but surely putting insane values here
> >> can lead to hard to debug guest crashes.
> >
> > I had the same thought (e.g. XSA-111), but all modifications like this
> > are already possible with a cunningly-crafted sethvmcontext so we are at
> > no more risk than before.
>
> By or for HVM guests. But how about PV?
>
> > Furthermore, I can't think of any plausible validation which could be
> > done.  It is entirely possible that this interface could be used to
> > bounce execution into a hidden introspection agent.
>
> Flipping VM, AC, NT or altering IOPL would all seem bogus to me.
>
> Jan
>

The entire interface is only available from a privileged control domain
already able to perform all of the steps that this option does. This option
just streamlines that operation so we don't have to do two hypercalls. So
while certainly a buggy vm_event application can crash the guest in
spectacular ways I don't think Xen could (or should) really prevent that
from happening. Currently vm_events are not implemented for PV guests so
that IMHO is not a problem we need to consider right now. Furthermore, even
if we add support for PV guests somewhere down the line the interface
should be on par with the current HVM implementation.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 3892 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1400 bytes --]

>> +            if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
> >> +                vm_event_set_registers(v, &rsp);
> >> +
> >>              if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
> >>                  vm_event_toggle_singlestep(d, v);
> >
> > What meaning has setting both flags and EFLAGS.TF in the new
> > register values?
>
> That's a good question. I'm not sure how that would affect an attached
> debugger type scenario.
>
> I'm also unsure of what a good solution to this issue would be. I could
> make the flags exclusive, but that would prevent, for example, setting
> EAX and singlestepping, which should not be a problem. I could also
> remove the eflags assignment from vm_event_set_registers() (maybe
> replace it with a comment).
>
> Tamas, do you need eflags set? What do you think? Again, I'm happy with
> just setting EIP, what scenarios are you interested in?
>
>
Being able to set registers this way and enabling MTF in one-shot I think
offers good flexibility and performance for vm_event applications. I don't
see any reason why these options should be exclusive. Furthermore, I don't
see why we would treat EFLAGS.TF any different then the others. If the
vm_event application decides to flip both the in-guest TF and the external
MTF, why prevent that? While I don't see an immediate use-case for this,  I
also don't see why it would be problematic either.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1971 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Jan Beulich]

>>> On 28.09.15 at 20:46, <tamas@tklengyel.com> wrote:
>> > +            if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
>> >> +                vm_event_set_registers(v, &rsp);
>> >> +
>> >>              if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
>> >>                  vm_event_toggle_singlestep(d, v);
>> >
>> > What meaning has setting both flags and EFLAGS.TF in the new
>> > register values?
>>
>> That's a good question. I'm not sure how that would affect an attached
>> debugger type scenario.
>>
>> I'm also unsure of what a good solution to this issue would be. I could
>> make the flags exclusive, but that would prevent, for example, setting
>> EAX and singlestepping, which should not be a problem. I could also
>> remove the eflags assignment from vm_event_set_registers() (maybe
>> replace it with a comment).
>>
>> Tamas, do you need eflags set? What do you think? Again, I'm happy with
>> just setting EIP, what scenarios are you interested in?
>>
>>
> Being able to set registers this way and enabling MTF in one-shot I think
> offers good flexibility and performance for vm_event applications. I don't
> see any reason why these options should be exclusive. Furthermore, I don't
> see why we would treat EFLAGS.TF any different then the others. If the
> vm_event application decides to flip both the in-guest TF and the external
> MTF, why prevent that? While I don't see an immediate use-case for this,  I
> also don't see why it would be problematic either.

Okay. Sounds like an ack then - if so, could you formalize that?

Jan

Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1090 bytes --]

On Mon, Sep 28, 2015 at 4:16 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> A previous version of this patch dealing with support for skipping
> the current instruction when a vm_event response requested it
> computed the instruction length in the hypervisor, adding non-trivial
> code dependencies. This patch allows a userspace vm_event client to
> simply request that the guest's EIP is set to an arbitary value,
> computed by the introspection application. The registers that can
> now be set are EAX-EDX, ESP, EBP, ESI, EDI, R8-R15, EFLAGS, and EIP.
> CR0, CR3 and CR4 are not set, as at the time of vm_event_resume()
> we can't call hvm_set_cr{0,3,4}() and simply setting
> v->arch.hvm_vcpu.guest_cr[{0,3,4}] is unlikely to have the desired
> effect. The rest of the vm_event registers are not set because
> they're not being filled by hvm_event_fill_regs(), but only by
> p2m_vm_event_fill_regs().
> The VCPU needs to be paused for this flag to take effect.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

[-- Attachment #1.2: Type: text/html, Size: 1642 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel