gnttab: __gnttab_unmap_common_complete() is all-or-nothing

All failures have to be detected in __gnttab_unmap_common(), the
completion function must not skip part of its processing. In particular
the GNTMAP_device_map related putting of page references and adjustment
of pin count must not occur if __gnttab_unmap_common() signaled an
error. Furthermore the function must not make adjustments to global
state (here: clearing GNTTAB_device_map) before all possibly failing
operations have been performed.

There's one exception for IOMMU related failures: As IOMMU manipulation
occurs after GNTMAP_*_map have been cleared already, the related page
reference and pin count adjustments need to be done nevertheless. A
fundamental requirement for the correctness of this is that
iommu_{,un}map_page() crash any affected DomU in case of failure.

The version check appears to be pointless (or could perhaps be a
BUG_ON() or ASSERT()), but for the moment also move it.

This is part of XSA-224.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
master commit: 11fc7ccb7217ccfb79edb727d1349618bcb0602d
master date: 2017-06-20 14:46:47 +0200

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 60e60d4241ebcd8efe02ec5eaa4211fc4e1b3472..f9654f13f556cb44109a0a87c2d79f016558a12e 100644 (file)
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -96,7 +96,7 @@ struct gnttab_unmap_common {
     int16_t status;
 
     /* Shared state beteen *_unmap and *_unmap_complete */
-    u16 flags;
+    u16 done;
     unsigned long frame;
     struct domain *rd;
     grant_ref_t ref;
@@ -948,7 +948,8 @@ __gnttab_map_grant_ref(
                 refcnt++;
             }
 
-            if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+            if ( gnttab_host_mapping_get_page_type(op->flags & GNTMAP_readonly,
+                                                   ld, rd) )
             {
                 if ( (owner == dom_cow) ||
                      !get_page_type(pg, PGT_writable_page) )
@@ -1095,6 +1096,7 @@ __gnttab_unmap_common(
     struct active_grant_entry *act;
     s16              rc = 0;
     struct grant_mapping *map;
+    unsigned int flags;
     bool put_handle = false;
 
     ld = current->domain;
@@ -1145,6 +1147,20 @@ __gnttab_unmap_common(
 
     grant_read_lock(rgt);
 
+    if ( rgt->gt_version == 0 )
+    {
+        /*
+         * This ought to be impossible, as such a mapping should not have
+         * been established (see the nr_grant_entries(rgt) bounds check in
+         * __gnttab_map_grant_ref()). Doing this check only in
+         * __gnttab_unmap_common_complete() - as it used to be done - would,
+         * however, be too late.
+         */
+        rc = GNTST_bad_gntref;
+        flags = 0;
+        goto unlock_out;
+    }
+
     op->rd = rd;
     op->ref = map->ref;
 
@@ -1160,6 +1176,7 @@ __gnttab_unmap_common(
     {
         gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle);
         rc = GNTST_bad_handle;
+        flags = 0;
         goto unlock_out;
     }
 
@@ -1173,9 +1190,9 @@ __gnttab_unmap_common(
      * hold anyway; see docs/misc/grant-tables.txt's "Locking" section.
      */
 
-    op->flags = read_atomic(&map->flags);
+    flags = read_atomic(&map->flags);
     smp_rmb();
-    if ( unlikely(!op->flags) || unlikely(map->domid != dom) ||
+    if ( unlikely(!flags) || unlikely(map->domid != dom) ||
          unlikely(map->ref != op->ref) )
     {
         gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle);
@@ -1185,24 +1202,27 @@ __gnttab_unmap_common(
 
     op->frame = act->frame;
 
-    if ( op->dev_bus_addr )
-    {
-        if ( unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
-            PIN_FAIL(act_release_out, GNTST_general_error,
-                     "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
-                     op->dev_bus_addr, pfn_to_paddr(act->frame));
-
-        map->flags &= ~GNTMAP_device_map;
-    }
+    if ( op->dev_bus_addr &&
+         unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
+        PIN_FAIL(act_release_out, GNTST_general_error,
+                 "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
+                 op->dev_bus_addr, pfn_to_paddr(act->frame));
 
-    if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) )
+    if ( op->host_addr && (flags & GNTMAP_host_map) )
     {
         if ( (rc = replace_grant_host_mapping(op->host_addr,
                                               op->frame, op->new_addr, 
-                                              op->flags)) < 0 )
+                                              flags)) < 0 )
             goto act_release_out;
 
         map->flags &= ~GNTMAP_host_map;
+        op->done |= GNTMAP_host_map | (flags & GNTMAP_readonly);
+    }
+
+    if ( op->dev_bus_addr && (flags & GNTMAP_device_map) )
+    {
+        map->flags &= ~GNTMAP_device_map;
+        op->done |= GNTMAP_device_map | (flags & GNTMAP_readonly);
     }
 
     if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) )
@@ -1239,7 +1259,7 @@ __gnttab_unmap_common(
     }
 
     /* If just unmapped a writable mapping, mark as dirtied */
-    if ( rc == GNTST_okay && !(op->flags & GNTMAP_readonly) )
+    if ( rc == GNTST_okay && !(flags & GNTMAP_readonly) )
          gnttab_mark_dirty(rd, op->frame);
 
     op->status = rc;
@@ -1256,13 +1276,9 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
     struct page_info *pg;
     uint16_t *status;
 
-    if ( rd == NULL )
+    if ( !op->done )
     { 
-        /*
-         * Suggests that __gntab_unmap_common failed in
-         * rcu_lock_domain_by_id() or earlier, and so we have nothing
-         * to complete
-         */
+        /* __gntab_unmap_common() didn't do anything - nothing to complete. */
         return;
     }
 
@@ -1272,8 +1288,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
     rgt = rd->grant_table;
 
     grant_read_lock(rgt);
-    if ( rgt->gt_version == 0 )
-        goto unlock_out;
 
     act = active_entry_acquire(rgt, op->ref);
     sha = shared_entry_header(rgt, op->ref);
@@ -1283,72 +1297,50 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
     else
         status = &status_entry(rgt, op->ref);
 
-    if ( op->dev_bus_addr &&
-         unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
-    {
-        /*
-         * Suggests that __gntab_unmap_common failed early and so
-         * nothing further to do
-         */
-        goto act_release_out;
-    }
-
     pg = mfn_to_page(op->frame);
 
-    if ( op->dev_bus_addr && (op->flags & GNTMAP_device_map) )
+    if ( op->done & GNTMAP_device_map )
     {
         if ( !is_iomem_page(act->frame) )
         {
-            if ( op->flags & GNTMAP_readonly )
+            if ( op->done & GNTMAP_readonly )
                 put_page(pg);
             else
                 put_page_and_type(pg);
         }
 
         ASSERT(act->pin & (GNTPIN_devw_mask | GNTPIN_devr_mask));
-        if ( op->flags & GNTMAP_readonly )
+        if ( op->done & GNTMAP_readonly )
             act->pin -= GNTPIN_devr_inc;
         else
             act->pin -= GNTPIN_devw_inc;
     }
 
-    if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) )
+    if ( op->done & GNTMAP_host_map )
     {
-        if ( op->status != 0 ) 
-        {
-            /*
-             * Suggests that __gntab_unmap_common failed in
-             * replace_grant_host_mapping() or IOMMU handling, so nothing
-             * further to do (short of re-establishing the mapping in the
-             * latter case).
-             */
-            goto act_release_out;
-        }
-
         if ( !is_iomem_page(op->frame) ) 
         {
-            if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+            if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly,
+                                                   ld, rd) )
                 put_page_type(pg);
             put_page(pg);
         }
 
         ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
