[xen master] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[patchbot@xxxxxxx]

commit 1b78d4c63687fbcd0baacb13a7bd97605ffe3b88
Author:     Jan Beulich <jbeulich@xxxxxxxx>
AuthorDate: Fri Dec 3 13:54:28 2021 +0100
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Fri Dec 3 13:54:28 2021 +0100

    gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()
    
    Without holding appropriate locks, attempting to remove a prior mapping
    of the underlying page is pointless, as the same (or another) mapping
    could be re-established by a parallel request on another vCPU. Move the
    code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
    xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
    course this new placement doesn't improve things in any way as far as
    the security of grant status frame mappings goes (see XSA-379). Proper
    locking would be needed here to allow status frames to be mapped
    securely.
    
    In turn this then requires replacing the other use in
    gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
    x86's gnttab_set_frame_gfn(). Note that with proper locking inside
    guest_physmap_remove_page() combined with checking the GFN's mapping
    there against the passed in MFN, there then is no issue with the
    involved multiple gnttab_set_frame_gfn()-s potentially returning varying
    values (due to a racing XENMAPSPACE_grant_table request).
    
    This, as a side effect, does away with gnttab_map_frame() having a local
    variable "gfn" which shadows a function parameter of the same name.
    
    Together with XSA-379 this points out that XSA-255's addition to
    gnttab_map_frame() was really useless.
    
    Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
    Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
    Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
---
 xen/common/grant_table.c          | 26 +++++++++++---------------
 xen/include/asm-arm/grant_table.h | 16 +++++++++++-----
 xen/include/asm-x86/grant_table.h |  7 ++++++-
 3 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 012a74455b..0262f2c48a 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -93,7 +93,7 @@ struct grant_table {
     struct radix_tree_root maptrack_tree;
 
     /* Domain to which this struct grant_table belongs. */
-    const struct domain *domain;
+    struct domain *domain;
 
     struct grant_table_arch arch;
 };
@@ -1796,8 +1796,8 @@ gnttab_unpopulate_status_frames(struct domain *d, struct 
grant_table *gt)
         {
             int rc = gfn_eq(gfn, INVALID_GFN)
                      ? 0
-                     : guest_physmap_remove_page(d, gfn,
-                                                 page_to_mfn(pg), 0);
+                     : gnttab_set_frame_gfn(gt, true, i, INVALID_GFN,
+                                            page_to_mfn(pg));
 
             if ( rc )
             {
@@ -1807,7 +1807,6 @@ gnttab_unpopulate_status_frames(struct domain *d, struct 
grant_table *gt)
                 domain_crash(d);
                 return rc;
             }
-            gnttab_set_frame_gfn(gt, true, i, INVALID_GFN);
         }
 
         BUG_ON(page_get_owner(pg) != d);
@@ -4150,6 +4149,9 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, 
gfn_t gfn, mfn_t *mfn)
     struct grant_table *gt = d->grant_table;
     bool status = false;
 
+    if ( gfn_eq(gfn, INVALID_GFN) )
+        return -EINVAL;
+
     grant_write_lock(gt);
 
     if ( evaluate_nospec(gt->gt_version == 2) && (idx & 
XENMAPIDX_grant_table_status) )
@@ -4162,24 +4164,18 @@ int gnttab_map_frame(struct domain *d, unsigned long 
idx, gfn_t gfn, mfn_t *mfn)
     else
         rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
 
-    if ( !rc && paging_mode_translate(d) )
-    {
-        gfn_t gfn = gnttab_get_frame_gfn(gt, status, idx);
-
-        if ( !gfn_eq(gfn, INVALID_GFN) )
-            rc = guest_physmap_remove_page(d, gfn, *mfn, 0);
-    }
-
     if ( !rc )
     {
+        struct page_info *pg = mfn_to_page(*mfn);
+
         /*
          * 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
+        if ( !get_page(pg, d) )
             rc = -EBUSY;
+        else if ( (rc = gnttab_set_frame_gfn(gt, status, idx, gfn, *mfn)) )
+            put_page(pg);
     }
 
     grant_write_unlock(gt);
diff --git a/xen/include/asm-arm/grant_table.h 
b/xen/include/asm-arm/grant_table.h
index 0ce77f9a1c..d31a4d6805 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t 
mfn,
         XFREE((gt)->arch.status_gfn);                                    \
     } while ( 0 )
 
-#define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
-    do {                                                                 \
-        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
-            (gfn);                                                       \
-    } while ( 0 )
+#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
+    ({                                                                   \
+        int rc_ = 0;                                                     \
+        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
+        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
+             (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn,   \
+                                              0)) == 0 )                 \
+            ((st) ? (gt)->arch.status_gfn                                \
+                  : (gt)->arch.shared_gfn)[idx] = (gfn);                 \
+        rc_;                                                             \
+    })
 
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
    (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
diff --git a/xen/include/asm-x86/grant_table.h 
b/xen/include/asm-x86/grant_table.h
index 84e32960c0..a8a21439a4 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -37,7 +37,12 @@ static inline int replace_grant_host_mapping(uint64_t addr, 
mfn_t frame,
 
 #define gnttab_init_arch(gt) 0
 #define gnttab_destroy_arch(gt) do {} while ( 0 )
-#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
+#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
+    (gfn_eq(gfn, INVALID_GFN)                                            \
+     ? guest_physmap_remove_page((gt)->domain,                           \
+                                 gnttab_get_frame_gfn(gt, st, idx),      \
+                                 mfn, 0)                                 \
+     : 0 /* Handled in add_to_physmap_one(). */)
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
     mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
                       : gnttab_shared_mfn(gt, idx);                      \
--
generated by git-patchbot for /home/xen/git/xen.git#master