[PATCH V3 0/3] Vm_event memory introspection helpers

[PATCH V3 0/3] Vm_event memory introspection helpers

[Razvan Cojocaru]

Version 3 of the series addresses V2 reviews, and consists of:

[PATCH V3 1/3] xen/mem_access: Support for memory-content hiding
[PATCH V3 2/3] xen/vm_event: Support for guest-requested events
[PATCH V3 3/3] xen/vm_event: Deny register writes if refused by
vm_event reply

The description of each patch includes a detailed description of
the changes since V2.


Thanks in advance for your review,
Razvan

[PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru]

This patch adds support for memory-content hiding, by modifying the
value returned by emulated instructions that read certain memory
addresses that contain sensitive data. The patch only applies to
cases where MEM_ACCESS_EMULATE or MEM_ACCESS_EMULATE_NOWRITE have
been set to a vm_event response.

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

---
Changes since V2:
 - Now memset()-ing all read buffers to 0 before writing the response
   data (don't leak HV stack contents).
 - Safe_bytes is now unsigned int everywhere instead of unsigned long.
 - Removed the custom hvmemul_rep_stos() handler.
 - EMUL_KIND_NORMAL became the default case in
   hvm_mem_access_emulate_one().
 - Replaced a 'break' with a more appropriate 'continue' in
   vm_event_enable().
 - The rep outs case now acts consistent with the read case
   (no longer ignoring *reps).
 - Removed the individual set_context static handlers in favour of
   a flag in struct hvmemul_ctxt.
 - Corrected patch title (it's mem_access and vm_event, there's no
   such thing as vm_access).
---
 tools/tests/xen-access/xen-access.c |    2 +-
 xen/arch/x86/domain.c               |    1 +
 xen/arch/x86/hvm/emulate.c          |  132 ++++++++++++++++++++++++++++++++++-
 xen/arch/x86/hvm/event.c            |   50 ++++++-------
 xen/arch/x86/mm/p2m.c               |   90 +++++++++++++-----------
 xen/common/vm_event.c               |   23 ++++++
 xen/include/asm-x86/domain.h        |    2 +
 xen/include/asm-x86/hvm/emulate.h   |   10 ++-
 xen/include/public/vm_event.h       |   21 +++++-
 9 files changed, 259 insertions(+), 72 deletions(-)

diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 12ab921..e6ca9ba 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -530,7 +530,7 @@ int main(int argc, char *argv[])
                 break;
             case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
                 printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n",
-                       req.regs.x86.rip,
+                       req.data.regs.x86.rip,
                        req.u.software_breakpoint.gfn,
                        req.vcpu_id);
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a8fe046..cbbc354 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -269,6 +269,7 @@ struct vcpu *alloc_vcpu_struct(void)
 
 void free_vcpu_struct(struct vcpu *v)
 {
+    xfree(v->arch.vm_event.emul_read_data);
     free_xenheap_page(v);
 }
 
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index fe5661d..3678e29 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -653,6 +653,28 @@ static int hvmemul_read(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    struct hvm_emulate_ctxt *hvmemul_ctxt =
+        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+
+    if ( unlikely(hvmemul_ctxt->set_context) )
+    {
+        struct vcpu *curr = current;
+        unsigned int len;
+
+        if ( !curr->arch.vm_event.emul_read_data )
+            return X86EMUL_UNHANDLEABLE;
+
+        len = min_t(unsigned int,
+                    bytes, curr->arch.vm_event.emul_read_data->size);
+
+        if ( len ) {
+            memset(p_data, 0, bytes);
+            memcpy(p_data, curr->arch.vm_event.emul_read_data->data, len);
+        }
+
+        return X86EMUL_OKAY;
+    }
+
     return __hvmemul_read(
         seg, offset, p_data, bytes, hvm_access_read,
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt));
@@ -893,6 +915,24 @@ static int hvmemul_cmpxchg(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    struct hvm_emulate_ctxt *hvmemul_ctxt =
+        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+
+    if ( unlikely(hvmemul_ctxt->set_context) )
+    {
+        struct vcpu *curr = current;
+
+        if ( curr->arch.vm_event.emul_read_data )
+        {
+            unsigned int safe_bytes = min_t(unsigned int, bytes,
+                curr->arch.vm_event.emul_read_data->size);
+
+            memset(p_new, 0, bytes);
+            memcpy(p_new, curr->arch.vm_event.emul_read_data->data,
+                   safe_bytes);
+        }
+    }
+
     /* Fix this in case the guest is really relying on r-m-w atomicity. */
     return hvmemul_write(seg, offset, p_new, bytes, ctxt);
 }
@@ -935,6 +975,41 @@ static int hvmemul_rep_ins(
                                !!(ctxt->regs->eflags & X86_EFLAGS_DF), gpa);
 }
 
+static int hvmemul_rep_outs_set_context(
+    enum x86_segment src_seg,
+    unsigned long src_offset,
+    uint16_t dst_port,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    unsigned int bytes = *reps * bytes_per_rep;
+    struct vcpu *curr = current;
+    unsigned int safe_bytes;
+    char *buf = NULL;
+    int rc;
+
+    if ( !curr->arch.vm_event.emul_read_data )
+        return X86EMUL_UNHANDLEABLE;
+
+    buf = xmalloc_bytes(bytes);
+
+    if ( buf == NULL )
+        return X86EMUL_UNHANDLEABLE;
+
+    memset(buf, 0, bytes);
+
+    safe_bytes = min_t(unsigned int, bytes,
+                       curr->arch.vm_event.emul_read_data->size);
+
+    memcpy(buf, curr->arch.vm_event.emul_read_data->data, safe_bytes);
+    rc = hvmemul_do_pio_buffer(dst_port, bytes, IOREQ_WRITE, buf);
+
+    xfree(buf);
+
+    return rc;
+}
+
 static int hvmemul_rep_outs(
     enum x86_segment src_seg,
     unsigned long src_offset,
@@ -951,6 +1026,10 @@ static int hvmemul_rep_outs(
     p2m_type_t p2mt;
     int rc;
 
+    if ( unlikely(hvmemul_ctxt->set_context) )
+        return hvmemul_rep_outs_set_context(src_seg, src_offset, dst_port,
+                                            bytes_per_rep, reps, ctxt);
+
     rc = hvmemul_virtual_to_linear(
         src_seg, src_offset, bytes_per_rep, reps, hvm_access_read,
         hvmemul_ctxt, &addr);
@@ -1063,7 +1142,20 @@ static int hvmemul_rep_movs(
      */
     rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
     if ( rc == HVMCOPY_okay )
+    {
+        struct vcpu *curr = current;
+
+        if ( unlikely(hvmemul_ctxt->set_context) &&
+             curr->arch.vm_event.emul_read_data )
+        {
+            unsigned int safe_bytes = min_t(unsigned int, bytes,
+                curr->arch.vm_event.emul_read_data->size);
+
+            memcpy(buf, curr->arch.vm_event.emul_read_data->data, safe_bytes);
+        }
+
         rc = hvm_copy_to_guest_phys(dgpa, buf, bytes);
+    }
 
     xfree(buf);
 
@@ -1222,7 +1314,27 @@ static int hvmemul_read_io(
     unsigned long *val,
     struct x86_emulate_ctxt *ctxt)
 {
+    struct hvm_emulate_ctxt *hvmemul_ctxt =
+        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+
     *val = 0;
+
+    if ( unlikely(hvmemul_ctxt->set_context) )
+    {
+        struct vcpu *curr = current;
+        unsigned int safe_bytes;
+
+        if ( !curr->arch.vm_event.emul_read_data )
+            return X86EMUL_UNHANDLEABLE;
+
+        safe_bytes = min_t(unsigned int, bytes,
+            curr->arch.vm_event.emul_read_data->size);
+
+        memcpy(val, curr->arch.vm_event.emul_read_data->data, safe_bytes);
+
+        return X86EMUL_OKAY;
+    }
+
     return hvmemul_do_pio_buffer(port, bytes, IOREQ_READ, val);
 }
 
@@ -1599,6 +1711,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
 int hvm_emulate_one(
     struct hvm_emulate_ctxt *hvmemul_ctxt)
 {
+    hvmemul_ctxt->set_context = 0;
     return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops);
 }
 
@@ -1608,7 +1721,14 @@ int hvm_emulate_one_no_write(
     return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops_no_write);
 }
 
-void hvm_mem_access_emulate_one(bool_t nowrite, unsigned int trapnr,
+static int hvm_emulate_one_set_context(
+    struct hvm_emulate_ctxt *hvmemul_ctxt)
+{
+    hvmemul_ctxt->set_context = 1;
+    return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops_no_write);
+}
+
+void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
     unsigned int errcode)
 {
     struct hvm_emulate_ctxt ctx = {{ 0 }};
@@ -1616,10 +1736,16 @@ void hvm_mem_access_emulate_one(bool_t nowrite, unsigned int trapnr,
 
     hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
 
-    if ( nowrite )
+    switch ( kind ) {
+    case EMUL_KIND_NOWRITE:
         rc = hvm_emulate_one_no_write(&ctx);
-    else
+        break;
+    case EMUL_KIND_SET_CONTEXT:
+        rc = hvm_emulate_one_set_context(&ctx);
+        break;
+    default:
         rc = hvm_emulate_one(&ctx);
+    }
 
     switch ( rc )
     {
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 53b9ca4..5341937 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -30,31 +30,31 @@ static void hvm_event_fill_regs(vm_event_request_t *req)
     const struct cpu_user_regs *regs = guest_cpu_user_regs();
     const struct vcpu *curr = current;
 
-    req->regs.x86.rax = regs->eax;
-    req->regs.x86.rcx = regs->ecx;
-    req->regs.x86.rdx = regs->edx;
-    req->regs.x86.rbx = regs->ebx;
-    req->regs.x86.rsp = regs->esp;
-    req->regs.x86.rbp = regs->ebp;
-    req->regs.x86.rsi = regs->esi;
-    req->regs.x86.rdi = regs->edi;
-
-    req->regs.x86.r8  = regs->r8;
-    req->regs.x86.r9  = regs->r9;
-    req->regs.x86.r10 = regs->r10;
-    req->regs.x86.r11 = regs->r11;
-    req->regs.x86.r12 = regs->r12;
-    req->regs.x86.r13 = regs->r13;
-    req->regs.x86.r14 = regs->r14;
-    req->regs.x86.r15 = regs->r15;
-
-    req->regs.x86.rflags = regs->eflags;
-    req->regs.x86.rip    = regs->eip;
-
-    req->regs.x86.msr_efer = curr->arch.hvm_vcpu.guest_efer;
-    req->regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0];
-    req->regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3];
-    req->regs.x86.cr4 = curr->arch.hvm_vcpu.guest_cr[4];
+    req->data.regs.x86.rax = regs->eax;
+    req->data.regs.x86.rcx = regs->ecx;
+    req->data.regs.x86.rdx = regs->edx;
+    req->data.regs.x86.rbx = regs->ebx;
+    req->data.regs.x86.rsp = regs->esp;
+    req->data.regs.x86.rbp = regs->ebp;
+    req->data.regs.x86.rsi = regs->esi;
+    req->data.regs.x86.rdi = regs->edi;
+
+    req->data.regs.x86.r8  = regs->r8;
+    req->data.regs.x86.r9  = regs->r9;
+    req->data.regs.x86.r10 = regs->r10;
+    req->data.regs.x86.r11 = regs->r11;
+    req->data.regs.x86.r12 = regs->r12;
+    req->data.regs.x86.r13 = regs->r13;
+    req->data.regs.x86.r14 = regs->r14;
+    req->data.regs.x86.r15 = regs->r15;
+
+    req->data.regs.x86.rflags = regs->eflags;
+    req->data.regs.x86.rip    = regs->eip;
+
+    req->data.regs.x86.msr_efer = curr->arch.hvm_vcpu.guest_efer;
+    req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0];
+    req->data.regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3];
+    req->data.regs.x86.cr4 = curr->arch.hvm_vcpu.guest_cr[4];
 }
 
 static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6b39733..5d40d2c 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1369,49 +1369,49 @@ static void p2m_vm_event_fill_regs(vm_event_request_t *req)
     /* Architecture-specific vmcs/vmcb bits */
     hvm_funcs.save_cpu_ctxt(curr, &ctxt);
 
