x86/hap: fix race condition between ENABLE_LOGDIRTY and track_dirty_vram hypercall

There is a race condition between XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY and HVMOP_track_dirty_vram hypercall.

Although HVMOP_track_dirty_vram is called many times from qemu-dm which is connected via VNC, XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY is called only once from a migration process (e.g. xc_save, libxl-save-helper). So the race seldom happens, but the following cases are possible.

========================================================================
[case-1]
XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY hypercall
-> paging_enable_logdirty()
-> hap_logdirty_init()

    -> paging_log_dirty_disable()
       dirty_vram = NULL
    -> paging_log_dirty_init(d, hap_enable_log_dirty, ...) ---> (A)
  -> paging_log_dirty_enable()
**************************************************************************
    /* <--- (B) */
    -> hap_enable_vram_tracking() // should be hap_enable_log_dirty() !!!
       return -EINVAL

<- return -EINVAL // live-migration failure!!!
**************************************************************************

HVMOP_track_dirty_vram
-> hap_track_dirty_vram()
/* (!paging_mode_log_dirty(d) && !dirty_vram) */ -> hap_vram_tracking_init()

    /* <--- (A) */
    -> paging_log_dirty_init(d, hap_enable_vram_tracking, ...) ---> (B)

-> paging_log_dirty_enable()
========================================================================
[case-2]
XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY hypercall
-> paging_enable_logdirty()
-> hap_logdirty_init()

    -> paging_log_dirty_disable()
       /* d->arch.hvm_domain.dirty_vram != NULL */
       d->arch.paging.mode &= ~PG_log_dirty; ---> (C)
       /* d->arch.hvm_domain.dirty_vram is freed */
       dereference dirty_vram // access to freed memory !!! <--- (D)

HVMOP_track_dirty_vram
-> hap_track_dirty_vram()
/* (!paging_mode_log_dirty(d) && dirty_vram) */ <---(C)

    rc = -EINVAL
    xfree(dirty_vram); ---> (D)
    dirty_vram = d->arch.hvm_domain.dirty_vram = NULL;
    return rc;

========================================================================

Actually I encountered the xen crash by null pointer exception in xen-3.4. FYI, in xen-4.x, c/s 19738:8dd5c3cae086 mitigated it.

I'm not sure why paging_lock() is used partially in hap_XXX_vram_tracking functions. Thus, this patch introduces a new lock.
It would be better to use paging_lock() instead of the new lock since shadow paging mode (not HAP mode) uses paging_lock to avoid this race condition.

Thanks,
Kouya

diff -r d1d05cb59a76 xen/arch/x86/hvm/hvm.c
@@ -513,6 +513,7 @@ int hvm_domain_initialise(struct domain 
     spin_lock_init(&d->arch.hvm_domain.pbuf_lock);
     spin_lock_init(&d->arch.hvm_domain.irq_lock);
     spin_lock_init(&d->arch.hvm_domain.uc_lock);
+    spin_lock_init(&d->arch.hvm_domain.dirty_vram_lock);
 
     INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list);
     spin_lock_init(&d->arch.hvm_domain.msixtbl_list_lock);

diff -r d1d05cb59a76 xen/arch/x86/mm/hap/hap.c
@@ -122,7 +122,12 @@ int hap_track_dirty_vram(struct domain *
                          XEN_GUEST_HANDLE_64(uint8) dirty_bitmap)
 {
     long rc = 0;
-    struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
+    struct sh_dirty_vram *dirty_vram;
+
+    if ( !spin_trylock(&d->arch.hvm_domain.dirty_vram_lock) )
+        return -ENODATA;
+
+    dirty_vram = d->arch.hvm_domain.dirty_vram;
 
     if ( nr )
     {

@@ -174,6 +179,7 @@ int hap_track_dirty_vram(struct domain *
             rc = 0;
     }
 
+    spin_unlock(&d->arch.hvm_domain.dirty_vram_lock);
     return rc;
 
 param_fail:

@@ -182,6 +188,8 @@ param_fail:
         xfree(dirty_vram);
         dirty_vram = d->arch.hvm_domain.dirty_vram = NULL;
     }
+
+    spin_unlock(&d->arch.hvm_domain.dirty_vram_lock);
     return rc;
 }
 

