guest_physmap_remove_page() needs its return value checked

Callers, namely such subsequently freeing the page, must not blindly
assume success - the function may namely fail when needing to shatter a
super page, but there not being memory available for the then needed
intermediate page table.

As it happens, guest_remove_page() callers now also all check the
return value.

Furthermore a missed put_gfn() on an error path in gnttab_transfer() is
also being taken care of.

This is part of XSA-222.

Reported-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
master commit: a0cce6048d010a30ac82f8db7787bbf9aada64f4
master date: 2017-06-20 14:41:16 +0200

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 596283fc99ddf8504cdfc8ecf614c524645ba2ae..74acaf30d2d0ee2c1f217932a050c0ff9ddade5e 100644 (file)
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1340,13 +1340,14 @@ int replace_grant_host_mapping(unsigned long addr, unsigned long mfn,
 {
     gfn_t gfn = _gfn(addr >> PAGE_SHIFT);
     struct domain *d = current->domain;
+    int rc;
 
     if ( new_addr != 0 || (flags & GNTMAP_contains_pte) )
         return GNTST_general_error;
 
-    guest_physmap_remove_page(d, gfn, _mfn(mfn), 0);
+    rc = guest_physmap_remove_page(d, gfn, _mfn(mfn), 0);
 
-    return GNTST_okay;
+    return rc ? GNTST_general_error : GNTST_okay;
 }
 
 int is_iomem_page(unsigned long mfn)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f85424b1adaf77dfaf4c499066695da54afe524b..c0b3de00342c69c2f8745210f562741bca9fb56c 100644 (file)
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1217,11 +1217,10 @@ int guest_physmap_add_entry(struct domain *d,
     return p2m_insert_mapping(d, gfn, (1 << page_order), mfn, t);
 }
 
-void guest_physmap_remove_page(struct domain *d,
-                               gfn_t gfn,
-                               mfn_t mfn, unsigned int page_order)
+int guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
+                              unsigned int page_order)
 {
-    p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
+    return p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
 }
 
 static int p2m_alloc_table(struct domain *d)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7b4e5e5f9374dd5e825ac76f1f8d0c06deb240b6..a725b43a672d96dcf093c35d12ac5eb87577f00d 100644 (file)
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -808,7 +808,15 @@ int arch_domain_soft_reset(struct domain *d)
         ret = -ENOMEM;
         goto exit_put_gfn;
     }
-    guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), PAGE_ORDER_4K);
+
+    ret = guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), PAGE_ORDER_4K);
+    if ( ret )
+    {
+        printk(XENLOG_G_ERR "Failed to remove Dom%d's shared_info frame %lx\n",
+               d->domain_id, gfn);
+        free_domheap_page(new_page);
+        goto exit_put_gfn;
+    }
 
     ret = guest_physmap_add_page(d, _gfn(gfn), _mfn(page_to_mfn(new_page)),
                                  PAGE_ORDER_4K);

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 0a02d6588d2527eb8415d02d1e1b73fc8c5078bf..a34f7f5d73889c4fc04603d95201970c051e6fce 100644 (file)
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -427,7 +427,9 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
         if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
         {
             omfn = get_gfn_query_unlocked(d, gfn + i, &t);
-            guest_physmap_remove_page(d, _gfn(gfn + i), omfn, PAGE_ORDER_4K);
+            if ( guest_physmap_remove_page(d, _gfn(gfn + i), omfn,
+                                           PAGE_ORDER_4K) )
+                /* nothing, best effort only */;
             continue;
         }
 

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 88071ab952be0dcd8237124cb7b244eadb878a5b..ea1536d55c75124ca24075b4228c5ac760f8d180 100644 (file)
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -267,8 +267,9 @@ bool_t is_ioreq_server_page(struct domain *d, const struct page_info *page)
 static void hvm_remove_ioreq_gmfn(
     struct domain *d, struct hvm_ioreq_page *iorp)
 {
-    guest_physmap_remove_page(d, _gfn(iorp->gmfn),
-                              _mfn(page_to_mfn(iorp->page)), 0);
+    if ( guest_physmap_remove_page(d, _gfn(iorp->gmfn),
+                                   _mfn(page_to_mfn(iorp->page)), 0) )
+        domain_crash(d);
     clear_page(iorp->va);
 }
 

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c03479b7d3146a1eb4098634db46ae5c72258b3b..afc65d4c8b9bf378a045fe75d9e3ec0513887ffe 100644 (file)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4276,7 +4276,11 @@ static int replace_grant_p2m_mapping(
                 type, mfn_x(old_mfn), frame);
         return GNTST_general_error;
     }