-    req->regs.x86.rax = regs->eax;
-    req->regs.x86.rcx = regs->ecx;
-    req->regs.x86.rdx = regs->edx;
-    req->regs.x86.rbx = regs->ebx;
-    req->regs.x86.rsp = regs->esp;
-    req->regs.x86.rbp = regs->ebp;
-    req->regs.x86.rsi = regs->esi;
-    req->regs.x86.rdi = regs->edi;
-
-    req->regs.x86.r8  = regs->r8;
-    req->regs.x86.r9  = regs->r9;
-    req->regs.x86.r10 = regs->r10;
-    req->regs.x86.r11 = regs->r11;
-    req->regs.x86.r12 = regs->r12;
-    req->regs.x86.r13 = regs->r13;
-    req->regs.x86.r14 = regs->r14;
-    req->regs.x86.r15 = regs->r15;
-
-    req->regs.x86.rflags = regs->eflags;
-    req->regs.x86.rip    = regs->eip;
-
-    req->regs.x86.dr7 = curr->arch.debugreg[7];
-    req->regs.x86.cr0 = ctxt.cr0;
-    req->regs.x86.cr2 = ctxt.cr2;
-    req->regs.x86.cr3 = ctxt.cr3;
-    req->regs.x86.cr4 = ctxt.cr4;
-
-    req->regs.x86.sysenter_cs = ctxt.sysenter_cs;
-    req->regs.x86.sysenter_esp = ctxt.sysenter_esp;
-    req->regs.x86.sysenter_eip = ctxt.sysenter_eip;
-
-    req->regs.x86.msr_efer = ctxt.msr_efer;
-    req->regs.x86.msr_star = ctxt.msr_star;
-    req->regs.x86.msr_lstar = ctxt.msr_lstar;
+    req->data.regs.x86.rax = regs->eax;
+    req->data.regs.x86.rcx = regs->ecx;
+    req->data.regs.x86.rdx = regs->edx;
+    req->data.regs.x86.rbx = regs->ebx;
+    req->data.regs.x86.rsp = regs->esp;
+    req->data.regs.x86.rbp = regs->ebp;
+    req->data.regs.x86.rsi = regs->esi;
+    req->data.regs.x86.rdi = regs->edi;
+
+    req->data.regs.x86.r8  = regs->r8;
+    req->data.regs.x86.r9  = regs->r9;
+    req->data.regs.x86.r10 = regs->r10;
+    req->data.regs.x86.r11 = regs->r11;
+    req->data.regs.x86.r12 = regs->r12;
+    req->data.regs.x86.r13 = regs->r13;
+    req->data.regs.x86.r14 = regs->r14;
+    req->data.regs.x86.r15 = regs->r15;
+
+    req->data.regs.x86.rflags = regs->eflags;
+    req->data.regs.x86.rip    = regs->eip;
+
+    req->data.regs.x86.dr7 = curr->arch.debugreg[7];
+    req->data.regs.x86.cr0 = ctxt.cr0;
+    req->data.regs.x86.cr2 = ctxt.cr2;
+    req->data.regs.x86.cr3 = ctxt.cr3;
+    req->data.regs.x86.cr4 = ctxt.cr4;
+
+    req->data.regs.x86.sysenter_cs = ctxt.sysenter_cs;
+    req->data.regs.x86.sysenter_esp = ctxt.sysenter_esp;
+    req->data.regs.x86.sysenter_eip = ctxt.sysenter_eip;
+
+    req->data.regs.x86.msr_efer = ctxt.msr_efer;
+    req->data.regs.x86.msr_star = ctxt.msr_star;
+    req->data.regs.x86.msr_lstar = ctxt.msr_lstar;
 
     hvm_get_segment_register(curr, x86_seg_fs, &seg);
-    req->regs.x86.fs_base = seg.base;
+    req->data.regs.x86.fs_base = seg.base;
 
     hvm_get_segment_register(curr, x86_seg_gs, &seg);
-    req->regs.x86.gs_base = seg.base;
+    req->data.regs.x86.gs_base = seg.base;
 
     hvm_get_segment_register(curr, x86_seg_cs, &seg);
-    req->regs.x86.cs_arbytes = seg.attr.bytes;
+    req->data.regs.x86.cs_arbytes = seg.attr.bytes;
 }
 
 void p2m_mem_access_emulate_check(struct vcpu *v,
@@ -1466,6 +1466,10 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
         }
 
         v->arch.vm_event.emulate_flags = violation ? rsp->flags : 0;
+
+        if ( rsp->flags & MEM_ACCESS_SET_EMUL_READ_DATA &&
+             v->arch.vm_event.emul_read_data )
+            *v->arch.vm_event.emul_read_data = rsp->data.emul_read_data;
     }
 }
 
@@ -1552,9 +1556,15 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
 
     if ( v->arch.vm_event.emulate_flags )
     {
-        hvm_mem_access_emulate_one((v->arch.vm_event.emulate_flags &
-                                    MEM_ACCESS_EMULATE_NOWRITE) != 0,
-                                   TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+        enum emul_kind kind = EMUL_KIND_NORMAL;
+
+        if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_SET_EMUL_READ_DATA )
+            kind = EMUL_KIND_SET_CONTEXT;
+        else if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_EMULATE_NOWRITE )
+            kind = EMUL_KIND_NOWRITE;
+
+        hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
+                                   HVM_DELIVER_NO_ERROR_CODE);
 
         v->arch.vm_event.emulate_flags = 0;
         return 1;
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 120a78a..11438da 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -48,6 +48,7 @@ static int vm_event_enable(
 {
     int rc;
     unsigned long ring_gfn = d->arch.hvm_domain.params[param];
+    struct vcpu *v;
 
     /* Only one helper at a time. If the helper crashed,
      * the ring is in an undefined state and so is the guest.
@@ -63,6 +64,21 @@ static int vm_event_enable(
     vm_event_ring_lock_init(ved);
     vm_event_ring_lock(ved);
 
+    for_each_vcpu( d, v )
+    {
+        if ( v->arch.vm_event.emul_read_data )
+            continue;
+
+        v->arch.vm_event.emul_read_data =
+            xmalloc(struct vm_event_emul_read_data);
+
+        if ( !v->arch.vm_event.emul_read_data )
+        {
+            rc = -ENOMEM;
+            goto err;
+        }
+    }
+
     rc = prepare_ring_for_helper(d, ring_gfn, &ved->ring_pg_struct,
                                     &ved->ring_page);
     if ( rc < 0 )
@@ -225,6 +241,13 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain *ved)
 
         destroy_ring_for_helper(&ved->ring_page,
                                 ved->ring_pg_struct);
+
+        for_each_vcpu( d, v )
+        {
+            xfree(v->arch.vm_event.emul_read_data);
+            v->arch.vm_event.emul_read_data = NULL;
+        }
+
         vm_event_ring_unlock(ved);
     }
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 96bde65..7908844 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -10,6 +10,7 @@
 #include <asm/mce.h>
 #include <public/vcpu.h>
 #include <public/hvm/hvm_info_table.h>
+#include <public/vm_event.h>
 
 #define has_32bit_shinfo(d)    ((d)->arch.has_32bit_shinfo)
 #define is_pv_32bit_domain(d)  ((d)->arch.is_32bit_pv)
@@ -508,6 +509,7 @@ struct arch_vcpu
         uint32_t emulate_flags;
         unsigned long gpa;
         unsigned long eip;
+        struct vm_event_emul_read_data *emul_read_data;
     } vm_event;
 
 };
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index 594be38..49134b5 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -32,13 +32,21 @@ struct hvm_emulate_ctxt {
     struct hvm_trap trap;
 
     uint32_t intr_shadow;
+
+    bool_t set_context;
+};
+
+enum emul_kind {
+    EMUL_KIND_NORMAL,
+    EMUL_KIND_NOWRITE,
+    EMUL_KIND_SET_CONTEXT
 };
 
 int hvm_emulate_one(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
 int hvm_emulate_one_no_write(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
-void hvm_mem_access_emulate_one(bool_t nowrite,
+void hvm_mem_access_emulate_one(enum emul_kind kind,
     unsigned int trapnr,
     unsigned int errcode);
 void hvm_emulate_prepare(
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 577e971..3223bb4 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -149,6 +149,13 @@ struct vm_event_regs_x86 {
  * potentially having side effects (like memory mapped or port I/O) disabled.
  */
 #define MEM_ACCESS_EMULATE_NOWRITE      (1 << 7)
+/*
+ * Data is being sent back to the hypervisor in the event response, to be
+ * returned by the read function when emulating an instruction.
+ * This flag is only useful when combined with MEM_ACCESS_EMULATE or
+ * MEM_ACCESS_EMULATE_NOWRITE.
+ */
+#define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
 
 struct vm_event_mem_access {
     uint64_t gfn;
@@ -189,6 +196,12 @@ struct vm_event_sharing {
     uint32_t _pad;
 };
 
+struct vm_event_emul_read_data {
+    uint32_t size;
+    /* The struct is used in a union with vm_event_regs_x86. */
+    uint8_t  data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)];
+};
+
 typedef struct vm_event_st {
     uint32_t version;   /* VM_EVENT_INTERFACE_VERSION */
     uint32_t flags;     /* VM_EVENT_FLAG_* */
@@ -206,8 +219,12 @@ typedef struct vm_event_st {
     } u;
 
     union {
-        struct vm_event_regs_x86 x86;
-    } regs;
+        union {
+            struct vm_event_regs_x86 x86;
+        } regs;
+
+        struct vm_event_emul_read_data emul_read_data;
+    } data;
 } vm_event_request_t, vm_event_response_t;
 
 DEFINE_RING_TYPES(vm_event, vm_event_request_t, vm_event_response_t);
-- 
1.7.9.5

[PATCH V3 2/3] xen/vm_event: Support for guest-requested events

[Razvan Cojocaru]

Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,
sent via HVMOP_request_vm_event. The guest can request that a
generic vm_event (containing only the vm_event-filled guest registers
as information) be sent to userspace by setting up the correct
registers and doing a VMCALL. For example, for a 32-bit guest, this
means: EAX = 34 (hvmop), EBX = 24 (HVMOP_guest_request_vm_event),
ECX = 0 (NULL required for the hypercall parameter, reserved).

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

---
Changes since V2:
 - Renamed xc_monitor_requested() to xc_monitor_guest_request().
 - Renamed 'requested' or 'request' to 'guest_request' (consistency).
 - Removed the emoved #if defined(__XEN__) || defined(__XEN_TOOLS__)
   restriction to defining HVMOP_guest_request_vm_event.
 - Now requiring NULL as the hypercall parameter.
 - Changed the type of struct monitor's bitfields from uint16_t to
   unsigned int.
---
 tools/libxc/include/xenctrl.h   |    2 ++
 tools/libxc/xc_monitor.c        |   15 +++++++++++++++
 xen/arch/x86/hvm/event.c        |   16 ++++++++++++++++
 xen/arch/x86/hvm/hvm.c          |    8 +++++++-
 xen/arch/x86/monitor.c          |   16 ++++++++++++++++
 xen/include/asm-x86/domain.h    |   16 +++++++++-------
 xen/include/asm-x86/hvm/event.h |    1 +
 xen/include/public/domctl.h     |    6 ++++++
 xen/include/public/hvm/hvm_op.h |    2 ++
 xen/include/public/vm_event.h   |    2 ++
 10 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index d1d2ab3..4ce519a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2384,6 +2384,8 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable,
 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_guest_request(xc_interface *xch, domid_t domain_id,
+                             bool enable, bool sync);
 
 /***
  * Memory sharing operations.
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 63013de..d979122 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -105,3 +105,18 @@ int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id,
 
     return do_domctl(xch, &domctl);
 }
+
+int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
+                             bool sync)
+{
+    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_REQUEST;
+    domctl.u.monitor_op.u.guest_request.sync = sync;
+
+    return do_domctl(xch, &domctl);
+}
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 5341937..17638ea 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -126,6 +126,22 @@ void hvm_event_msr(unsigned int msr, uint64_t value)
         hvm_event_traps(1, &req);
 }
 
+void hvm_event_guest_request(void)
+{
+    struct vcpu *curr = current;
+    struct arch_domain *currad = &curr->domain->arch;
+
+    if ( currad->monitor.guest_request_enabled )
+    {
+        vm_event_request_t req = {
+            .reason = VM_EVENT_REASON_GUEST_REQUEST,
+            .vcpu_id = curr->vcpu_id,
+        };
+
+        hvm_event_traps(currad->monitor.guest_request_sync, &req);
+    }
+}
+
 int hvm_event_int3(unsigned long gla)
 {
     int rc = 0;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 535d622..536d1c8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5974,7 +5974,6 @@ static int hvmop_get_param(
 #define HVMOP_op_mask 0xff
 
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
-
 {
     unsigned long start_iter, mask;
     long rc = 0;
@@ -6388,6 +6387,13 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case HVMOP_guest_request_vm_event:
+        if ( guest_handle_is_null(arg) )
+            hvm_event_guest_request();
+        else
+            rc = -EINVAL;
+        break;
+
     default:
     {
         gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op);
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 896acf7..f8df7d2 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -161,6 +161,22 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
         break;
     }
 
+    case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
+    {
+        bool_t status = ad->monitor.guest_request_enabled;
+
+        rc = status_check(mop, status);
+        if ( rc )
+            return rc;
+
+        ad->monitor.guest_request_sync = mop->u.guest_request.sync;
+
+        domain_pause(d);
+        ad->monitor.guest_request_enabled = !status;
+        domain_unpause(d);
+        break;
+    }
+
     default:
         return -EOPNOTSUPP;
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 7908844..f712caa 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -346,13 +346,15 @@ struct arch_domain
 
     /* Monitor options */
     struct {
-        uint16_t write_ctrlreg_enabled       : 4;
-        uint16_t write_ctrlreg_sync          : 4;
-        uint16_t write_ctrlreg_onchangeonly  : 4;
-        uint16_t mov_to_msr_enabled          : 1;
-        uint16_t mov_to_msr_extended         : 1;
-        uint16_t singlestep_enabled          : 1;
-        uint16_t software_breakpoint_enabled : 1;
+        unsigned int write_ctrlreg_enabled       : 4;
+        unsigned int write_ctrlreg_sync          : 4;
+        unsigned int write_ctrlreg_onchangeonly  : 4;
+        unsigned int mov_to_msr_enabled          : 1;
+        unsigned int mov_to_msr_extended         : 1;
+        unsigned int singlestep_enabled          : 1;
+        unsigned int software_breakpoint_enabled : 1;
+        unsigned int guest_request_enabled       : 1;
+        unsigned int guest_request_sync          : 1;
     } monitor;
 
     /* Mem_access emulation control */
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
index 819ef96..ab5abd0 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -26,6 +26,7 @@ void hvm_event_msr(unsigned int msr, uint64_t value);
 /* Called for current VCPU: returns -1 if no listener */
 int hvm_event_int3(unsigned long gla);
 int hvm_event_single_step(unsigned long gla);
+void hvm_event_guest_request(void);
 
 #endif /* __ASM_X86_HVM_EVENT_H__ */
 
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index bc45ea5..6812016 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1012,6 +1012,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #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
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
@@ -1034,6 +1035,11 @@ struct xen_domctl_monitor_op {
             /* Enable the capture of an extended set of MSRs */
             uint8_t extended_capture;
         } mov_to_msr;
+
+        struct {
+            /* Pause vCPU until response */
+            uint8_t sync;
+        } guest_request;
     } u;
 };
 typedef struct xen_domctl_monitor_op xen_domctl_monitor_op_t;
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 9b84e84..d053db9 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -396,6 +396,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_evtchn_upcall_vector_t);
 
 #endif /* defined(__i386__) || defined(__x86_64__) */
 
