gnttab: fix pin count / page reference race

Dropping page references before decrementing pin counts is a bad idea
if assumptions are being made that a non-zero pin count implies a valid
page. Fix the order of operations in gnttab_copy_release_buf(), but at
the same time also remove the assertion that was found to trigger:
map_grant_ref() also has the potential of causing a race here, and
changing the order of operations there would likely be quite a bit more
involved.

This is CVE-2017-15597 / XSA-236.

Reported-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 0558de9ce874845ae25983782456ba104da3a561..c5950f2b3f11a3ac6c88c7f712d8fa305eec6666 100644 (file)
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2525,9 +2525,20 @@ acquire_grant_for_copy(
         td = page_get_owner_and_reference(*page);
         /*
          * act->pin being non-zero should guarantee the page to have a
-         * non-zero refcount and hence a valid owner.
+         * non-zero refcount and hence a valid owner (matching the one on
+         * record), with one exception: If the owning domain is dying we
+         * had better not make implications from pin count (map_grant_ref()
+         * updates pin counts before obtaining page references, for
+         * example).
          */
-        ASSERT(td);
+        if ( td != rd || rd->is_dying )
+        {
+            if ( td )
+                put_page(*page);
+            *page = NULL;
+            rc = GNTST_bad_domain;
+            goto unlock_out_clear;
+        }
     }
 
     act->pin += readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
@@ -2639,6 +2650,11 @@ static void gnttab_copy_release_buf(struct gnttab_copy_buf *buf)
         unmap_domain_page(buf->virt);
         buf->virt = NULL;
     }
+    if ( buf->have_grant )
+    {
+        release_grant_for_copy(buf->domain, buf->ptr.u.ref, buf->read_only);
+        buf->have_grant = 0;
+    }
     if ( buf->have_type )
     {
         put_page_type(buf->page);
@@ -2649,11 +2665,6 @@ static void gnttab_copy_release_buf(struct gnttab_copy_buf *buf)
         put_page(buf->page);
         buf->page = NULL;
     }
-    if ( buf->have_grant )
-    {
-        release_grant_for_copy(buf->domain, buf->ptr.u.ref, buf->read_only);
-        buf->have_grant = 0;
-    }
 }
 
 static int gnttab_copy_claim_buf(const struct gnttab_copy *op,