-    guest_physmap_remove_page(d, _gfn(gfn), _mfn(frame), PAGE_ORDER_4K);
+    if ( guest_physmap_remove_page(d, _gfn(gfn), _mfn(frame), PAGE_ORDER_4K) )
+    {
+        put_gfn(d, gfn);
+        return GNTST_general_error;
+    }
 
     put_gfn(d, gfn);
     return GNTST_okay;
@@ -4801,7 +4805,7 @@ int xenmem_add_to_physmap_one(
     struct page_info *page = NULL;
     unsigned long gfn = 0; /* gcc ... */
     unsigned long prev_mfn, mfn = 0, old_gpfn;
-    int rc;
+    int rc = 0;
     p2m_type_t p2mt;
 
     switch ( space )
@@ -4875,25 +4879,30 @@ int xenmem_add_to_physmap_one(
     {
         if ( is_xen_heap_mfn(prev_mfn) )
             /* Xen heap frames are simply unhooked from this phys slot. */
-            guest_physmap_remove_page(d, gpfn, _mfn(prev_mfn), PAGE_ORDER_4K);
+            rc = guest_physmap_remove_page(d, gpfn, _mfn(prev_mfn), PAGE_ORDER_4K);
         else
             /* Normal domain memory is freed, to avoid leaking memory. */
-            guest_remove_page(d, gfn_x(gpfn));
+            rc = guest_remove_page(d, gfn_x(gpfn));
     }
     /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */
     put_gfn(d, gfn_x(gpfn));
 
+    if ( rc )
+        goto put_both;
+
     /* Unmap from old location, if any. */
     old_gpfn = get_gpfn_from_mfn(mfn);
     ASSERT( old_gpfn != SHARED_M2P_ENTRY );
     if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
         ASSERT( old_gpfn == gfn );
     if ( old_gpfn != INVALID_M2P_ENTRY )
-        guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn), PAGE_ORDER_4K);
+        rc = guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn), PAGE_ORDER_4K);
 
     /* Map at new location. */
-    rc = guest_physmap_add_page(d, gpfn, _mfn(mfn), PAGE_ORDER_4K);
+    if ( !rc )
+        rc = guest_physmap_add_page(d, gpfn, _mfn(mfn), PAGE_ORDER_4K);
 
