path: root/emulators/xen-kernel/files/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch

From 3ad26b95cd9bacedad5ba503515cf6e618162be1 Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Thu, 15 Jun 2017 16:25:27 +0100
Subject: [PATCH 4/4] 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.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 xen/common/grant_table.c          | 108 ++++++++++++++++++--------------------
 xen/include/asm-arm/grant_table.h |   2 +-
 xen/include/asm-x86/grant_table.h |   5 +-
 3 files changed, 55 insertions(+), 60 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 5e92e2c..025aad0 100644
--- 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_t put_handle = 0;
 
     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 %#x\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 ) 
+        if ( !is_iomem_page(op->frame) )
         {
-            /*
-             * 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;
@@ -3389,7 +3383,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 5e076cc..d76c7c7 100644
--- 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 8c9bbcf..9ca631c 100644
--- 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) )
-- 
2.1.4