gnttab: correct locking order reversal

Coverity ID 1087189

Correct a lock order reversal between a domains page allocation and grant
table locks.

This is CVE-2013-4494 / XSA-73.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Consolidate error handling.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Keir Fraser <keir@xen.org>
Tested-by: Matthew Daley <mattjd@gmail.com>
master commit: a321abc6d8122f8cb3928066cc74363c4fdddcfd
master date: 2013-11-04 10:06:36 +0100

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index e0f91ab9bc70ddead13a503d21672e2a1566e978..156ef9a204765643bb049f16c04a69fcd13ace5e 100644 (file)
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1459,6 +1459,8 @@ gnttab_transfer(
 
     for ( i = 0; i < count; i++ )
     {
+        bool_t okay;
+
         if (i && hypercall_preempt_check())
             return i;
 
@@ -1555,16 +1557,18 @@ gnttab_transfer(
          * pages when it is dying.
          */
         if ( unlikely(e->is_dying) ||
-             unlikely(e->tot_pages >= e->max_pages) ||
-             unlikely(!gnttab_prepare_for_transfer(e, d, gop.ref)) )
+             unlikely(e->tot_pages >= e->max_pages) )
         {
-            if ( !e->is_dying )
-                gdprintk(XENLOG_INFO, "gnttab_transfer: "
-                        "Transferee has no reservation "
-                        "headroom (%d,%d) or provided a bad grant ref (%08x) "
-                        "or is dying (%d)\n",
-                        e->tot_pages, e->max_pages, gop.ref, e->is_dying);
             spin_unlock(&e->page_alloc_lock);
+
+            if ( e->is_dying )
+                gdprintk(XENLOG_INFO, "gnttab_transfer: "
+                         "Transferee (d%d) is dying\n", e->domain_id);
+            else
+                gdprintk(XENLOG_INFO, "gnttab_transfer: "
+                         "Transferee (d%d) has no headroom (tot %u, max %u)\n",
+                         e->domain_id, e->tot_pages, e->max_pages);
+
             rcu_unlock_domain(e);
             page->count_info &= ~(PGC_count_mask|PGC_allocated);
             free_domheap_page(page);
@@ -1575,6 +1579,37 @@ gnttab_transfer(
         /* Okay, add the page to 'e'. */
         if ( unlikely(e->tot_pages++ == 0) )
             get_knownalive_domain(e);
+
+        /*
+         * We must drop the lock to avoid a possible deadlock in
+         * gnttab_prepare_for_transfer.  We have reserved a page in e so can
+         * safely drop the lock and re-aquire it later to add page to the
+         * pagelist.
+         */
+        spin_unlock(&e->page_alloc_lock);
+        okay = gnttab_prepare_for_transfer(e, d, gop.ref);
+        spin_lock(&e->page_alloc_lock);
+
+        if ( unlikely(!okay) || unlikely(e->is_dying) )
+        {
+            bool_t drop_dom_ref = !--e->tot_pages;
+
+            spin_unlock(&e->page_alloc_lock);
+
+            if ( okay /* i.e. e->is_dying due to the surrounding if() */ )
+                gdprintk(XENLOG_INFO, "gnttab_transfer: "
+                         "Transferee (d%d) is now dying\n", e->domain_id);
+
+            if ( drop_dom_ref )
+                put_domain(e);
+            rcu_unlock_domain(e);
+
+            page->count_info &= ~(PGC_count_mask|PGC_allocated);
+            free_domheap_page(page);
+            gop.status = GNTST_general_error;
+            goto copyback;
+        }
+
         page_list_add_tail(page, &e->page_list);
         page_set_owner(page, e);