+#define HVMOP_guest_request_vm_event 24
+
 #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
 
 /*
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 3223bb4..f0da008 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -68,6 +68,8 @@
 #define VM_EVENT_REASON_SOFTWARE_BREAKPOINT     6
 /* Single-step (e.g. MTF) */
 #define VM_EVENT_REASON_SINGLESTEP              7
+/* An event has been requested via HVMOP_guest_request_vm_event. */
+#define VM_EVENT_REASON_GUEST_REQUEST           8
 
 /* Supported values for the vm_event_write_ctrlreg index. */
 #define VM_EVENT_X86_CR0    0
-- 
1.7.9.5

[PATCH V3 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[Razvan Cojocaru]

Deny register writes if a vm_client subscribed to mov_to_msr or
control register write events forbids them. Currently supported for
MSR, CR0, CR3 and CR4 events.

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

---
Changes since V2:
 - Added a comment to explain why hvm_event_cr() returns bool_t where
   the other event functions return void.
 - Moved an ASSERT() to the beginning of hvm_do_resume() and wrapped
   the d->arch.event_write_data test in an unlikely().
 - Renamed 'event_only' to 'may_defer' in the hvm_set_<register>()
   functions.
 - Added a comment to / fixed VMX_CONTROL_REG_ACCESS_TYPE_CLTS code.
 - Now initializing CR4 before CR3 in hvm_do_resume().
 - Moved an ASSERT() in p2m_mem_access_emulate_check().
 - Changed uint64_t msr to uint32_t msr struct monitor_write_data.
 - Changed uint8_t to unsigned int in the do_write bitfield.
 - Fixed MEM_ACCESS_DENY stale comment and stale patch description.
---
 xen/arch/x86/domain.c             |    2 +
 xen/arch/x86/hvm/emulate.c        |    8 +--
 xen/arch/x86/hvm/event.c          |    5 +-
 xen/arch/x86/hvm/hvm.c            |  120 ++++++++++++++++++++++++++++++++-----
 xen/arch/x86/hvm/svm/nestedsvm.c  |   14 ++---
 xen/arch/x86/hvm/svm/svm.c        |    2 +-
 xen/arch/x86/hvm/vmx/vmx.c        |   14 +++--
 xen/arch/x86/hvm/vmx/vvmx.c       |   18 +++---
 xen/arch/x86/mm/p2m.c             |   29 +++++++++
 xen/common/vm_event.c             |    6 ++
 xen/include/asm-x86/domain.h      |   18 +++++-
 xen/include/asm-x86/hvm/event.h   |    9 ++-
 xen/include/asm-x86/hvm/support.h |    9 +--
 xen/include/public/vm_event.h     |    5 ++
 14 files changed, 213 insertions(+), 46 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index cbbc354..8914aff 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -679,6 +679,8 @@ void arch_domain_destroy(struct domain *d)
     cleanup_domain_irq_mapping(d);
 
     psr_free_rmid(d);
+
+    xfree(d->arch.event_write_data);
 }
 
 void arch_domain_shutdown(struct domain *d)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 3678e29..f0d05fa 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1377,14 +1377,14 @@ static int hvmemul_write_cr(
     switch ( reg )
     {
     case 0:
-        return hvm_set_cr0(val);
+        return hvm_set_cr0(val, 1);
     case 2:
         current->arch.hvm_vcpu.guest_cr[2] = val;
         return X86EMUL_OKAY;
     case 3:
-        return hvm_set_cr3(val);
+        return hvm_set_cr3(val, 1);
     case 4:
-        return hvm_set_cr4(val);
+        return hvm_set_cr4(val, 1);
     default:
         break;
     }
@@ -1405,7 +1405,7 @@ static int hvmemul_write_msr(
     uint64_t val,
     struct x86_emulate_ctxt *ctxt)
 {
-    return hvm_msr_write_intercept(reg, val);
+    return hvm_msr_write_intercept(reg, val, 1);
 }
 
 static int hvmemul_wbinvd(
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 17638ea..042e583 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -90,7 +90,7 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
     return 1;
 }
 
-void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
+bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
 {
     struct arch_domain *currad = &current->domain->arch;
     unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
@@ -109,7 +109,10 @@ void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
 
         hvm_event_traps(currad->monitor.write_ctrlreg_sync & ctrlreg_bitmask,
                         &req);
+        return 1;
     }
+
+    return 0;
 }
 
 void hvm_event_msr(unsigned int msr, uint64_t value)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 536d1c8..6508aa1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -52,6 +52,7 @@
 #include <asm/traps.h>
 #include <asm/mc146818rtc.h>
 #include <asm/mce.h>
+#include <asm/monitor.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/vpt.h>
 #include <asm/hvm/support.h>
@@ -443,6 +444,8 @@ void hvm_do_resume(struct vcpu *v)
     struct domain *d = v->domain;
     struct hvm_ioreq_server *s;
 
+    ASSERT(v == current);
+
     check_wakeup_from_wait();
 
     if ( is_hvm_domain(d) )
@@ -468,6 +471,35 @@ void hvm_do_resume(struct vcpu *v)
         }
     }
 
+    if ( unlikely(d->arch.event_write_data) )
+    {
+        struct monitor_write_data *w = &d->arch.event_write_data[v->vcpu_id];
+
+        if ( w->do_write.msr )
+        {
+            hvm_msr_write_intercept(w->msr, w->value, 0);
+            w->do_write.msr = 0;
+        }
+
+        if ( w->do_write.cr0 )
+        {
+            hvm_set_cr0(w->cr0, 0);
+            w->do_write.cr0 = 0;
+        }
+
+        if ( w->do_write.cr4 )
+        {
+            hvm_set_cr4(w->cr4, 0);
+            w->do_write.cr4 = 0;
+        }
+
+        if ( w->do_write.cr3 )
+        {
+            hvm_set_cr3(w->cr3, 0);
+            w->do_write.cr3 = 0;
+        }
+    }
+
     /* Inject pending hw/sw trap */
     if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) 
     {
@@ -3099,13 +3131,13 @@ int hvm_mov_to_cr(unsigned int cr, unsigned int gpr)
     switch ( cr )
     {
     case 0:
-        return hvm_set_cr0(val);
+        return hvm_set_cr0(val, 1);
 
     case 3:
-        return hvm_set_cr3(val);
+        return hvm_set_cr3(val, 1);
 
     case 4:
-        return hvm_set_cr4(val);
+        return hvm_set_cr4(val, 1);
 
     case 8:
         vlapic_set_reg(vcpu_vlapic(curr), APIC_TASKPRI, ((val & 0x0f) << 4));
@@ -3202,12 +3234,13 @@ static void hvm_update_cr(struct vcpu *v, unsigned int cr, unsigned long value)
     hvm_update_guest_cr(v, cr);
 }
 
-int hvm_set_cr0(unsigned long value)
+int hvm_set_cr0(unsigned long value, bool_t may_defer)
 {
     struct vcpu *v = current;
     struct domain *d = v->domain;
     unsigned long gfn, old_value = v->arch.hvm_vcpu.guest_cr[0];
     struct page_info *page;
+    struct arch_domain *currad = &v->domain->arch;
 
     HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR0 value = %lx", value);
 
@@ -3237,6 +3270,22 @@ int hvm_set_cr0(unsigned long value)
         goto gpf;
     }
 
+    if ( may_defer && unlikely(currad->monitor.write_ctrlreg_enabled &
+                               monitor_ctrlreg_bitmask(VM_EVENT_X86_CR0)) &&
+         value != old_value )
+    {
+        ASSERT(currad->event_write_data != NULL);
+
+        if ( hvm_event_crX(CR0, value, old_value) )
+        {
+            /* The actual write will occur in hvm_do_resume(), if permitted. */
+            currad->event_write_data[v->vcpu_id].do_write.cr0 = 1;
+            currad->event_write_data[v->vcpu_id].cr0 = value;
+
+            return X86EMUL_OKAY;
+        }
+    }
+
     if ( (value & X86_CR0_PG) && !(old_value & X86_CR0_PG) )
     {
         if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
@@ -3303,7 +3352,6 @@ int hvm_set_cr0(unsigned long value)
         hvm_funcs.handle_cd(v, value);
 
     hvm_update_cr(v, 0, value);
-    hvm_event_crX(CR0, value, old_value);
 
     if ( (value ^ old_value) & X86_CR0_PG ) {
         if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
@@ -3319,11 +3367,28 @@ int hvm_set_cr0(unsigned long value)
     return X86EMUL_EXCEPTION;
 }
 
-int hvm_set_cr3(unsigned long value)
+int hvm_set_cr3(unsigned long value, bool_t may_defer)
 {
     struct vcpu *v = current;
     struct page_info *page;
-    unsigned long old;
+    unsigned long old = v->arch.hvm_vcpu.guest_cr[3];
+    struct arch_domain *currad = &v->domain->arch;
+
+    if ( may_defer && unlikely(currad->monitor.write_ctrlreg_enabled &
+                               monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3)) &&
+         value != old )
+    {
+        ASSERT(currad->event_write_data != NULL);
+
+        if ( hvm_event_crX(CR3, value, old) )
+        {
+            /* The actual write will occur in hvm_do_resume(), if permitted. */
+            currad->event_write_data[v->vcpu_id].do_write.cr3 = 1;
+            currad->event_write_data[v->vcpu_id].cr3 = value;
+
+            return X86EMUL_OKAY;
+        }
+    }
 
     if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) &&
          (value != v->arch.hvm_vcpu.guest_cr[3]) )
@@ -3341,10 +3406,8 @@ int hvm_set_cr3(unsigned long value)
         HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR3 value = %lx", value);
     }
 
-    old=v->arch.hvm_vcpu.guest_cr[3];
     v->arch.hvm_vcpu.guest_cr[3] = value;
     paging_update_cr3(v);
-    hvm_event_crX(CR3, value, old);
     return X86EMUL_OKAY;
 
  bad_cr3:
@@ -3353,10 +3416,11 @@ int hvm_set_cr3(unsigned long value)
     return X86EMUL_UNHANDLEABLE;
 }
 
-int hvm_set_cr4(unsigned long value)
+int hvm_set_cr4(unsigned long value, bool_t may_defer)
 {
     struct vcpu *v = current;
     unsigned long old_cr;
+    struct arch_domain *currad = &v->domain->arch;
 
     if ( value & hvm_cr4_guest_reserved_bits(v, 0) )
     {
@@ -3384,8 +3448,23 @@ int hvm_set_cr4(unsigned long value)
         goto gpf;
     }
 
+    if ( may_defer && unlikely(currad->monitor.write_ctrlreg_enabled &
+                               monitor_ctrlreg_bitmask(VM_EVENT_X86_CR4)) &&
+         value != old_cr )
+    {
+        ASSERT(currad->event_write_data != NULL);
+
+        if ( hvm_event_crX(CR4, value, old_cr) )
+        {
+            /* The actual write will occur in hvm_do_resume(), if permitted. */
+            currad->event_write_data[v->vcpu_id].do_write.cr4 = 1;
+            currad->event_write_data[v->vcpu_id].cr4 = value;
+
+            return X86EMUL_OKAY;
+        }
+    }
+
     hvm_update_cr(v, 4, value);
-    hvm_event_crX(CR4, value, old_cr);
 
     /*
      * Modifying CR4.{PSE,PAE,PGE,SMEP}, or clearing CR4.PCIDE
@@ -3849,7 +3928,7 @@ void hvm_task_switch(
         goto out;
 
 
-    if ( hvm_set_cr3(tss.cr3) )
+    if ( hvm_set_cr3(tss.cr3, 1) )
         goto out;
 
     regs->eip    = tss.eip;
@@ -4551,12 +4630,14 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     goto out;
 }
 
-int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
+int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
+                            bool_t may_defer)
 {
     struct vcpu *v = current;
     bool_t mtrr;
     unsigned int edx, index;
     int ret = X86EMUL_OKAY;
+    struct arch_domain *currad = &current->domain->arch;
 
     HVMTRACE_3D(MSR_WRITE, msr,
                (uint32_t)msr_content, (uint32_t)(msr_content >> 32));
@@ -4564,7 +4645,18 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
     hvm_cpuid(1, NULL, NULL, NULL, &edx);
     mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR));
 
-    hvm_event_msr(msr, msr_content);
+    if ( may_defer && unlikely(currad->monitor.mov_to_msr_enabled) )
+    {
+        ASSERT(currad->event_write_data != NULL);
+
+        /* The actual write will occur in hvm_do_resume() (if permitted). */
+        currad->event_write_data[v->vcpu_id].do_write.msr = 1;
+        currad->event_write_data[v->vcpu_id].msr = msr;
+        currad->event_write_data[v->vcpu_id].value = msr_content;
+
+        hvm_event_msr(msr, msr_content);
+        return X86EMUL_OKAY;
+    }
 
     switch ( msr )
     {
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index be5797a..07e3cad 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -274,7 +274,7 @@ int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
 
     /* CR4 */
     v->arch.hvm_vcpu.guest_cr[4] = n1vmcb->_cr4;
-    rc = hvm_set_cr4(n1vmcb->_cr4);
+    rc = hvm_set_cr4(n1vmcb->_cr4, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr4 failed, rc: %u\n", rc);
 
@@ -283,7 +283,7 @@ int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
         svm->ns_cr0, v->arch.hvm_vcpu.guest_cr[0]);
     v->arch.hvm_vcpu.guest_cr[0] = n1vmcb->_cr0 | X86_CR0_PE;
     n1vmcb->rflags &= ~X86_EFLAGS_VM;
-    rc = hvm_set_cr0(n1vmcb->_cr0 | X86_CR0_PE);
+    rc = hvm_set_cr0(n1vmcb->_cr0 | X86_CR0_PE, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr0 failed, rc: %u\n", rc);
     svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0];
@@ -309,7 +309,7 @@ int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
         v->arch.guest_table = pagetable_null();
         /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
     }
