gnttab: don't blindly free status pages upon version change

There may still be active mappings, which would trigger the respective
BUG_ON(). Split the loop into one dealing with the page attributes and
the second (when the first fully passed) freeing the pages. Return an
error if any pages still have pending references.

This is part of XSA-255.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
master commit: 38bfcc165dda5f4284d7c218b91df9e144ddd88d
master date: 2018-02-27 14:07:12 +0100

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 591cfd870e6c417d936f2c7345d47a9329427a3c..4c1a407c958753a4b19feb85f2b4174d2bf9db09 100644 (file)
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1182,12 +1182,22 @@ int xenmem_add_to_physmap_one(
                 mfn = mfn_x(INVALID_MFN);
         }
 
+        if ( mfn != mfn_x(INVALID_MFN) &&
+             !gfn_eq(gnttab_get_frame_gfn(d, status, idx), INVALID_GFN) )
+        {
+            rc = guest_physmap_remove_page(d,
+                                           gnttab_get_frame_gfn(d, status, idx),
+                                           _mfn(mfn), 0);
+            if ( rc )
+            {
+                grant_write_unlock(d->grant_table);
+                return rc;
+            }
+        }
+
         if ( mfn != mfn_x(INVALID_MFN) )
         {
-            if ( status )
-                d->arch.grant_status_gfn[idx] = gfn;
-            else
-                d->arch.grant_shared_gfn[idx] = gfn;
+            gnttab_set_frame_gfn(d, status, idx, gfn);
 
             t = p2m_ram_rw;
         }

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 32d093f6e96bf1ab1536410f9f756571d9617586..58cad3a6d4d8bbd3db70e67e8ba125cac0573fc7 100644 (file)
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1516,23 +1516,74 @@ status_alloc_failed:
     return -ENOMEM;
 }
 
-static void
+static int
 gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
 {
-    int i;
+    unsigned int i;
 
     for ( i = 0; i < nr_status_frames(gt); i++ )
     {
         struct page_info *pg = virt_to_page(gt->status[i]);
+        gfn_t gfn = gnttab_get_frame_gfn(d, true, i);
+
+        /*
+         * For translated domains, recovering from failure after partial
+         * changes were made is more complicated than it seems worth
+         * implementing at this time. Hence respective error paths below
+         * crash the domain in such a case.
+         */
+        if ( paging_mode_translate(d) )
+        {
+            int rc = gfn_eq(gfn, INVALID_GFN)
+                     ? 0
+                     : guest_physmap_remove_page(d, gfn,
+                                                 _mfn(page_to_mfn(pg)), 0);
+
+            if ( rc )
+            {
+                gprintk(XENLOG_ERR,
+                        "Could not remove status frame %u (GFN %#lx) from P2M\n",
+                        i, gfn_x(gfn));
+                domain_crash(d);
+                return rc;
+            }
+            gnttab_set_frame_gfn(d, true, i, INVALID_GFN);
+        }
 
         BUG_ON(page_get_owner(pg) != d);
         if ( test_and_clear_bit(_PGC_allocated, &pg->count_info) )
             put_page(pg);
-        BUG_ON(pg->count_info & ~PGC_xen_heap);
+
+        if ( pg->count_info & ~PGC_xen_heap )
+        {
+            if ( paging_mode_translate(d) )
+            {
+                gprintk(XENLOG_ERR,
+                        "Wrong page state %#lx of status frame %u (GFN %#lx)\n",
+                        pg->count_info, i, gfn_x(gfn));
+                domain_crash(d);
+            }
+            else
+            {
+                if ( get_page(pg, d) )
+                    set_bit(_PGC_allocated, &pg->count_info);
+                while ( i-- )
+                    gnttab_create_status_page(d, gt, i);
+            }
+            return -EBUSY;
+        }
+
+        page_set_owner(pg, NULL);
+    }
+
+    for ( i = 0; i < nr_status_frames(gt); i++ )
+    {
         free_xenheap_page(gt->status[i]);
         gt->status[i] = NULL;
     }
     gt->nr_status_frames = 0;
+
+    return 0;
 }
 
 /*
@@ -2774,8 +2825,9 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
         break;
     }
 
-    if ( op.version < 2 && gt->gt_version == 2 )
-        gnttab_unpopulate_status_frames(currd, gt);
+    if ( op.version < 2 && gt->gt_version == 2 &&
+         (res = gnttab_unpopulate_status_frames(currd, gt)) != 0 )
+        goto out_unlock;
 
     /* Make sure there's no crud left over from the old version. */
     for ( i = 0; i < nr_grant_frames(gt); i++ )

diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 4d576c59054ff93f9e4cb361657d502937211a47..2f9224fb7aac7c8896c9f49d0c50cceb1727bb05 100644 (file)
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -20,6 +20,17 @@ static inline int replace_grant_supported(void)
     return 1;
 }
 
+#define gnttab_set_frame_gfn(d, st, idx, gfn)                            \
+    do {                                                                 \
+        ((st) ? (d)->arch.grant_status_gfn                               \
+              : (d)->arch.grant_shared_gfn)[idx] = (gfn);                \
+    } while ( 0 )
+
+#define gnttab_get_frame_gfn(d, st, idx) ({                              \
+   _gfn((st) ? gnttab_status_gmfn(d, (d)->grant_table, idx)              \
+             : gnttab_shared_gmfn(d, (d)->grant_table, idx));            \
+})
+
 #define gnttab_create_shared_page(d, t, i)                               \
     do {                                                                 \
         share_xen_page_with_guest(                                       \

diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 9ca631c4e3414e3f74cd7149ed4659b1dd3eb234..9c64361ee2602df06bd28670217e3f1b582335b0 100644 (file)
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -18,6 +18,14 @@ int create_grant_host_mapping(uint64_t addr, unsigned long frame,
 int replace_grant_host_mapping(
     uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int flags);
 
+#define gnttab_set_frame_gfn(d, st, idx, gfn) do {} while ( 0 )
+#define gnttab_get_frame_gfn(d, st, idx) ({                              \
+    unsigned long mfn_ = (st) ? gnttab_status_mfn((d)->grant_table, idx) \
+                              : gnttab_shared_mfn((d)->grant_table, idx); \
+    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_);                       \
+    VALID_M2P(gpfn_) ? _gfn(gpfn_) : INVALID_GFN;                        \
+})
+
 #define gnttab_create_shared_page(d, t, i)                               \
     do {                                                                 \
         share_xen_page_with_guest(                                       \
@@ -33,11 +41,11 @@ int replace_grant_host_mapping(
     } while ( 0 )
 
 
-#define gnttab_shared_mfn(d, t, i)                      \
+#define gnttab_shared_mfn(t, i)                         \
     ((virt_to_maddr((t)->shared_raw[i]) >> PAGE_SHIFT))
 
 #define gnttab_shared_gmfn(d, t, i)                     \
-    (mfn_to_gmfn(d, gnttab_shared_mfn(d, t, i)))
+    (mfn_to_gmfn(d, gnttab_shared_mfn(t, i)))
 
 
 #define gnttab_status_mfn(t, i)                         \