+ put_both:
     /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */
     if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
         put_gfn(d, gfn);

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 2e456bc75ff9b14d9d2ef9eb2a587ae88d454ecd..a86dd8156f77c177c6ae8e48a5b717f97912432c 100644 (file)
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2928,10 +2928,12 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
     {
         if ( is_xen_heap_mfn(mfn_x(prev_mfn)) )
             /* Xen heap frames are simply unhooked from this phys slot */
-            guest_physmap_remove_page(tdom, _gfn(gpfn), prev_mfn, 0);
+            rc = guest_physmap_remove_page(tdom, _gfn(gpfn), prev_mfn, 0);
         else
             /* Normal domain memory is freed, to avoid leaking memory. */
-            guest_remove_page(tdom, gpfn);
+            rc = guest_remove_page(tdom, gpfn);
+        if ( rc )
+            goto put_both;
     }
     /*
      * Create the new mapping. Can't use guest_physmap_add_page() because it
@@ -2944,6 +2946,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
                  "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
                  gpfn, mfn_x(mfn), fgfn, tdom->domain_id, fdom->domain_id);
 
+ put_both:
     put_page(page);
 
     /*

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index b5f74c2c9a1e3f5c8911a2b654d117bb645d7cd3..9fb879f9d35dba871dd49f96f8e13198f8f3b69c 100644 (file)
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1804,6 +1804,7 @@ gnttab_transfer(
     for ( i = 0; i < count; i++ )
     {
         bool_t okay;
+        int rc;
 
         if (i && hypercall_preempt_check())
             return i;
@@ -1854,27 +1855,33 @@ gnttab_transfer(
             goto copyback;
         }
 
-        guest_physmap_remove_page(d, _gfn(gop.mfn), _mfn(mfn), 0);
+        rc = guest_physmap_remove_page(d, _gfn(gop.mfn), _mfn(mfn), 0);
         gnttab_flush_tlb(d);
+        if ( rc )
+        {
+            gdprintk(XENLOG_INFO,
+                     "gnttab_transfer: can't remove GFN %"PRI_xen_pfn" (MFN %lx)\n",
+                     gop.mfn, mfn);
+            gop.status = GNTST_general_error;
+            goto put_gfn_and_copyback;
+        }
 
         /* Find the target domain. */
         if ( unlikely((e = rcu_lock_domain_by_id(gop.domid)) == NULL) )
         {
-            put_gfn(d, gop.mfn);
             gdprintk(XENLOG_INFO, "gnttab_transfer: can't find domain %d\n",
                     gop.domid);
-            page->count_info &= ~(PGC_count_mask|PGC_allocated);
-            free_domheap_page(page);
             gop.status = GNTST_bad_domain;
-            goto copyback;
+            goto put_gfn_and_copyback;
         }
 
         if ( xsm_grant_transfer(XSM_HOOK, d, e) )
         {
-            put_gfn(d, gop.mfn);
             gop.status = GNTST_permission_denied;
         unlock_and_copyback:
             rcu_unlock_domain(e);
+        put_gfn_and_copyback:
+            put_gfn(d, gop.mfn);
             page->count_info &= ~(PGC_count_mask|PGC_allocated);
             free_domheap_page(page);
             goto copyback;
@@ -1923,12 +1930,8 @@ 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);
-            put_gfn(d, gop.mfn);
-            page->count_info &= ~(PGC_count_mask|PGC_allocated);
-            free_domheap_page(page);
             gop.status = GNTST_general_error;
-            goto copyback;
+            goto unlock_and_copyback;
         }
 
         /* Okay, add the page to 'e'. */
@@ -1957,13 +1960,8 @@ gnttab_transfer(
 
             if ( drop_dom_ref )
                 put_domain(e);
-            rcu_unlock_domain(e);
-
-            put_gfn(d, gop.mfn);
-            page->count_info &= ~(PGC_count_mask|PGC_allocated);
-            free_domheap_page(page);
             gop.status = GNTST_general_error;
-            goto copyback;
+            goto unlock_and_copyback;
         }
 
         page_list_add_tail(page, &e->page_list);

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 1429620be841c72b4f9634f3e0ec5d854a720a69..8b2c7a9596f2ec5e065e1bbfb5c353118fdf98f8 100644 (file)
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -272,8 +272,12 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
     mfn = get_gfn_query(d, gmfn, &p2mt);
     if ( unlikely(p2m_is_paging(p2mt)) )
     {
-        guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
+        rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
         put_gfn(d, gmfn);
+
+        if ( rc )
+            return rc;
+
         /* If the page hasn't yet been paged out, there is an
          * actual page that needs to be released. */
         if ( p2mt == p2m_ram_paging_out )
@@ -337,7 +341,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
         return -ENXIO;
     }
 