-    rc = hvm_set_cr3(n1vmcb->_cr3);
+    rc = hvm_set_cr3(n1vmcb->_cr3, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
 
@@ -534,7 +534,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
 
     /* CR4 */
     v->arch.hvm_vcpu.guest_cr[4] = ns_vmcb->_cr4;
-    rc = hvm_set_cr4(ns_vmcb->_cr4);
+    rc = hvm_set_cr4(ns_vmcb->_cr4, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr4 failed, rc: %u\n", rc);
 
@@ -542,7 +542,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
     svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0];
     cr0 = nestedsvm_fpu_vmentry(svm->ns_cr0, ns_vmcb, n1vmcb, n2vmcb);
     v->arch.hvm_vcpu.guest_cr[0] = ns_vmcb->_cr0;
-    rc = hvm_set_cr0(cr0);
+    rc = hvm_set_cr0(cr0, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr0 failed, rc: %u\n", rc);
 
@@ -558,7 +558,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
         nestedsvm_vmcb_set_nestedp2m(v, ns_vmcb, n2vmcb);
 
         /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
-        rc = hvm_set_cr3(ns_vmcb->_cr3);
+        rc = hvm_set_cr3(ns_vmcb->_cr3, 1);
         if (rc != X86EMUL_OKAY)
             gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
     } else if (paging_mode_hap(v->domain)) {
@@ -570,7 +570,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
          * we assume it intercepts page faults.
          */
         /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
-        rc = hvm_set_cr3(ns_vmcb->_cr3);
+        rc = hvm_set_cr3(ns_vmcb->_cr3, 1);
         if (rc != X86EMUL_OKAY)
             gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
     } else {
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index a02f983..5e39b88 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1945,7 +1945,7 @@ static void svm_do_msr_access(struct cpu_user_regs *regs)
         if ( (inst_len = __get_instruction_length(v, INSTR_WRMSR)) == 0 )
             return;
         msr_content = ((uint64_t)regs->edx << 32) | (uint32_t)regs->eax;
-        rc = hvm_msr_write_intercept(regs->ecx, msr_content);
+        rc = hvm_msr_write_intercept(regs->ecx, msr_content, 1);
     }
 
     if ( rc == X86EMUL_OKAY )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index fc29b89..394feca 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2010,9 +2010,15 @@ static int vmx_cr_access(unsigned long exit_qualification)
     }
     case VMX_CONTROL_REG_ACCESS_TYPE_CLTS: {
         unsigned long old = curr->arch.hvm_vcpu.guest_cr[0];
-        curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
+        unsigned long value = old & ~X86_CR0_TS;
+
+        /*
+         * Special case unlikely to be interesting to a MEM_ACCESS_DENY-capable
+         * application, so the hvm_event_crX() return value is ignored for now.
+         */
+        hvm_event_crX(CR0, value, old);
+        curr->arch.hvm_vcpu.guest_cr[0] = value;
         vmx_update_guest_cr(curr, 0);
-        hvm_event_crX(CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
         HVMTRACE_0D(CLTS);
         break;
     }
@@ -2024,7 +2030,7 @@ static int vmx_cr_access(unsigned long exit_qualification)
                 (VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
                  (X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS));
         HVMTRACE_LONG_1D(LMSW, value);
-        return hvm_set_cr0(value);
+        return hvm_set_cr0(value, 1);
     }
     default:
         BUG();
@@ -3035,7 +3041,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
     {
         uint64_t msr_content;
         msr_content = ((uint64_t)regs->edx << 32) | (uint32_t)regs->eax;
-        if ( hvm_msr_write_intercept(regs->ecx, msr_content) == X86EMUL_OKAY )
+        if ( hvm_msr_write_intercept(regs->ecx, msr_content, 1) == X86EMUL_OKAY )
             update_guest_eip(); /* Safe: WRMSR */
         break;
     }
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index ac6e3b3..52bf39c 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1048,15 +1048,16 @@ static void load_shadow_guest_state(struct vcpu *v)
 
     nvcpu->guest_cr[0] = __get_vvmcs(vvmcs, CR0_READ_SHADOW);
     nvcpu->guest_cr[4] = __get_vvmcs(vvmcs, CR4_READ_SHADOW);
-    hvm_set_cr0(__get_vvmcs(vvmcs, GUEST_CR0));
-    hvm_set_cr4(__get_vvmcs(vvmcs, GUEST_CR4));
-    hvm_set_cr3(__get_vvmcs(vvmcs, GUEST_CR3));
+    hvm_set_cr0(__get_vvmcs(vvmcs, GUEST_CR0), 1);
+    hvm_set_cr4(__get_vvmcs(vvmcs, GUEST_CR4), 1);
+    hvm_set_cr3(__get_vvmcs(vvmcs, GUEST_CR3), 1);
 
     control = __get_vvmcs(vvmcs, VM_ENTRY_CONTROLS);
     if ( control & VM_ENTRY_LOAD_GUEST_PAT )
         hvm_set_guest_pat(v, __get_vvmcs(vvmcs, GUEST_PAT));
     if ( control & VM_ENTRY_LOAD_PERF_GLOBAL_CTRL )
-        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, __get_vvmcs(vvmcs, GUEST_PERF_GLOBAL_CTRL));
+        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
+                                __get_vvmcs(vvmcs, GUEST_PERF_GLOBAL_CTRL), 0);
 
     hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
 
@@ -1249,15 +1250,16 @@ static void load_vvmcs_host_state(struct vcpu *v)
         __vmwrite(vmcs_h2g_field[i].guest_field, r);
     }
 
-    hvm_set_cr0(__get_vvmcs(vvmcs, HOST_CR0));
-    hvm_set_cr4(__get_vvmcs(vvmcs, HOST_CR4));
-    hvm_set_cr3(__get_vvmcs(vvmcs, HOST_CR3));
+    hvm_set_cr0(__get_vvmcs(vvmcs, HOST_CR0), 1);
+    hvm_set_cr4(__get_vvmcs(vvmcs, HOST_CR4), 1);
+    hvm_set_cr3(__get_vvmcs(vvmcs, HOST_CR3), 1);
 
     control = __get_vvmcs(vvmcs, VM_EXIT_CONTROLS);
     if ( control & VM_EXIT_LOAD_HOST_PAT )
         hvm_set_guest_pat(v, __get_vvmcs(vvmcs, HOST_PAT));
     if ( control & VM_EXIT_LOAD_PERF_GLOBAL_CTRL )
-        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, __get_vvmcs(vvmcs, HOST_PERF_GLOBAL_CTRL));
+        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
+                                __get_vvmcs(vvmcs, HOST_PERF_GLOBAL_CTRL), 1);
 
     hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
 
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 5d40d2c..bfa2430 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1417,6 +1417,35 @@ static void p2m_vm_event_fill_regs(vm_event_request_t *req)
 void p2m_mem_access_emulate_check(struct vcpu *v,
                                   const vm_event_response_t *rsp)
 {
+    if ( rsp->flags & MEM_ACCESS_DENY )
+    {
+        struct monitor_write_data *w =
+            &v->domain->arch.event_write_data[v->vcpu_id];
+
+        ASSERT(v->domain->arch.event_write_data != NULL);
+
+        switch ( rsp->reason )
+        {
+        case VM_EVENT_REASON_MOV_TO_MSR:
+            w->do_write.msr = 0;
+            break;
+        case VM_EVENT_REASON_WRITE_CTRLREG:
+            switch ( rsp->u.write_ctrlreg.index )
+            {
+            case VM_EVENT_X86_CR0:
+                w->do_write.cr0 = 0;
+                break;
+            case VM_EVENT_X86_CR3:
+                w->do_write.cr3 = 0;
+                break;
+            case VM_EVENT_X86_CR4:
+                w->do_write.cr4 = 0;
+                break;
+            }
+            break;
+        }
+    }
+
     /* Mark vcpu for skipping one instruction upon rescheduling. */
     if ( rsp->flags & MEM_ACCESS_EMULATE )
     {
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 11438da..e9ac776 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -79,6 +79,10 @@ static int vm_event_enable(
         }
     }
 
+    if ( !d->arch.event_write_data )
+        d->arch.event_write_data = xzalloc_array(struct monitor_write_data,
+                                                 d->max_vcpus);
+
     rc = prepare_ring_for_helper(d, ring_gfn, &ved->ring_pg_struct,
                                     &ved->ring_page);
     if ( rc < 0 )
@@ -410,6 +414,8 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
 
 #ifdef HAS_MEM_ACCESS
         case VM_EVENT_REASON_MEM_ACCESS:
+        case VM_EVENT_REASON_MOV_TO_MSR:
+        case VM_EVENT_REASON_WRITE_CTRLREG:
             mem_access_resume(v, &rsp);
             break;
 #endif
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index f712caa..8990277 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -249,6 +249,21 @@ struct pv_domain
     struct mapcache_domain mapcache;
 };
 
+struct monitor_write_data {
+    struct {
+        unsigned int msr : 1;
+        unsigned int cr0 : 1;
+        unsigned int cr3 : 1;
+        unsigned int cr4 : 1;
+    } do_write;
+
+    uint32_t msr;
+    uint64_t value;
+    uint64_t cr0;
+    uint64_t cr3;
+    uint64_t cr4;
+};
+
 struct arch_domain
 {
     struct page_info *perdomain_l3_pg;
@@ -359,6 +374,8 @@ struct arch_domain
 
     /* Mem_access emulation control */
     bool_t mem_access_emulate_enabled;
+
+    struct monitor_write_data *event_write_data;
 } __cacheline_aligned;
 
 #define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
@@ -513,7 +530,6 @@ struct arch_vcpu
         unsigned long eip;
         struct vm_event_emul_read_data *emul_read_data;
     } vm_event;
-
 };
 
 smap_check_policy_t smap_policy_change(struct vcpu *v,
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
index ab5abd0..c082c20 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -18,8 +18,13 @@
 #ifndef __ASM_X86_HVM_EVENT_H__
 #define __ASM_X86_HVM_EVENT_H__
 
-/* Called for current VCPU on crX/MSR changes by guest */
-void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old);
+/*
+ * Called for current VCPU on crX/MSR changes by guest.
+ * The event might not fire if the client has subscribed to it in onchangeonly
+ * mode, hence the bool_t return type for control register write events.
+ */
+bool_t hvm_event_cr(unsigned int index, unsigned long value,
+                    unsigned long old);
 #define hvm_event_crX(what, new, old) \
     hvm_event_cr(VM_EVENT_X86_##what, new, old)
 void hvm_event_msr(unsigned int msr, uint64_t value);
diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h
index 05ef5c5..95d3bb2 100644
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -124,11 +124,12 @@ void hvm_shadow_handle_cd(struct vcpu *v, unsigned long value);
 
 /* These functions all return X86EMUL return codes. */
 int hvm_set_efer(uint64_t value);
-int hvm_set_cr0(unsigned long value);
-int hvm_set_cr3(unsigned long value);
-int hvm_set_cr4(unsigned long value);
+int hvm_set_cr0(unsigned long value, bool_t may_defer);
+int hvm_set_cr3(unsigned long value, bool_t may_defer);
+int hvm_set_cr4(unsigned long value, bool_t may_defer);
 int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
-int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content);
+int hvm_msr_write_intercept(
+    unsigned int msr, uint64_t msr_content, bool_t may_defer);
 int hvm_mov_to_cr(unsigned int cr, unsigned int gpr);
 int hvm_mov_from_cr(unsigned int cr, unsigned int gpr);
 
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index f0da008..bc97334 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -158,6 +158,11 @@ struct vm_event_regs_x86 {
  * MEM_ACCESS_EMULATE_NOWRITE.
  */
 #define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
+ /*
+  * Deny completion of the operation that triggered the event.
+  * Currently only useful for MSR, CR0, CR3 and CR4 write events.
+  */
+#define MEM_ACCESS_DENY                 (1 << 9)
 
 struct vm_event_mem_access {
     uint64_t gfn;
-- 
1.7.9.5

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Lengyel, Tamas]

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

Handy feature, thanks for doing it!

@@ -1466,6 +1466,10 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>          }
>
>          v->arch.vm_event.emulate_flags = violation ? rsp->flags : 0;
> +
> +        if ( rsp->flags & MEM_ACCESS_SET_EMUL_READ_DATA &&
>

So one of the use-cases for this feature I would have is using it in the
vm_event response to the software breakpoint event. A little bit of
context: I have written 0xCC into target memory locations, which are
further protected by mem_access R/W. This setup right now would suffice to
hide the content easily from the R mem_access events without having to
remove it. But for X tracing I'm not using mem_access events. Here this
feature is locked to be only in response to a mem_access events. I can
*technically* work around that by changing the response type to the
mem_access event, but it would be nice if this feature would be clearly
available for non-mem_access events as well (or at least for software
breakpoint). What do you think, does that usecase make sense here?

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1383 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/3] xen/vm_event: Support for guest-requested events

[Lengyel, Tamas]

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

On Mon, Jul 6, 2015 at 11:51 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,
> sent via HVMOP_request_vm_event. The guest can request that a
> generic vm_event (containing only the vm_event-filled guest registers
> as information) be sent to userspace by setting up the correct
> registers and doing a VMCALL. For example, for a 32-bit guest, this
> means: EAX = 34 (hvmop), EBX = 24 (HVMOP_guest_request_vm_event),
> ECX = 0 (NULL required for the hypercall parameter, reserved).
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>

The vm_event components LGTM.

Acked-by: Tamas K Lengyel <tlengyel@novetta.com>