diff -r d1d05cb59a76 xen/arch/x86/mm/paging.c
@@ -697,14 +697,21 @@ int paging_domctl(struct domain *d, xen_
             break;
         /* Else fall through... */
     case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY:
+        spin_lock(&d->arch.hvm_domain.dirty_vram_lock);
         if ( hap_enabled(d) )
             hap_logdirty_init(d);
-        return paging_log_dirty_enable(d);
+        rc = paging_log_dirty_enable(d);
+        spin_unlock(&d->arch.hvm_domain.dirty_vram_lock);
+        return rc;
 
     case XEN_DOMCTL_SHADOW_OP_OFF:
+        spin_lock(&d->arch.hvm_domain.dirty_vram_lock);
+        rc = 0;
         if ( paging_mode_log_dirty(d) )
-            if ( (rc = paging_log_dirty_disable(d)) != 0 )
-                return rc;
+            rc = paging_log_dirty_disable(d);
+        spin_unlock(&d->arch.hvm_domain.dirty_vram_lock);
+        if ( rc != 0 )
+            return rc;
         break;
 
     case XEN_DOMCTL_SHADOW_OP_CLEAN:

diff -r d1d05cb59a76 xen/include/asm-x86/hvm/domain.h
@@ -75,6 +75,7 @@ struct hvm_domain {
 
     /* VRAM dirty support. */
     struct sh_dirty_vram *dirty_vram;
+    spinlock_t dirty_vram_lock;
 
     /* If one of vcpus of this domain is in no_fill_mode or
      * mtrr/pat between vcpus is not the same, set is_in_uc_mode

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

I think you're right - it would be better to use the paging_lock.

Cc'ing Robert Phillips, who's got a big patch outstanding that touches the locking in this code. I think the right thing to do is make sure his patch fixes the issue and the backport just the locking parts of it to older trees.

Robert, in your patch you do wrap this all in the paging_lock, but then unlock to call various enable and disable routines. Is there a version of this reace condition there, where some other CPU might call LOG_DIRTY_ENABLE while you've temporarily dropped the lock?

Cheers,

Tim.

Tim,

My proposed patch does not modify the problematic locking code so, unfortunately, it preserves the race condition that Kouya Shimura has discovered.

I question whether his proposed patch would be suitable for the multiple frame buffer situation that my proposed patch addresses. It is possible that a guest might be updating its frame buffers when live migration starts, and the same race would result.

I think the domain.arch.paging.log_dirty function pointers are problematic. They are modified and executed without benefit of locking.

I am uncomfortable with adding another lock.

I will look at updating my patch to avoid the race and will (hopefully) avoid adding another lock.

-- rsp

Hi,

Thanks. I think the paging_lock can probably cover everything we need here. These are toolstack operations and should be fairly rare, and HAP can do most of its work without the paging_lock.

Also, in the next version can you please update this section:

+int hap_track_dirty_vram(struct domain *d,

+                         unsigned long begin_pfn,
+                         unsigned long nr,
+                         XEN_GUEST_HANDLE_64(uint8) guest_dirty_bitmap)

+{
+ long rc = 0;
+ dv_dirty_vram_t *dirty_vram;
+

+    paging_lock(d);
+    dirty_vram = d->arch.hvm_domain.dirty_vram;
+    if ( nr )
+    {
+        dv_range_t *range = NULL;
+        int size = ( nr + BITS_PER_LONG - 1 ) & ~( BITS_PER_LONG - 1 );
+        uint8_t dirty_bitmap[size];

not to allocate a guest-specified amount of stack memory. This is one of the things recently found and fixed in the existing code as XSA-27.
http://xenbits.xen.org/hg/staging/xen-4.1-testing.hg/rev/53ef1f35a0f8

Cheers,

Tim.

Hi Tim,

I agree. I am currently working on fix. Should be ready in a short while.

I'll include this improvement too.
-- rsp