-    if ( test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
+    rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
+
+    if ( !rc && test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
         put_page_and_type(page);
 
     /*
@@ -348,16 +354,14 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
      * For this purpose (and to match populate_physmap() behavior), the page
      * is kept allocated.
      */
-    if ( !is_domain_direct_mapped(d) &&
+    if ( !rc && !is_domain_direct_mapped(d) &&
          test_and_clear_bit(_PGC_allocated, &page->count_info) )
         put_page(page);
 
-    guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
-
     put_page(page);
     put_gfn(d, gmfn);
 
-    return 0;
+    return rc;
 }
 
 static void decrease_reservation(struct memop_args *a)
@@ -592,7 +596,8 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
             gfn = mfn_to_gmfn(d, mfn);
             /* Pages were unshared above */
             BUG_ON(SHARED_M2P(gfn));
-            guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), 0);
+            if ( guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), 0) )
+                domain_crash(d);
             put_page(page);
         }
 
@@ -1151,8 +1156,8 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
         if ( page )
         {
-            guest_physmap_remove_page(d, _gfn(xrfp.gpfn),
-                                      _mfn(page_to_mfn(page)), 0);
+            rc = guest_physmap_remove_page(d, _gfn(xrfp.gpfn),
+                                           _mfn(page_to_mfn(page)), 0);
             put_page(page);
         }
         else

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index cf8b8b81672490eab934ae7dc71fc163fd647a40..48e4f1b552c15d7d5277d5d71093ec5c42a88f1b 100644 (file)
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2786,9 +2786,7 @@ static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
        if ( !is_domain_direct_mapped(d) )
                return -EINVAL;
 
-       guest_physmap_remove_page(d, _gfn(gfn), _mfn(gfn), 0);
-
-       return 0;
+       return guest_physmap_remove_page(d, _gfn(gfn), _mfn(gfn), 0);
 }
 
 static const struct iommu_ops arm_smmu_iommu_ops = {

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 9e71776e53a0c58e878402eab60a14ca500b0a17..355c2bb513d112013f4d57bc316b0c48e4aa555f 100644 (file)
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -268,10 +268,6 @@ static inline int guest_physmap_add_page(struct domain *d,
     return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
 }
 
-void guest_physmap_remove_page(struct domain *d,
-                               gfn_t gfn,
-                               mfn_t mfn, unsigned int page_order);
-
 mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
 
 /*

diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 7035860903fb3607d4d3b25061935cb20f53577a..8dc1320408a353159ea42e37095a1c0940f1ab3c 100644 (file)
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -561,10 +561,6 @@ static inline int guest_physmap_add_page(struct domain *d,
     return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
 }
 
-/* Remove a page from a domain's p2m table */
-int guest_physmap_remove_page(struct domain *d,
-                              gfn_t gfn, mfn_t mfn, unsigned int page_order);
-
 /* Set a p2m range as populate-on-demand */
 int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
                                           unsigned int order);

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 8b5ffd070dfe59e04b13b18219cf81cb37f38c95..07b24012bac932f43531e6fcb1167dbed98a07ec 100644 (file)
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -554,7 +554,7 @@ int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
                               unsigned long idx, gfn_t gfn);
 
 /* Returns 0 on success, or negative on error. */
-int guest_remove_page(struct domain *d, unsigned long gmfn);
+int __must_check guest_remove_page(struct domain *d, unsigned long gmfn);
 
 #define RAM_TYPE_CONVENTIONAL 0x00000001
 #define RAM_TYPE_RESERVED     0x00000002

diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 3be1e914dbbbcb747d27b327974e698479fc618e..0f64a6e11650e8e1c5df89dacc5c945c20579aa8 100644 (file)
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -1,6 +1,7 @@
 #ifndef _XEN_P2M_COMMON_H
 #define _XEN_P2M_COMMON_H
 
+#include <xen/mm.h>
 #include <public/vm_event.h>
 
 /*
@@ -33,6 +34,11 @@ typedef enum {
     /* NOTE: Assumed to be only 4 bits right now on x86. */
 } p2m_access_t;
 
+/* Remove a page from a domain's p2m table */
+int __must_check
+guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
+                          unsigned int page_order);
+
 /* Map MMIO regions in the p2m: start_gfn and nr describe the range in
  *  * the guest physical address space to map, starting from the machine
  *   * frame number mfn. */