[-- Attachment #1.2: Type: text/html, Size: 1222 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 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[Lengyel, Tamas]

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

@@ -410,6 +414,8 @@ void vm_event_resume(struct domain *d, struct
vm_event_domain *ved)

>
>  #ifdef HAS_MEM_ACCESS
>          case VM_EVENT_REASON_MEM_ACCESS:
> +        case VM_EVENT_REASON_MOV_TO_MSR:
> +        case VM_EVENT_REASON_WRITE_CTRLREG:
>

This doesn't really make much sense to be associated with MEM_ACCESS. I'm
adding a separate arch-specific vm_event file in my other singlestep patch,
I think these should trigger their appropriate handler there, not in
mem_access_resume.


>              mem_access_resume(v, &rsp);
>              break;
>  #endifdiff --git a/xen/include/public/vm_event.h
> b/xen/include/public/vm_event.h
> index f0da008..bc97334 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -158,6 +158,11 @@ struct vm_event_regs_x86 {
>   * MEM_ACCESS_EMULATE_NOWRITE.
>   */
>  #define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
> + /*
> +  * Deny completion of the operation that triggered the event.
> +  * Currently only useful for MSR, CR0, CR3 and CR4 write events.
> +  */
> +#define MEM_ACCESS_DENY                 (1 << 9)
>
>
Same comment here, this feature is not really denying a mem_access, it
denies register writes. Associating it with mem_access just makes it
confusing. IMHO defining it as a VM_EVENT_FLAG_DENY_REGISTER_CHANGE or
something similar instead would make it a lot more descriptive and inline
with that it is actually doing.

Cheers,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 1944 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 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[Razvan Cojocaru]

On 07/06/2015 08:05 PM, Lengyel, Tamas wrote:
> @@ -410,6 +414,8 @@ void vm_event_resume(struct domain *d, struct
> vm_event_domain *ved)
> 
> 
>      #ifdef HAS_MEM_ACCESS
>              case VM_EVENT_REASON_MEM_ACCESS:
>     +        case VM_EVENT_REASON_MOV_TO_MSR:
>     +        case VM_EVENT_REASON_WRITE_CTRLREG:
> 
> 
> This doesn't really make much sense to be associated with MEM_ACCESS.
> I'm adding a separate arch-specific vm_event file in my other singlestep
> patch, I think these should trigger their appropriate handler there, not
> in mem_access_resume.

To be honest I've somewhat anticipated this critique, and it's
definitely valid. I just wasn't sure if somebody would have said that
it's better to process all responses in one place for now, so I've
decided to try it this way first. But yes, that's actually my preference
as well, so no problem.

>                  mem_access_resume(v, &rsp);
>                  break;
>      #endifdiff --git a/xen/include/public/vm_event.h
>     b/xen/include/public/vm_event.h
>     index f0da008..bc97334 100644
>     --- a/xen/include/public/vm_event.h
>     +++ b/xen/include/public/vm_event.h
>     @@ -158,6 +158,11 @@ struct vm_event_regs_x86 {
>       * MEM_ACCESS_EMULATE_NOWRITE.
>       */
>      #define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
>     + /*
>     +  * Deny completion of the operation that triggered the event.
>     +  * Currently only useful for MSR, CR0, CR3 and CR4 write events.
>     +  */
>     +#define MEM_ACCESS_DENY                 (1 << 9)
> 
> 
> Same comment here, this feature is not really denying a mem_access, it
> denies register writes. Associating it with mem_access just makes it
> confusing. IMHO defining it as a VM_EVENT_FLAG_DENY_REGISTER_CHANGE or
> something similar instead would make it a lot more descriptive and
> inline with that it is actually doing.

Fair enough, I'll think of a more appropriate name for it.


Thanks,
Razvan

Re: [PATCH V3 2/3] xen/vm_event: Support for guest-requested events

[Wei Liu]

On Mon, Jul 06, 2015 at 06:51:12PM +0300, Razvan Cojocaru wrote:
> Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,
> sent via HVMOP_request_vm_event. The guest can request that a
> generic vm_event (containing only the vm_event-filled guest registers
> as information) be sent to userspace by setting up the correct
> registers and doing a VMCALL. For example, for a 32-bit guest, this
> means: EAX = 34 (hvmop), EBX = 24 (HVMOP_guest_request_vm_event),
> ECX = 0 (NULL required for the hypercall parameter, reserved).
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> 

For tools:

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru]

On 07/06/2015 07:50 PM, Lengyel, Tamas wrote:
> Handy feature, thanks for doing it!

You're very welcome, I'm quite happy you find it useful.

>     @@ -1466,6 +1466,10 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>              }
> 
>              v->arch.vm_event.emulate_flags = violation ? rsp->flags : 0;
>     +
>     +        if ( rsp->flags & MEM_ACCESS_SET_EMUL_READ_DATA &&
> 
> 
> So one of the use-cases for this feature I would have is using it in the
> vm_event response to the software breakpoint event. A little bit of
> context: I have written 0xCC into target memory locations, which are
> further protected by mem_access R/W. This setup right now would suffice
> to hide the content easily from the R mem_access events without having
> to remove it. But for X tracing I'm not using mem_access events. Here
> this feature is locked to be only in response to a mem_access events. I
> can *technically* work around that by changing the response type to the
> mem_access event, but it would be nice if this feature would be clearly
> available for non-mem_access events as well (or at least for software
> breakpoint). What do you think, does that usecase make sense here?

It does make a fair ammount of sense, but with this little time until
the feature freeze it does look like a non-trivial change, so if it
doesn't bother you very much to use the plain mem_access event while we
work towards making the response more general, I'd go for trying to get
this patch into 4.6, whatever its chances are.

If you'd prefer that I do some ground work for the future (i.e. rename
MEM_ACCESS constants, etc.), please let me know.


Thanks,
Razvan

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Lengyel, Tamas]

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

>
> If you'd prefer that I do some ground work for the future (i.e. rename
> MEM_ACCESS constants, etc.), please let me know.


Yeap, that sounds reasonable to me.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 443 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 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru]

On 07/06/2015 09:30 PM, Lengyel, Tamas wrote:
>     If you'd prefer that I do some ground work for the future (i.e. rename
>     MEM_ACCESS constants, etc.), please let me know.
> 
>  
> Yeap, that sounds reasonable to me.

Any objections to this renaming?

151 #define MEM_ACCESS_EMULATE_NOWRITE       (1 << 7)
152 /*
153  * Data is being sent back to the hypervisor in the event response,
to be
154  * returned by the read function when emulating an instruction.
155  * This flag is only useful when combined with MEM_ACCESS_EMULATE or
156  * MEM_ACCESS_EMULATE_NOWRITE.
157  * The flag has been defined here because it is used with mem_access
158  * events, and so should not accidentaly overwrite one of the above.
159  */
160 #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 8)

Are there any other small changes you'd like to see in this patch?


Thanks,
Razvan

Re: [PATCH V3 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[Razvan Cojocaru]

On 07/06/2015 08:05 PM, Lengyel, Tamas wrote:
> @@ -410,6 +414,8 @@ void vm_event_resume(struct domain *d, struct
> vm_event_domain *ved)
> 
> 
>      #ifdef HAS_MEM_ACCESS
>              case VM_EVENT_REASON_MEM_ACCESS:
>     +        case VM_EVENT_REASON_MOV_TO_MSR:
>     +        case VM_EVENT_REASON_WRITE_CTRLREG:
> 
> 
> This doesn't really make much sense to be associated with MEM_ACCESS.
> I'm adding a separate arch-specific vm_event file in my other singlestep
> patch, I think these should trigger their appropriate handler there, not
> in mem_access_resume.

As said, I very much agree with the suggestion, but I don't see your
patch in staging yet.

Should I either (with the goal of ideally making the 4.6 release, and of
course unless somebody else has other issues with the patch or this
specific change):

* Add the new file your patch added again in my patch;

* If it's about to be commited soon (?) wait for your patch to make it
into staging (this I think would be the best path, if possible), or

* Leave it as it is for now and follow up post-4.6?


Thanks,
Razvan

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[George Dunlap]

On 07/06/2015 04:51 PM, Razvan Cojocaru wrote:
> This patch adds support for memory-content hiding, by modifying the
> value returned by emulated instructions that read certain memory
> addresses that contain sensitive data. The patch only applies to
> cases where MEM_ACCESS_EMULATE or MEM_ACCESS_EMULATE_NOWRITE have
> been set to a vm_event response.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

MM bits:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH V3 2/3] xen/vm_event: Support for guest-requested events

[George Dunlap]

On 07/06/2015 04:51 PM, Razvan Cojocaru wrote:
> Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,
> sent via HVMOP_request_vm_event. The guest can request that a
> generic vm_event (containing only the vm_event-filled guest registers
> as information) be sent to userspace by setting up the correct
> registers and doing a VMCALL. For example, for a 32-bit guest, this
> means: EAX = 34 (hvmop), EBX = 24 (HVMOP_guest_request_vm_event),
> ECX = 0 (NULL required for the hypercall parameter, reserved).
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> 
> ---
> Changes since V2:
>  - Renamed xc_monitor_requested() to xc_monitor_guest_request().
>  - Renamed 'requested' or 'request' to 'guest_request' (consistency).
>  - Removed the emoved #if defined(__XEN__) || defined(__XEN_TOOLS__)
>    restriction to defining HVMOP_guest_request_vm_event.
>  - Now requiring NULL as the hypercall parameter.
>  - Changed the type of struct monitor's bitfields from uint16_t to
>    unsigned int.
> ---
>  tools/libxc/include/xenctrl.h   |    2 ++
>  tools/libxc/xc_monitor.c        |   15 +++++++++++++++
>  xen/arch/x86/hvm/event.c        |   16 ++++++++++++++++
>  xen/arch/x86/hvm/hvm.c          |    8 +++++++-
>  xen/arch/x86/monitor.c          |   16 ++++++++++++++++
>  xen/include/asm-x86/domain.h    |   16 +++++++++-------
>  xen/include/asm-x86/hvm/event.h |    1 +
>  xen/include/public/domctl.h     |    6 ++++++
>  xen/include/public/hvm/hvm_op.h |    2 ++
>  xen/include/public/vm_event.h   |    2 ++
>  10 files changed, 76 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index d1d2ab3..4ce519a 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2384,6 +2384,8 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable,
>  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_guest_request(xc_interface *xch, domid_t domain_id,
> +                             bool enable, bool sync);
>  
>  /***
>   * Memory sharing operations.
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index 63013de..d979122 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -105,3 +105,18 @@ int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id,
>  
>      return do_domctl(xch, &domctl);
>  }
> +
> +int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
> +                             bool sync)
> +{
> +    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_REQUEST;
> +    domctl.u.monitor_op.u.guest_request.sync = sync;
> +
> +    return do_domctl(xch, &domctl);
> +}
> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index 5341937..17638ea 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -126,6 +126,22 @@ void hvm_event_msr(unsigned int msr, uint64_t value)
>          hvm_event_traps(1, &req);
>  }
>  
> +void hvm_event_guest_request(void)
> +{
> +    struct vcpu *curr = current;
> +    struct arch_domain *currad = &curr->domain->arch;
> +
> +    if ( currad->monitor.guest_request_enabled )
> +    {
> +        vm_event_request_t req = {
> +            .reason = VM_EVENT_REASON_GUEST_REQUEST,
> +            .vcpu_id = curr->vcpu_id,
> +        };
> +
> +        hvm_event_traps(currad->monitor.guest_request_sync, &req);
> +    }
> +}
> +
>  int hvm_event_int3(unsigned long gla)
>  {
>      int rc = 0;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 535d622..536d1c8 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5974,7 +5974,6 @@ static int hvmop_get_param(
>  #define HVMOP_op_mask 0xff
>  
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> -
>  {
>      unsigned long start_iter, mask;
>      long rc = 0;
> @@ -6388,6 +6387,13 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case HVMOP_guest_request_vm_event:
> +        if ( guest_handle_is_null(arg) )
> +            hvm_event_guest_request();
> +        else
> +            rc = -EINVAL;
> +        break;
> +
>      default:
>      {
>          gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op);
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 896acf7..f8df7d2 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -161,6 +161,22 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>          break;
>      }
>  
> +    case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
> +    {
> +        bool_t status = ad->monitor.guest_request_enabled;
> +
> +        rc = status_check(mop, status);
> +        if ( rc )
> +            return rc;
> +
> +        ad->monitor.guest_request_sync = mop->u.guest_request.sync;
> +
> +        domain_pause(d);
> +        ad->monitor.guest_request_enabled = !status;

If I'm reading this right, what this hypercall does is *toggle* guest
requests?

Wouldn't it make more sense to either set it or clear it, rather than
have the caller need to keep track (or guess) what state it's in?

 -George

Re: [PATCH V3 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[George Dunlap]

On 07/06/2015 04:51 PM, Razvan Cojocaru wrote:
> Deny register writes if a vm_client subscribed to mov_to_msr or
> control register write events forbids them. Currently supported for
> MSR, CR0, CR3 and CR4 events.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Haven't done a thorough review; the mm bit looks OK to me:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH V3 2/3] xen/vm_event: Support for guest-requested events

[Razvan Cojocaru]

On 07/07/2015 02:01 PM, George Dunlap wrote:
> On 07/06/2015 04:51 PM, Razvan Cojocaru wrote:
>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index 896acf7..f8df7d2 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -161,6 +161,22 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>>          break;
>>      }
>>  
>> +    case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
>> +    {
>> +        bool_t status = ad->monitor.guest_request_enabled;
>> +
>> +        rc = status_check(mop, status);
>> +        if ( rc )
>> +            return rc;
>> +
>> +        ad->monitor.guest_request_sync = mop->u.guest_request.sync;
>> +
>> +        domain_pause(d);
>> +        ad->monitor.guest_request_enabled = !status;
> 
> If I'm reading this right, what this hypercall does is *toggle* guest
> requests?
> 
> Wouldn't it make more sense to either set it or clear it, rather than
> have the caller need to keep track (or guess) what state it's in?

First of all, thanks for the review!

No, it doesn't just get toggled (you can see that all the other cases
above in that file handle things the same way), although I that was my
impression too when I first came across that code.

The code is a bit involved:

 31 /*
 32  * Sanity check whether option is already enabled/disabled
 33  */
 34 static inline
 35 int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
 36 {
 37     bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
 38
 39     if ( status == requested_status )
 40         return -EEXIST;
 41
 42     return 0;
 43 }

So this function checks to see if the requested status is the same as
the current status. If it's the same, it returns -EEXIST, corresponding
to the "if ( rc ) return rc;" part in the quoted code above.

If it returns 0, then toggling the status is the right thing to do.
Again, the rest of the cases above this one handle it in the exact same
manner, this bit has been more or less of a copy / paste job.


