[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH 15/16] x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes



The arch_vm_event.monitor structure is dynamically allocated and freed e.g. when
the toolstack user calls xc_monitor_disable(), which in turn effectively
discards any information that was in arch_vm_event.monitor->write_data.

But this can yield unexpected behavior since if a CR-write was awaiting to be
committed (non-zero write_data.writes_pending) on the scheduling tail
(hvm_do_resume()->monitor_ctrlreg_write_data()) before xc_monitor_disable() is
called, then the domain CR-write is wrongfully ignored, which of course, in
these cases, can easily render a domain crash.

To fix the issue, this patch:

* makes arch_vm_event.monitor->emul_read_data dynamically allocated and only
  frees the entire arch_vm_event.monitor in monitor_cleanup_domain() if there
  are no pending CR-writes, otherwise it only frees emul_read_data

* if there were pending CR-writes when monitor_cleanup_domain() was called,
  arch_vm_event.monitor will eventually be freed either after the CR-writes are
  committed (monitor_ctrlreg_write_data()) or when the domain/vcpu is destroyed

* calls monitor_ctrlreg_write_data() even if the monitor subsystem is not
  initialized

Signed-off-by: Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>
---
 xen/arch/x86/hvm/emulate.c   |  4 ++--
 xen/arch/x86/hvm/hvm.c       |  7 ++++++-
 xen/arch/x86/mm/p2m.c        |  2 +-
 xen/arch/x86/monitor.c       | 49 +++++++++++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/domain.h |  3 +--
 5 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 7a9c6af..e08d9c6 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -77,9 +77,9 @@ static int set_context_data(void *buffer, unsigned int size)
     if ( unlikely(monitor_domain_initialised(curr->domain)) )
     {
         struct arch_vm_event_monitor *vem = curr->arch.vm_event->monitor;
-        unsigned int safe_size = min(size, vem->emul_read_data.size);
+        unsigned int safe_size = min(size, vem->emul_read_data->size);
 
-        memcpy(buffer, vem->emul_read_data.data, safe_size);
+        memcpy(buffer, vem->emul_read_data->data, safe_size);
         memset(buffer + safe_size, 0, size - safe_size);
         return X86EMUL_OKAY;
     }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 1bfd9d4..a2568fd 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -488,9 +488,14 @@ void hvm_do_resume(struct vcpu *v)
 
             vem->emulate_flags = 0;
         }
+    }
 
+    /* 
+     * Handle pending monitor CR-writes. Note that the monitor subsystem
+     * might be uninitialized.
+     */
+    if ( unlikely(v->arch.vm_event && v->arch.vm_event->monitor) )
         monitor_ctrlreg_write_data(v);
-    }
 
     /* Inject pending hw/sw trap */
     if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 92c0576..b280dc0 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1645,7 +1645,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
         vem->emulate_flags = violation ? rsp->flags : 0;
 
         if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
-            vem->emul_read_data = rsp->data.emul_read_data;
+            *vem->emul_read_data = rsp->data.emul_read_data;
     }
 }
 
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index c3123bc..05a2f0d 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -56,6 +56,7 @@ int monitor_init_domain(struct domain *d)
 {
     int rc = 0;
     struct vcpu *v;
+    struct arch_vm_event_monitor *vem;
 
     /* Already initialised? */
     if ( unlikely(monitor_domain_initialised(d)) )
@@ -65,10 +66,22 @@ int monitor_init_domain(struct domain *d)
     for_each_vcpu(d, v)
     {
         ASSERT(v->arch.vm_event);
-        ASSERT(!v->arch.vm_event->monitor);
 
-        v->arch.vm_event->monitor = xzalloc(struct arch_vm_event_monitor);
-        if ( unlikely(!v->arch.vm_event->monitor) )
+        if ( likely(!v->arch.vm_event->monitor) )
+        {
+            v->arch.vm_event->monitor = xzalloc(struct arch_vm_event_monitor);
+            if ( unlikely(!v->arch.vm_event->monitor) )
+            {
+                rc = -ENOMEM;
+                goto err;
+            }
+        }
+
+        vem = v->arch.vm_event->monitor;
+
+        ASSERT(!vem->emul_read_data);
+        vem->emul_read_data = xzalloc(struct vm_event_emul_read_data);
+        if ( unlikely(!vem->emul_read_data) )
         {
             rc = -ENOMEM;
             goto err;
@@ -100,6 +113,7 @@ err:
 void monitor_cleanup_domain(struct domain *d)
 {
     struct vcpu *v;
+    struct arch_vm_event_monitor *vem;
 
     xfree(d->arch.monitor.msr_bitmap);
     d->arch.monitor.msr_bitmap = NULL;
@@ -109,8 +123,22 @@ void monitor_cleanup_domain(struct domain *d)
         if ( unlikely(!v->arch.vm_event || !v->arch.vm_event->monitor) )
             continue;
 
-        xfree(v->arch.vm_event->monitor);
-        v->arch.vm_event->monitor = NULL;
+        vem = v->arch.vm_event->monitor;
+        xfree(vem->emul_read_data);
+        vem->emul_read_data = NULL;
+
+        /*
+         * Only invalidate write_data if no CR-writes are pending or domain is
+         * dead (domain_kill(d) code-path). If CR-writes are pending, 
write_data
+         * will eventually be freed when those writes are committed (see
+         * monitor_ctrlreg_write_data() function).
+         */
+        if ( likely(!vem->write_data.writes_pending) ||
+             unlikely(DOMDYING_dead == d->is_dying) )
+        {
+           xfree(vem);
+           v->arch.vm_event->monitor = NULL;
+        }
     }
 
     monitor_ctrlreg_disable_traps(d);
@@ -192,6 +220,17 @@ void monitor_ctrlreg_write_data(struct vcpu *v)
         hvm_set_cr3(w->cr3, 0);
         w->do_write.cr3 = 0;
     }
+
+    /*
+     * Was the monitor subsystem actually uninitialized
+     * and only waiting for pending CR-writes?
+     */
+    if ( unlikely(!monitor_domain_initialised(v->domain)) )
+    {
+        xfree(v->arch.vm_event->monitor);
+        xfree(v->arch.vm_event);
+        v->arch.vm_event = NULL;
+    }
 }
 
 static unsigned long *monitor_bitmap_for_msr(const struct domain *d, u32 *msr)
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 7663da2..e337cb3 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -9,7 +9,6 @@
 #include <asm/e820.h>
 #include <asm/mce.h>
 #include <public/vcpu.h>
-#include <public/vm_event.h>
 #include <public/hvm/hvm_info_table.h>
 
 #define has_32bit_shinfo(d)    ((d)->arch.has_32bit_shinfo)
@@ -510,7 +509,7 @@ typedef enum __packed {
  */
 struct arch_vm_event_monitor {
     uint32_t emulate_flags;
-    struct vm_event_emul_read_data emul_read_data;
+    struct vm_event_emul_read_data *emul_read_data;
     struct monitor_write_data write_data;
 };
 
-- 
2.5.0


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.