-        if ( op->flags & GNTMAP_readonly )
+        if ( op->done & GNTMAP_readonly )
             act->pin -= GNTPIN_hstr_inc;
         else
             act->pin -= GNTPIN_hstw_inc;
     }
 
     if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) &&
-         !(op->flags & GNTMAP_readonly) )
+         !(op->done & GNTMAP_readonly) )
         gnttab_clear_flag(_GTF_writing, status);
 
     if ( act->pin == 0 )
         gnttab_clear_flag(_GTF_reading, status);
 
- act_release_out:
     active_entry_release(act);
- unlock_out:
     grant_read_unlock(rgt);
 
     rcu_unlock_domain(rd);
@@ -1364,6 +1356,7 @@ __gnttab_unmap_grant_ref(
     common->handle = op->handle;
 
     /* Intialise these in case common contains old state */
+    common->done = 0;
     common->new_addr = 0;
     common->rd = NULL;
     common->frame = 0;
@@ -1429,6 +1422,7 @@ __gnttab_unmap_and_replace(
     common->handle = op->handle;
     
     /* Intialise these in case common contains old state */
+    common->done = 0;
     common->dev_bus_addr = 0;
     common->rd = NULL;
     common->frame = 0;
@@ -3388,7 +3382,9 @@ gnttab_release_mappings(
                 if ( gnttab_release_host_mappings(d) &&
                      !is_iomem_page(act->frame) )
                 {
-                    if ( gnttab_host_mapping_get_page_type(map, d, rd) )
+                    if ( gnttab_host_mapping_get_page_type((map->flags &
+                                                            GNTMAP_readonly),
+                                                           d, rd) )
                         put_page_type(pg);
                     put_page(pg);
                 }

diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index eb024230ba70f338818872945f947a66f68141df..bc4d61a940e366de91c7052eb1110428deb4f9dd 100644 (file)
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -9,7 +9,7 @@ void gnttab_clear_flag(unsigned long nr, uint16_t *addr);
 int create_grant_host_mapping(unsigned long gpaddr,
         unsigned long mfn, unsigned int flags, unsigned int
         cache_flags);
-#define gnttab_host_mapping_get_page_type(op, d, rd) (0)
+#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
 int replace_grant_host_mapping(unsigned long gpaddr, unsigned long mfn,
         unsigned long new_gpaddr, unsigned int flags);
 void gnttab_mark_dirty(struct domain *d, unsigned long l);

diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 8c9bbcf753cb813784cb1c85377883f705cb0828..9ca631c4e3414e3f74cd7149ed4659b1dd3eb234 100644 (file)
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -58,9 +58,8 @@ static inline void gnttab_clear_flag(unsigned int nr, uint16_t *st)
 }
 
 /* Foreign mappings of HHVM-guest pages do not modify the type count. */
-#define gnttab_host_mapping_get_page_type(op, ld, rd)   \
-    (!((op)->flags & GNTMAP_readonly) &&                \
-     (((ld) == (rd)) || !paging_mode_external(rd)))
+#define gnttab_host_mapping_get_page_type(ro, ld, rd)   \
+    (!(ro) && (((ld) == (rd)) || !paging_mode_external(rd)))
 
 /* Done implicitly when page tables are destroyed. */
 #define gnttab_release_host_mappings(domain) ( paging_mode_external(domain) )