Thanks,
Razvan

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Lengyel, Tamas]

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

On Tue, Jul 7, 2015 at 4:10 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 07/06/2015 09:30 PM, Lengyel, Tamas wrote:
> >     If you'd prefer that I do some ground work for the future (i.e.
> rename
> >     MEM_ACCESS constants, etc.), please let me know.
> >
> >
> > Yeap, that sounds reasonable to me.
>
> Any objections to this renaming?
>
> 151 #define MEM_ACCESS_EMULATE_NOWRITE       (1 << 7)
> 152 /*
> 153  * Data is being sent back to the hypervisor in the event response,
> to be
> 154  * returned by the read function when emulating an instruction.
> 155  * This flag is only useful when combined with MEM_ACCESS_EMULATE or
> 156  * MEM_ACCESS_EMULATE_NOWRITE.
> 157  * The flag has been defined here because it is used with mem_access
> 158  * events, and so should not accidentaly overwrite one of the above.
> 159  */
> 160 #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 8)
>
> Are there any other small changes you'd like to see in this patch?
>

That should suffice - with the flag being move to the VM_EVENT_FLAG list
and the bitshift adjusted accordingly.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1593 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 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru]

On 07/07/2015 03:04 PM, Lengyel, Tamas wrote:
> 
> 
> On Tue, Jul 7, 2015 at 4:10 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     On 07/06/2015 09:30 PM, Lengyel, Tamas wrote:
>     >     If you'd prefer that I do some ground work for the future
>     (i.e. rename
>     >     MEM_ACCESS constants, etc.), please let me know.
>     >
>     >
>     > Yeap, that sounds reasonable to me.
> 
>     Any objections to this renaming?
> 
>     151 #define MEM_ACCESS_EMULATE_NOWRITE       (1 << 7)
>     152 /*
>     153  * Data is being sent back to the hypervisor in the event response,
>     to be
>     154  * returned by the read function when emulating an instruction.
>     155  * This flag is only useful when combined with MEM_ACCESS_EMULATE or
>     156  * MEM_ACCESS_EMULATE_NOWRITE.
>     157  * The flag has been defined here because it is used with mem_access
>     158  * events, and so should not accidentaly overwrite one of the above.
>     159  */
>     160 #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 8)
> 
>     Are there any other small changes you'd like to see in this patch?
> 
> 
> That should suffice - with the flag being move to the VM_EVENT_FLAG list
> and the bitshift adjusted accordingly.

Right, sorry about that, that flag really doesn't have to care about
what the last MEM_ACCESS_ flag bit shift is, since that's only specific
to the mem_access event. For some obviously unexplainable reason I had
been convinced that there's some corner case where that matters but
that's cleary not true.


Thanks,
Razvan

Re: [PATCH V3 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[Lengyel, Tamas]

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

On Tue, Jul 7, 2015 at 5:06 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 07/06/2015 08:05 PM, Lengyel, Tamas wrote:
> > @@ -410,6 +414,8 @@ void vm_event_resume(struct domain *d, struct
> > vm_event_domain *ved)
> >
> >
> >      #ifdef HAS_MEM_ACCESS
> >              case VM_EVENT_REASON_MEM_ACCESS:
> >     +        case VM_EVENT_REASON_MOV_TO_MSR:
> >     +        case VM_EVENT_REASON_WRITE_CTRLREG:
> >
> >
> > This doesn't really make much sense to be associated with MEM_ACCESS.
> > I'm adding a separate arch-specific vm_event file in my other singlestep
> > patch, I think these should trigger their appropriate handler there, not
> > in mem_access_resume.
>
> As said, I very much agree with the suggestion, but I don't see your
> patch in staging yet.
>
> Should I either (with the goal of ideally making the 4.6 release, and of
> course unless somebody else has other issues with the patch or this
> specific change):
>
> * Add the new file your patch added again in my patch;
>
> * If it's about to be commited soon (?) wait for your patch to make it
> into staging (this I think would be the best path, if possible), or
>
> * Leave it as it is for now and follow up post-4.6?
>
>
> Thanks,
> Razvan
>

We can also just coordinate our two patch series. I'll push mine into
github, you can rebase on top of it and submit the entire thing in one
send. How does that sound?

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1967 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 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru]

On 07/07/2015 03:04 PM, Lengyel, Tamas wrote:
> 
> 
> On Tue, Jul 7, 2015 at 4:10 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     On 07/06/2015 09:30 PM, Lengyel, Tamas wrote:
>     >     If you'd prefer that I do some ground work for the future
>     (i.e. rename
>     >     MEM_ACCESS constants, etc.), please let me know.
>     >
>     >
>     > Yeap, that sounds reasonable to me.
> 
>     Any objections to this renaming?
> 
>     151 #define MEM_ACCESS_EMULATE_NOWRITE       (1 << 7)
>     152 /*
>     153  * Data is being sent back to the hypervisor in the event response,
>     to be
>     154  * returned by the read function when emulating an instruction.
>     155  * This flag is only useful when combined with MEM_ACCESS_EMULATE or
>     156  * MEM_ACCESS_EMULATE_NOWRITE.
>     157  * The flag has been defined here because it is used with mem_access
>     158  * events, and so should not accidentaly overwrite one of the above.
>     159  */
>     160 #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 8)
> 
>     Are there any other small changes you'd like to see in this patch?
> 
> 
> That should suffice - with the flag being move to the VM_EVENT_FLAG list
> and the bitshift adjusted accordingly.

Actually, thinking about this more, there is a small problem with the
vm_event.h code:

