gnttab: deal with status frame mapping race

Once gnttab_map_frame() drops the grant table lock, the MFN it reports
back to its caller is free to other manipulation. In particular
gnttab_unpopulate_status_frames() might free it, by a racing request on
another CPU, thus resulting in a reference to a deallocated page getting
added to a domain's P2M.

Obtain a page reference in gnttab_map_frame() to prevent freeing of the
page until xenmem_add_to_physmap_one() has actually completed its acting
on the page. Do so uniformly, even if only strictly required for v2
status pages, to avoid extra conditionals (which then would all need to
be kept in sync going forward).

This is CVE-2021-28701 / XSA-384.

Reported-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
master commit: eb6bbf7b30da5bae87932514d54d0e3c68b23757
master date: 2021-09-08 14:37:45 +0200

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index de6009e54bd18186a21be32b469a495cf670e2d8..c4e176098813948c36b1941965652baaf32a72b7 100644 (file)
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1227,6 +1227,8 @@ int xenmem_add_to_physmap_one(
         if ( rc )
             return rc;
 
+        /* Need to take care of the reference obtained in gnttab_map_frame(). */
+        page = mfn_to_page(mfn);
         t = p2m_ram_rw;
 
         break;
@@ -1294,9 +1296,12 @@ int xenmem_add_to_physmap_one(
     /* Map at new location. */
     rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
 
-    /* If we fail to add the mapping, we need to drop the reference we
-     * took earlier on foreign pages */
-    if ( rc && space == XENMAPSPACE_gmfn_foreign )
+    /*
+     * For XENMAPSPACE_gmfn_foreign if we failed to add the mapping, we need
+     * to drop the reference we took earlier. In all other cases we need to
+     * drop any reference we took earlier (perhaps indirectly).
+     */
+    if ( space == XENMAPSPACE_gmfn_foreign ? rc : page != NULL )
     {
         ASSERT(page != NULL);
         put_page(page);

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f3767387e6c77dd01ee17e98bd0a5b1b0ee230dc..8c0a9b022644b23ab37ba4909615fe419ccedb7b 100644 (file)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4777,6 +4777,8 @@ int xenmem_add_to_physmap_one(
             rc = gnttab_map_frame(d, idx, gpfn, &mfn);
             if ( rc )
                 return rc;
+            /* Need to take care of the ref obtained in gnttab_map_frame(). */
+            page = mfn_to_page(mfn);
             break;
         case XENMAPSPACE_gmfn:
         {

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 3056454b951f19ff3a60aab0176d64276315a855..2bc00f30a3882310c23338817008804d177ad8ba 100644 (file)
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3986,7 +3986,16 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
     }
 
     if ( !rc )
-        gnttab_set_frame_gfn(gt, status, idx, gfn);
+    {
+        /*
+         * Make sure gnttab_unpopulate_status_frames() won't (successfully)
+         * free the page until our caller has completed its operation.
+         */
+        if ( get_page(mfn_to_page(*mfn), d) )
+            gnttab_set_frame_gfn(gt, status, idx, gfn);
+        else
+            rc = -EBUSY;
+    }
 
     grant_write_unlock(gt);