1417 void p2m_mem_access_emulate_check(struct vcpu *v,
1418                                   const vm_event_response_t *rsp)
1419 {
1420     /* Mark vcpu for skipping one instruction upon rescheduling. */
1421     if ( rsp->flags & MEM_ACCESS_EMULATE )
1422     {
1423         xenmem_access_t access;
1424         bool_t violation = 1;
1425         const struct vm_event_mem_access *data = &rsp->u.mem_access;
1426
1427         if ( p2m_get_mem_access(v->domain, data->gfn, &access) == 0 )
1428         {
1429             switch ( access )
1430             {
1431             case XENMEM_access_n:
1432             case XENMEM_access_n2rwx:
1433             default:
1434                 violation = data->flags & MEM_ACCESS_RWX;

So, in p2m_mem_access_emulate_check() MEM_ACCESS_EMULATE is checked for
in rsp->flags (_not_ data->flags), but then in vm_event.h:

 42 /*
 43  * VCPU_PAUSED in a request signals that the vCPU triggering the
event has been
 44  *  paused
 45  * VCPU_PAUSED in a response signals to unpause the vCPU
 46  */
 47 #define VM_EVENT_FLAG_VCPU_PAUSED        (1 << 0)
 48 /* Flags to aid debugging mem_event */
 49 #define VM_EVENT_FLAG_FOREIGN            (1 << 1)

[...]

126 /*
127  * mem_access flag definitions
128  *
129  * These flags are set only as part of a mem_event request.
130  *
131  * R/W/X: Defines the type of violation that has triggered the event
132  *        Multiple types can be set in a single violation!
133  * GLA_VALID: If the gla field holds a guest VA associated with the
event
134  * FAULT_WITH_GLA: If the violation was triggered by accessing gla
135  * FAULT_IN_GPT: If the violation was triggered during translating gla
136  */
137 #define MEM_ACCESS_R                    (1 << 0)
138 #define MEM_ACCESS_W                    (1 << 1)
139 #define MEM_ACCESS_X                    (1 << 2)
140 #define MEM_ACCESS_RWX                  (MEM_ACCESS_R | MEM_ACCESS_W
| MEM_ACCESS_X)
141 #define MEM_ACCESS_RW                   (MEM_ACCESS_R | MEM_ACCESS_W)
142 #define MEM_ACCESS_RX                   (MEM_ACCESS_R | MEM_ACCESS_X)
143 #define MEM_ACCESS_WX                   (MEM_ACCESS_W | MEM_ACCESS_X)
144 #define MEM_ACCESS_GLA_VALID            (1 << 3)
145 #define MEM_ACCESS_FAULT_WITH_GLA       (1 << 4)
146 #define MEM_ACCESS_FAULT_IN_GPT         (1 << 5)
147 /*
148  * The following flags can be set in the response.
149  *
150  * Emulate the fault-causing instruction (if set in the event
response flags).
151  * This will allow the guest to continue execution without lifting
the page
152  * access restrictions.
153  */
154 #define MEM_ACCESS_EMULATE              (1 << 6)
155 /*
156  * Same as MEM_ACCESS_EMULATE, but with write operations or operations
157  * potentially having side effects (like memory mapped or port I/O)
disabled.
158  */
159 #define MEM_ACCESS_EMULATE_NOWRITE      (1 << 7)

So VM_EVENT_FLAG_FOREIGN (1 << 1), and then MEM_ACCESS_EMULATE (1 << 6).
Now you're adding VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2), and if we're
not very careful, slowly but surely the VM_EVENT_FLAG_ constants will
crawl towards (1 << 6) and start overlapping with MEM_ACCESS_EMULATE,
MEM_ACCESS_EMULATE_NOWRITE and so on, because they're not clearly
#defined right after the rest of the VM_EVENT_FLAG_ ones, are called
MEM_ACCESS_<something> just like the mem_access event specific flags,
and they use bit shifts relative to the MEM_ACCESS_ ones as well. This
is in part what has caused my confusion here.

What do you think?


Thanks,
Razvan

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Lengyel, Tamas]

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

On Tue, Jul 7, 2015 at 9:09 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 07/07/2015 03:04 PM, Lengyel, Tamas wrote:
> >
> >
> > On Tue, Jul 7, 2015 at 4:10 AM, Razvan Cojocaru
> > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> >
> >     On 07/06/2015 09:30 PM, Lengyel, Tamas wrote:
> >     >     If you'd prefer that I do some ground work for the future
> >     (i.e. rename
> >     >     MEM_ACCESS constants, etc.), please let me know.
> >     >
> >     >
> >     > Yeap, that sounds reasonable to me.
> >
> >     Any objections to this renaming?
> >
> >     151 #define MEM_ACCESS_EMULATE_NOWRITE       (1 << 7)
> >     152 /*
> >     153  * Data is being sent back to the hypervisor in the event
> response,
> >     to be
> >     154  * returned by the read function when emulating an instruction.
> >     155  * This flag is only useful when combined with
> MEM_ACCESS_EMULATE or
> >     156  * MEM_ACCESS_EMULATE_NOWRITE.
> >     157  * The flag has been defined here because it is used with
> mem_access
> >     158  * events, and so should not accidentaly overwrite one of the
> above.
> >     159  */
> >     160 #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 8)
> >
> >     Are there any other small changes you'd like to see in this patch?
> >
> >
> > That should suffice - with the flag being move to the VM_EVENT_FLAG list
> > and the bitshift adjusted accordingly.
>
> Actually, thinking about this more, there is a small problem with the
> vm_event.h code:
>
> 1417 void p2m_mem_access_emulate_check(struct vcpu *v,
> 1418                                   const vm_event_response_t *rsp)
> 1419 {
> 1420     /* Mark vcpu for skipping one instruction upon rescheduling. */
> 1421     if ( rsp->flags & MEM_ACCESS_EMULATE )
> 1422     {
> 1423         xenmem_access_t access;
> 1424         bool_t violation = 1;
> 1425         const struct vm_event_mem_access *data = &rsp->u.mem_access;
> 1426
> 1427         if ( p2m_get_mem_access(v->domain, data->gfn, &access) == 0 )
> 1428         {
> 1429             switch ( access )
> 1430             {
> 1431             case XENMEM_access_n:
> 1432             case XENMEM_access_n2rwx:
> 1433             default:
> 1434                 violation = data->flags & MEM_ACCESS_RWX;
>
> So, in p2m_mem_access_emulate_check() MEM_ACCESS_EMULATE is checked for
> in rsp->flags (_not_ data->flags), but then in vm_event.h:
>
>  42 /*
>  43  * VCPU_PAUSED in a request signals that the vCPU triggering the
> event has been
>  44  *  paused
>  45  * VCPU_PAUSED in a response signals to unpause the vCPU
>  46  */
>  47 #define VM_EVENT_FLAG_VCPU_PAUSED        (1 << 0)
>  48 /* Flags to aid debugging mem_event */
>  49 #define VM_EVENT_FLAG_FOREIGN            (1 << 1)
>
> [...]
>
> 126 /*
> 127  * mem_access flag definitions
> 128  *
> 129  * These flags are set only as part of a mem_event request.
> 130  *
> 131  * R/W/X: Defines the type of violation that has triggered the event
> 132  *        Multiple types can be set in a single violation!
> 133  * GLA_VALID: If the gla field holds a guest VA associated with the
> event
> 134  * FAULT_WITH_GLA: If the violation was triggered by accessing gla
> 135  * FAULT_IN_GPT: If the violation was triggered during translating gla
> 136  */
> 137 #define MEM_ACCESS_R                    (1 << 0)
> 138 #define MEM_ACCESS_W                    (1 << 1)
> 139 #define MEM_ACCESS_X                    (1 << 2)
> 140 #define MEM_ACCESS_RWX                  (MEM_ACCESS_R | MEM_ACCESS_W
> | MEM_ACCESS_X)
> 141 #define MEM_ACCESS_RW                   (MEM_ACCESS_R | MEM_ACCESS_W)
> 142 #define MEM_ACCESS_RX                   (MEM_ACCESS_R | MEM_ACCESS_X)
> 143 #define MEM_ACCESS_WX                   (MEM_ACCESS_W | MEM_ACCESS_X)
> 144 #define MEM_ACCESS_GLA_VALID            (1 << 3)
> 145 #define MEM_ACCESS_FAULT_WITH_GLA       (1 << 4)
> 146 #define MEM_ACCESS_FAULT_IN_GPT         (1 << 5)
> 147 /*
> 148  * The following flags can be set in the response.
> 149  *
> 150  * Emulate the fault-causing instruction (if set in the event
> response flags).
> 151  * This will allow the guest to continue execution without lifting
> the page
> 152  * access restrictions.
> 153  */
> 154 #define MEM_ACCESS_EMULATE              (1 << 6)
> 155 /*
> 156  * Same as MEM_ACCESS_EMULATE, but with write operations or operations
> 157  * potentially having side effects (like memory mapped or port I/O)
> disabled.
> 158  */
> 159 #define MEM_ACCESS_EMULATE_NOWRITE      (1 << 7)
>
> So VM_EVENT_FLAG_FOREIGN (1 << 1), and then MEM_ACCESS_EMULATE (1 << 6).
> Now you're adding VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2), and if we're
> not very careful, slowly but surely the VM_EVENT_FLAG_ constants will
> crawl towards (1 << 6) and start overlapping with MEM_ACCESS_EMULATE,
> MEM_ACCESS_EMULATE_NOWRITE and so on, because they're not clearly
> #defined right after the rest of the VM_EVENT_FLAG_ ones, are called
> MEM_ACCESS_<something> just like the mem_access event specific flags,
> and they use bit shifts relative to the MEM_ACCESS_ ones as well. This
> is in part what has caused my confusion here.
>
> What do you think?
>
> Thanks,
> Razvan
>

Well, this is clearly an oversight that we need to fix. MEM_ACCESS_* flags
should be only set on the mem_access flags field.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 6502 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 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru]

On 07/07/2015 04:15 PM, Lengyel, Tamas wrote:
> 
> 
> On Tue, Jul 7, 2015 at 9:09 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     So VM_EVENT_FLAG_FOREIGN (1 << 1), and then MEM_ACCESS_EMULATE (1 << 6).
>     Now you're adding VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2), and if we're
>     not very careful, slowly but surely the VM_EVENT_FLAG_ constants will
>     crawl towards (1 << 6) and start overlapping with MEM_ACCESS_EMULATE,
>     MEM_ACCESS_EMULATE_NOWRITE and so on, because they're not clearly
>     #defined right after the rest of the VM_EVENT_FLAG_ ones, are called
>     MEM_ACCESS_<something> just like the mem_access event specific flags,
>     and they use bit shifts relative to the MEM_ACCESS_ ones as well. This
>     is in part what has caused my confusion here.
> 
>     What do you think?
> 
>     Thanks,
>     Razvan
> 
> 
> Well, this is clearly an oversight that we need to fix. MEM_ACCESS_*
> flags should be only set on the mem_access flags field.

Right, if there are no objections, I'll just submit a small patch (in a
few minutes) that moves MEM_ACCESS_EMULATE and
MEM_ACCESS_EMULATE_NOWRITE up near the VM_EVENT_FLAG_ constants, and
renames them VM_EVENT_FLAG_EMULATE and VM_EVENT_FLAG_EMULATE_NOWRITE.

It seems like this would be a pretty straightforward change, so I assume
it's likely to go in fast.


Thanks,
Razvan

Re: [PATCH V3 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[Razvan Cojocaru]

On 07/07/2015 03:55 PM, Lengyel, Tamas wrote:
> 
> 
> On Tue, Jul 7, 2015 at 5:06 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     On 07/06/2015 08:05 PM, Lengyel, Tamas wrote:
>     > @@ -410,6 +414,8 @@ void vm_event_resume(struct domain *d, struct
>     > vm_event_domain *ved)
>     >
>     >
>     >      #ifdef HAS_MEM_ACCESS
>     >              case VM_EVENT_REASON_MEM_ACCESS:
>     >     +        case VM_EVENT_REASON_MOV_TO_MSR:
>     >     +        case VM_EVENT_REASON_WRITE_CTRLREG:
>     >
>     >
>     > This doesn't really make much sense to be associated with MEM_ACCESS.
>     > I'm adding a separate arch-specific vm_event file in my other singlestep
>     > patch, I think these should trigger their appropriate handler there, not
>     > in mem_access_resume.
> 
>     As said, I very much agree with the suggestion, but I don't see your
>     patch in staging yet.
> 
>     Should I either (with the goal of ideally making the 4.6 release, and of
>     course unless somebody else has other issues with the patch or this
>     specific change):
> 
>     * Add the new file your patch added again in my patch;
> 
>     * If it's about to be commited soon (?) wait for your patch to make it
>     into staging (this I think would be the best path, if possible), or
> 
>     * Leave it as it is for now and follow up post-4.6?
> 
> 
>     Thanks,
>     Razvan
> 
> 
> We can also just coordinate our two patch series. I'll push mine into
> github, you can rebase on top of it and submit the entire thing in one
> send. How does that sound?

That sounds very nice for an ideal scenario (thanks!), but I'm worried
that the additional synchronization overhead will have an additional
negative impact on the initial goal of getting these in before the 4.6
release. As long as moving as fast as possible is desirable, I'd prefer
to only depend on staging / master.

I guess frendliest way to your series to go about it now would be to
just add the file you've added with just my code in it (shouldn't be much).


Thanks,
Razvan

Re: [PATCH V3 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[Lengyel, Tamas]

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

On Tue, Jul 7, 2015 at 9:21 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 07/07/2015 03:55 PM, Lengyel, Tamas wrote:
> >
> >
> > On Tue, Jul 7, 2015 at 5:06 AM, Razvan Cojocaru
> > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> >
> >     On 07/06/2015 08:05 PM, Lengyel, Tamas wrote:
> >     > @@ -410,6 +414,8 @@ void vm_event_resume(struct domain *d, struct
> >     > vm_event_domain *ved)
> >     >
> >     >
> >     >      #ifdef HAS_MEM_ACCESS
> >     >              case VM_EVENT_REASON_MEM_ACCESS:
> >     >     +        case VM_EVENT_REASON_MOV_TO_MSR:
> >     >     +        case VM_EVENT_REASON_WRITE_CTRLREG:
> >     >
> >     >
> >     > This doesn't really make much sense to be associated with
> MEM_ACCESS.
> >     > I'm adding a separate arch-specific vm_event file in my other
> singlestep
> >     > patch, I think these should trigger their appropriate handler
> there, not
> >     > in mem_access_resume.
> >
> >     As said, I very much agree with the suggestion, but I don't see your
> >     patch in staging yet.
> >
> >     Should I either (with the goal of ideally making the 4.6 release,
> and of
> >     course unless somebody else has other issues with the patch or this
> >     specific change):
> >
> >     * Add the new file your patch added again in my patch;
> >
> >     * If it's about to be commited soon (?) wait for your patch to make
> it
> >     into staging (this I think would be the best path, if possible), or
> >
> >     * Leave it as it is for now and follow up post-4.6?
> >
> >
> >     Thanks,
> >     Razvan
> >
> >
> > We can also just coordinate our two patch series. I'll push mine into
> > github, you can rebase on top of it and submit the entire thing in one
> > send. How does that sound?
>
> That sounds very nice for an ideal scenario (thanks!), but I'm worried
> that the additional synchronization overhead will have an additional
> negative impact on the initial goal of getting these in before the 4.6
> release. As long as moving as fast as possible is desirable, I'd prefer
> to only depend on staging / master.
>
> I guess frendliest way to your series to go about it now would be to
> just add the file you've added with just my code in it (shouldn't be much).
>
>
> Thanks,
> Razvan
>

Well, one of us will have to rebase and resend either way we do it =) My
two patches are fixed up and good to go but not sure how long it takes
before they land in staging. Your call.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 3358 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 1/3] xen/mem_access: Support for memory-content hiding

[Jan Beulich]

>>> On 06.07.15 at 17:51, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -269,6 +269,7 @@ struct vcpu *alloc_vcpu_struct(void)
>  
>  void free_vcpu_struct(struct vcpu *v)
>  {
> +    xfree(v->arch.vm_event.emul_read_data);
>      free_xenheap_page(v);
>  }

Please note the function's name - nothing else should be freed here imo.

> @@ -893,6 +915,24 @@ static int hvmemul_cmpxchg(
>      unsigned int bytes,
>      struct x86_emulate_ctxt *ctxt)
>  {
> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> +
> +    if ( unlikely(hvmemul_ctxt->set_context) )
> +    {
> +        struct vcpu *curr = current;
> +
> +        if ( curr->arch.vm_event.emul_read_data )
> +        {

hvmemul_read() returns X86EMUL_UNHANDLEABLE when
!curr->arch.vm_event.emul_read_data. I think this ought to
be consistent.

> +            unsigned int safe_bytes = min_t(unsigned int, bytes,
> +                curr->arch.vm_event.emul_read_data->size);
> +
> +            memset(p_new, 0, bytes);
> +            memcpy(p_new, curr->arch.vm_event.emul_read_data->data,
> +                   safe_bytes);

Here as well as in elsewhere - pretty inefficient to zero the
whole array in order to then possibly overwrite all of it. I'd really
like to see only the excess bytes to be zeroed.

> @@ -935,6 +975,41 @@ static int hvmemul_rep_ins(
>                                 !!(ctxt->regs->eflags & X86_EFLAGS_DF), gpa);
>  }
>  
> +static int hvmemul_rep_outs_set_context(
> +    enum x86_segment src_seg,
> +    unsigned long src_offset,
> +    uint16_t dst_port,
> +    unsigned int bytes_per_rep,
> +    unsigned long *reps,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    unsigned int bytes = *reps * bytes_per_rep;
> +    struct vcpu *curr = current;
> +    unsigned int safe_bytes;
> +    char *buf = NULL;
> +    int rc;
> +
> +    if ( !curr->arch.vm_event.emul_read_data )
> +        return X86EMUL_UNHANDLEABLE;
> +
> +    buf = xmalloc_bytes(bytes);
> +
> +    if ( buf == NULL )
> +        return X86EMUL_UNHANDLEABLE;
> +
> +    memset(buf, 0, bytes);

If this were to stay (see above), xzalloc_array() above please. And
xmalloc_array() otherwise.

> @@ -1063,7 +1142,20 @@ static int hvmemul_rep_movs(
>       */
>      rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
>      if ( rc == HVMCOPY_okay )
> +    {
> +        struct vcpu *curr = current;
> +
> +        if ( unlikely(hvmemul_ctxt->set_context) &&
> +             curr->arch.vm_event.emul_read_data )
> +        {
> +            unsigned int safe_bytes = min_t(unsigned int, bytes,
> +                curr->arch.vm_event.emul_read_data->size);
> +
> +            memcpy(buf, curr->arch.vm_event.emul_read_data->data, safe_bytes);
> +        }

This again is inconsistent with what you do above: You need to
decide whether excess data (beyond safe_bytes) is safe to read
(like you do here) or should be returned as zeroes (as done above).

> @@ -1599,6 +1711,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>  int hvm_emulate_one(
>      struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
> +    hvmemul_ctxt->set_context = 0;

I think this would better go into hvm_emulate_prepare().

> @@ -1616,10 +1736,16 @@ void hvm_mem_access_emulate_one(bool_t nowrite, 
> unsigned int trapnr,
>  
>      hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
>  
> -    if ( nowrite )
> +    switch ( kind ) {

Coding style (also elsewhere in the patch).

> +    case EMUL_KIND_NOWRITE:
>          rc = hvm_emulate_one_no_write(&ctx);
> -    else
> +        break;
> +    case EMUL_KIND_SET_CONTEXT:
> +        rc = hvm_emulate_one_set_context(&ctx);

Unless you see future uses for it, please expand the wrapper here.

> @@ -1552,9 +1556,15 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>  
>      if ( v->arch.vm_event.emulate_flags )
>      {
> -        hvm_mem_access_emulate_one((v->arch.vm_event.emulate_flags &
> -                                    MEM_ACCESS_EMULATE_NOWRITE) != 0,
> -                                   TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
> +        enum emul_kind kind = EMUL_KIND_NORMAL;
> +
> +        if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_SET_EMUL_READ_DATA )
> +            kind = EMUL_KIND_SET_CONTEXT;
> +        else if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_EMULATE_NOWRITE )

Is there code in place rejecting both flags being set at once? I don't
recall having seen any...

Jan

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Lengyel, Tamas]

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

On Tue, Jul 7, 2015 at 9:21 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 07/07/2015 04:15 PM, Lengyel, Tamas wrote:
> >
> >
> > On Tue, Jul 7, 2015 at 9:09 AM, Razvan Cojocaru
> > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> >
> >     So VM_EVENT_FLAG_FOREIGN (1 << 1), and then MEM_ACCESS_EMULATE (1 <<
> 6).
> >     Now you're adding VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2), and if
> we're
> >     not very careful, slowly but surely the VM_EVENT_FLAG_ constants will
> >     crawl towards (1 << 6) and start overlapping with MEM_ACCESS_EMULATE,
> >     MEM_ACCESS_EMULATE_NOWRITE and so on, because they're not clearly
> >     #defined right after the rest of the VM_EVENT_FLAG_ ones, are called
> >     MEM_ACCESS_<something> just like the mem_access event specific flags,
> >     and they use bit shifts relative to the MEM_ACCESS_ ones as well.
> This
> >     is in part what has caused my confusion here.
> >
> >     What do you think?
> >
> >     Thanks,
> >     Razvan
> >
> >
> > Well, this is clearly an oversight that we need to fix. MEM_ACCESS_*
> > flags should be only set on the mem_access flags field.
>
> Right, if there are no objections, I'll just submit a small patch (in a
> few minutes) that moves MEM_ACCESS_EMULATE and
> MEM_ACCESS_EMULATE_NOWRITE up near the VM_EVENT_FLAG_ constants, and
> renames them VM_EVENT_FLAG_EMULATE and VM_EVENT_FLAG_EMULATE_NOWRITE.
>
> It seems like this would be a pretty straightforward change, so I assume
> it's likely to go in fast.
>
>
> Thanks,
> Razvan
>

Sounds good. Probably it shouldn't take long to land that small fix in
staging. But we will have conflicts as my patches introduce new
VM_EVENT_FLAGs too so depending on the order of things, it's a PITA ;)

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2507 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/3] xen/vm_event: Support for guest-requested events

[Jan Beulich]

>>> On 06.07.15 at 17:51, <rcojocaru@bitdefender.com> wrote:
> @@ -6388,6 +6387,13 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case HVMOP_guest_request_vm_event:
> +        if ( guest_handle_is_null(arg) )
> +            hvm_event_guest_request();
> +        else
> +            rc = -EINVAL;
> +        break;

Pending Daniel's confirmation that not having an XSM check here
is okay,
Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH V3 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[Jan Beulich]

>>> On 06.07.15 at 17:51, <rcojocaru@bitdefender.com> wrote:
> @@ -443,6 +444,8 @@ void hvm_do_resume(struct vcpu *v)
>      struct domain *d = v->domain;
>      struct hvm_ioreq_server *s;
>  
> +    ASSERT(v == current);

This seems rather pointless in this function - nothing would work if that
wasn't the case.

With that removed,
Acked-by: Jan Beulich <jbeulich@suse.com>

Also ...

> @@ -468,6 +471,35 @@ void hvm_do_resume(struct vcpu *v)
>          }
>      }
>  
> +    if ( unlikely(d->arch.event_write_data) )
> +    {
> +        struct monitor_write_data *w = &d->arch.event_write_data[v->vcpu_id];
> +
> +        if ( w->do_write.msr )
> +        {
> +            hvm_msr_write_intercept(w->msr, w->value, 0);
> +            w->do_write.msr = 0;
> +        }
> +
> +        if ( w->do_write.cr0 )
> +        {
> +            hvm_set_cr0(w->cr0, 0);
> +            w->do_write.cr0 = 0;
> +        }
> +
> +        if ( w->do_write.cr4 )
> +        {
> +            hvm_set_cr4(w->cr4, 0);
> +            w->do_write.cr4 = 0;
> +        }
> +
> +        if ( w->do_write.cr3 )
> +        {
> +            hvm_set_cr3(w->cr3, 0);
> +            w->do_write.cr3 = 0;
> +        }
> +    }

... despite the CR ordering now being better, I continue to not be
convinced of this model when it comes to multiple updates happening
together. Yet that's not meant as a reason for the patch not to go
in; it's just a (maybe only) theoretical issue I see, and that I would
think _you_ want addressed.

Jan

Re: [PATCH V3 2/3] xen/vm_event: Support for guest-requested events

[Daniel De Graaf]

On 07/07/2015 09:30 AM, Jan Beulich wrote:
>>>> On 06.07.15 at 17:51, <rcojocaru@bitdefender.com> wrote:
>> @@ -6388,6 +6387,13 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           break;
>>       }
>>
>> +    case HVMOP_guest_request_vm_event:
>> +        if ( guest_handle_is_null(arg) )
>> +            hvm_event_guest_request();
>> +        else
>> +            rc = -EINVAL;
>> +        break;
>
> Pending Daniel's confirmation that not having an XSM check here
> is okay,
> Acked-by: Jan Beulich <jbeulich@suse.com>

I don't think an XSM check is needed here, if only because the events are
being added to an existing channel from the guest to the monitor.  The
best way to control this communication is probably when the shared page is
mapped by the monitor, but this is an existing mechanism which appears to
be covered by the ability to map any page in the target domain.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru]

On 07/07/2015 04:27 PM, Jan Beulich wrote:
>>>> On 06.07.15 at 17:51, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -269,6 +269,7 @@ struct vcpu *alloc_vcpu_struct(void)
>>  
>>  void free_vcpu_struct(struct vcpu *v)
>>  {
>> +    xfree(v->arch.vm_event.emul_read_data);
>>      free_xenheap_page(v);
>>  }
> 
> Please note the function's name - nothing else should be freed here imo.

Ack, moved it to alloc_vcpu() and complete_domain_destroy() (the callers
of free_vcpu_struct(), in xen/common/domain.c).

>> @@ -893,6 +915,24 @@ static int hvmemul_cmpxchg(
>>      unsigned int bytes,
>>      struct x86_emulate_ctxt *ctxt)
>>  {
>> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
>> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>> +
>> +    if ( unlikely(hvmemul_ctxt->set_context) )
>> +    {
>> +        struct vcpu *curr = current;
>> +
>> +        if ( curr->arch.vm_event.emul_read_data )
>> +        {
> 
> hvmemul_read() returns X86EMUL_UNHANDLEABLE when
> !curr->arch.vm_event.emul_read_data. I think this ought to
> be consistent.

Ack.

>> +            unsigned int safe_bytes = min_t(unsigned int, bytes,
>> +                curr->arch.vm_event.emul_read_data->size);
>> +
>> +            memset(p_new, 0, bytes);
>> +            memcpy(p_new, curr->arch.vm_event.emul_read_data->data,
>> +                   safe_bytes);
> 
> Here as well as in elsewhere - pretty inefficient to zero the
> whole array in order to then possibly overwrite all of it. I'd really
> like to see only the excess bytes to be zeroed.

Ack, now zeroing only the remainder.

>> @@ -935,6 +975,41 @@ static int hvmemul_rep_ins(
>>                                 !!(ctxt->regs->eflags & X86_EFLAGS_DF), gpa);
>>  }
>>  
>> +static int hvmemul_rep_outs_set_context(
>> +    enum x86_segment src_seg,
>> +    unsigned long src_offset,
>> +    uint16_t dst_port,
>> +    unsigned int bytes_per_rep,
>> +    unsigned long *reps,
>> +    struct x86_emulate_ctxt *ctxt)
>> +{
>> +    unsigned int bytes = *reps * bytes_per_rep;
>> +    struct vcpu *curr = current;
>> +    unsigned int safe_bytes;
>> +    char *buf = NULL;
>> +    int rc;
>> +
>> +    if ( !curr->arch.vm_event.emul_read_data )
>> +        return X86EMUL_UNHANDLEABLE;
>> +
>> +    buf = xmalloc_bytes(bytes);
>> +
>> +    if ( buf == NULL )
>> +        return X86EMUL_UNHANDLEABLE;
>> +
>> +    memset(buf, 0, bytes);
> 
> If this were to stay (see above), xzalloc_array() above please. And
> xmalloc_array() otherwise.

Changed it to xmalloc_array() and zeroing out only the reminder.

>> @@ -1063,7 +1142,20 @@ static int hvmemul_rep_movs(
>>       */
>>      rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
>>      if ( rc == HVMCOPY_okay )
>> +    {
>> +        struct vcpu *curr = current;
>> +
>> +        if ( unlikely(hvmemul_ctxt->set_context) &&
>> +             curr->arch.vm_event.emul_read_data )
>> +        {
>> +            unsigned int safe_bytes = min_t(unsigned int, bytes,
>> +                curr->arch.vm_event.emul_read_data->size);
>> +
>> +            memcpy(buf, curr->arch.vm_event.emul_read_data->data, safe_bytes);
>> +        }
> 
> This again is inconsistent with what you do above: You need to
> decide whether excess data (beyond safe_bytes) is safe to read
> (like you do here) or should be returned as zeroes (as done above).

An omission, now fixed.

>> @@ -1599,6 +1711,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>>  int hvm_emulate_one(
>>      struct hvm_emulate_ctxt *hvmemul_ctxt)
>>  {
>> +    hvmemul_ctxt->set_context = 0;
> 
> I think this would better go into hvm_emulate_prepare().

Ack.

>> @@ -1616,10 +1736,16 @@ void hvm_mem_access_emulate_one(bool_t nowrite, 
>> unsigned int trapnr,
>>  
>>      hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
>>  
>> -    if ( nowrite )
>> +    switch ( kind ) {
> 
> Coding style (also elsewhere in the patch).

Sorry about that, fixed.

>> +    case EMUL_KIND_NOWRITE:
>>          rc = hvm_emulate_one_no_write(&ctx);
>> -    else
>> +        break;
>> +    case EMUL_KIND_SET_CONTEXT:
>> +        rc = hvm_emulate_one_set_context(&ctx);
> 
> Unless you see future uses for it, please expand the wrapper here.

Now that setting set_context to 0 happens in hvm_emulate_one_no_write()
I'm just setting it to 1 and falling through to the default case here.

>> @@ -1552,9 +1556,15 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>  
>>      if ( v->arch.vm_event.emulate_flags )
>>      {
>> -        hvm_mem_access_emulate_one((v->arch.vm_event.emulate_flags &
>> -                                    MEM_ACCESS_EMULATE_NOWRITE) != 0,
>> -                                   TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
>> +        enum emul_kind kind = EMUL_KIND_NORMAL;
>> +
>> +        if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_SET_EMUL_READ_DATA )
>> +            kind = EMUL_KIND_SET_CONTEXT;
>> +        else if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_EMULATE_NOWRITE )
> 
> Is there code in place rejecting both flags being set at once? I don't
> recall having seen any...

No, there isn't. Both flags can be set at once, but if so only the
SET_EMUL_READ_DATA will be honored.


Thanks,
Razvan

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Jan Beulich]

>>> On 07.07.15 at 17:32, <rcojocaru@bitdefender.com> wrote:
> On 07/07/2015 04:27 PM, Jan Beulich wrote:
>>>>> On 06.07.15 at 17:51, <rcojocaru@bitdefender.com> wrote:
>>> @@ -1552,9 +1556,15 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>  
>>>      if ( v->arch.vm_event.emulate_flags )
>>>      {
>>> -        hvm_mem_access_emulate_one((v->arch.vm_event.emulate_flags &
>>> -                                    MEM_ACCESS_EMULATE_NOWRITE) != 0,
>>> -                                   TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
>>> +        enum emul_kind kind = EMUL_KIND_NORMAL;
>>> +
>>> +        if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_SET_EMUL_READ_DATA )
>>> +            kind = EMUL_KIND_SET_CONTEXT;
>>> +        else if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_EMULATE_NOWRITE )
>> 
>> Is there code in place rejecting both flags being set at once? I don't
>> recall having seen any...
> 
> No, there isn't. Both flags can be set at once, but if so only the
> SET_EMUL_READ_DATA will be honored.

But to me, purely theoretically setting both flags together makes
sense, and hence this combination, if it isn't working today,
shouldn't result in unexpected behavior (perhaps differing from
what a future Xen version might do).

Jan

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru]

On 07/07/2015 06:40 PM, Jan Beulich wrote:
>>>> On 07.07.15 at 17:32, <rcojocaru@bitdefender.com> wrote:
>> On 07/07/2015 04:27 PM, Jan Beulich wrote:
>>>>>> On 06.07.15 at 17:51, <rcojocaru@bitdefender.com> wrote:
>>>> @@ -1552,9 +1556,15 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>>  
>>>>      if ( v->arch.vm_event.emulate_flags )
>>>>      {
>>>> -        hvm_mem_access_emulate_one((v->arch.vm_event.emulate_flags &
>>>> -                                    MEM_ACCESS_EMULATE_NOWRITE) != 0,
>>>> -                                   TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
>>>> +        enum emul_kind kind = EMUL_KIND_NORMAL;
>>>> +
>>>> +        if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_SET_EMUL_READ_DATA )
>>>> +            kind = EMUL_KIND_SET_CONTEXT;
>>>> +        else if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_EMULATE_NOWRITE )
>>>
>>> Is there code in place rejecting both flags being set at once? I don't
>>> recall having seen any...
>>
>> No, there isn't. Both flags can be set at once, but if so only the
>> SET_EMUL_READ_DATA will be honored.
> 
> But to me, purely theoretically setting both flags together makes
> sense, and hence this combination, if it isn't working today,
> shouldn't result in unexpected behavior (perhaps differing from
> what a future Xen version might do).

That's a very good point. I have tried to be as clear about this as
possible in the code by using an enum for the possible emulation kinds,
instead of #defines or something potentially interpretable as bit
setters. But due to the design of the vm_event mechanism it's not
possible to return an error when the response contains OR-ed bits that
don't belong together.

The behaviour now is that NOWRITE outranks the plain EMULATE, and
SET_CONTEXT outranks them both, and there are no plans to change this in
the future. I suppose a comment stating this clearly belongs in vm_event.h?


Thanks,
Razvan

Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Jan Beulich]

>>> On 07.07.15 at 18:20, <rcojocaru@bitdefender.com> wrote:
> On 07/07/2015 06:40 PM, Jan Beulich wrote:
>>>>> On 07.07.15 at 17:32, <rcojocaru@bitdefender.com> wrote:
>>> On 07/07/2015 04:27 PM, Jan Beulich wrote:
>>>>>>> On 06.07.15 at 17:51, <rcojocaru@bitdefender.com> wrote:
>>>>> @@ -1552,9 +1556,15 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>>>  
>>>>>      if ( v->arch.vm_event.emulate_flags )
>>>>>      {
>>>>> -        hvm_mem_access_emulate_one((v->arch.vm_event.emulate_flags &
>>>>> -                                    MEM_ACCESS_EMULATE_NOWRITE) != 0,
>>>>> -                                   TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
>>>>> +        enum emul_kind kind = EMUL_KIND_NORMAL;
>>>>> +
>>>>> +        if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_SET_EMUL_READ_DATA )
>>>>> +            kind = EMUL_KIND_SET_CONTEXT;
>>>>> +        else if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_EMULATE_NOWRITE )
>>>>
>>>> Is there code in place rejecting both flags being set at once? I don't
>>>> recall having seen any...
>>>
>>> No, there isn't. Both flags can be set at once, but if so only the
>>> SET_EMUL_READ_DATA will be honored.
>> 
>> But to me, purely theoretically setting both flags together makes
>> sense, and hence this combination, if it isn't working today,
>> shouldn't result in unexpected behavior (perhaps differing from
>> what a future Xen version might do).
> 
> That's a very good point. I have tried to be as clear about this as
> possible in the code by using an enum for the possible emulation kinds,
> instead of #defines or something potentially interpretable as bit
> setters. But due to the design of the vm_event mechanism it's not
> possible to return an error when the response contains OR-ed bits that
> don't belong together.
> 
> The behaviour now is that NOWRITE outranks the plain EMULATE, and
> SET_CONTEXT outranks them both, and there are no plans to change this in
> the future. I suppose a comment stating this clearly belongs in vm_event.h?

I would say so, yes.

Jan