[Xen-devel] [PATCH] x86/memshr: properly check grant references

["Jan Beulich" <JBeulich@xxxxxxxx>]

They need to be range checked against the current table limit in any
event.

Reported-by: Huawei PSIRT <psirt@xxxxxxxxxx>

Move the code to where it belongs, eliminating a number of duplicate
definitions. Add locking. Produce proper error codes, and consume them
instead of making one up. Check grant type. Convert parameter types at
once.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
Note that likely there's more work needed subsequently: The grant isn't
being marked in use for the duration of the use of the GFN. But I'll
leave that to someone actually knowing how to properly to test this.

--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -753,75 +753,25 @@ int mem_sharing_debug_gfn(struct domain
     return num_refs;
 }
 
-#define SHGNT_PER_PAGE_V1 (PAGE_SIZE / sizeof(grant_entry_v1_t))
-#define shared_entry_v1(t, e) \
-    ((t)->shared_v1[(e)/SHGNT_PER_PAGE_V1][(e)%SHGNT_PER_PAGE_V1])
-#define SHGNT_PER_PAGE_V2 (PAGE_SIZE / sizeof(grant_entry_v2_t))
-#define shared_entry_v2(t, e) \
-    ((t)->shared_v2[(e)/SHGNT_PER_PAGE_V2][(e)%SHGNT_PER_PAGE_V2])
-#define STGNT_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
-#define status_entry(t, e) \
-    ((t)->status[(e)/STGNT_PER_PAGE][(e)%STGNT_PER_PAGE])
-
-static grant_entry_header_t *
-shared_entry_header(struct grant_table *t, grant_ref_t ref)
-{
-    ASSERT (t->gt_version != 0);
-    if ( t->gt_version == 1 )
-        return (grant_entry_header_t*)&shared_entry_v1(t, ref);
-    else
-        return &shared_entry_v2(t, ref).hdr;
-}
-
-static int mem_sharing_gref_to_gfn(struct domain *d, 
-                                   grant_ref_t ref, 
-                                   unsigned long *gfn)
-{
-    if ( d->grant_table->gt_version < 1 )
-        return -1;
-
-    if ( d->grant_table->gt_version == 1 ) 
-    {
-        grant_entry_v1_t *sha1;
-        sha1 = &shared_entry_v1(d->grant_table, ref);
-        *gfn = sha1->frame;
-    } 
-    else 
-    {
-        grant_entry_v2_t *sha2;
-        sha2 = &shared_entry_v2(d->grant_table, ref);
-        *gfn = sha2->full_page.frame;
-    }
- 
-    return 0;
-}
-
-
 int mem_sharing_debug_gref(struct domain *d, grant_ref_t ref)
 {
-    grant_entry_header_t *shah;
+    int rc;
     uint16_t status;
-    unsigned long gfn;
+    gfn_t gfn;
 
-    if ( d->grant_table->gt_version < 1 )
+    rc = mem_sharing_gref_to_gfn(d->grant_table, ref, &gfn, &status);
+    if ( rc )
     {
-        MEM_SHARING_DEBUG( 
-                "Asked to debug [dom=%d,gref=%d], but not yet inited.\n",
-                d->domain_id, ref);
-        return -EINVAL;
-    }
-    (void)mem_sharing_gref_to_gfn(d, ref, &gfn); 
-    shah = shared_entry_header(d->grant_table, ref);
-    if ( d->grant_table->gt_version == 1 ) 
-        status = shah->flags;
-    else 
-        status = status_entry(d->grant_table, ref);
+        MEM_SHARING_DEBUG("Asked to debug [dom=%d,gref=%u]: error %d.\n",
+                          d->domain_id, ref, rc);
+        return rc;
+    }
     
     MEM_SHARING_DEBUG(
             "==> Grant [dom=%d,ref=%d], status=%x. ", 
             d->domain_id, ref, status);
 
-    return mem_sharing_debug_gfn(d, gfn); 
+    return mem_sharing_debug_gfn(d, gfn_x(gfn));
 }
 
 int mem_sharing_nominate_page(struct domain *d,
@@ -1422,23 +1372,24 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_P
         case XENMEM_sharing_op_nominate_gref:
         {
             grant_ref_t gref = mso.u.nominate.u.grant_ref;
-            unsigned long gfn;
+            gfn_t gfn;
             shr_handle_t handle;
 
             rc = -EINVAL;
             if ( !mem_sharing_enabled(d) )
                 goto out;
-            if ( mem_sharing_gref_to_gfn(d, gref, &gfn) < 0 )
+            rc = mem_sharing_gref_to_gfn(d->grant_table, gref, &gfn, NULL);
+            if ( rc < 0 )
                 goto out;
 
-            rc = mem_sharing_nominate_page(d, gfn, 3, &handle);
+            rc = mem_sharing_nominate_page(d, gfn_x(gfn), 3, &handle);
             mso.u.nominate.handle = handle;
         }
         break;
 
         case XENMEM_sharing_op_share:
         {
-            unsigned long sgfn, cgfn;
+            gfn_t sgfn, cgfn;
             struct domain *cd;
             shr_handle_t sh, ch;
 
@@ -1470,35 +1421,38 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_P
                 grant_ref_t gref = (grant_ref_t) 
                                     (XENMEM_SHARING_OP_FIELD_GET_GREF(
                                         mso.u.share.source_gfn));
-                if ( mem_sharing_gref_to_gfn(d, gref, &sgfn) < 0 )
+                rc = mem_sharing_gref_to_gfn(d->grant_table, gref, &sgfn,
+                                             NULL);
+                if ( rc < 0 )
                 {
                     rcu_unlock_domain(cd);
-                    rc = -EINVAL;
                     goto out;
                 }
-            } else {
-                sgfn  = mso.u.share.source_gfn;
             }
+            else
+                sgfn = _gfn(mso.u.share.source_gfn);
 
             if ( XENMEM_SHARING_OP_FIELD_IS_GREF(mso.u.share.client_gfn) )
             {
                 grant_ref_t gref = (grant_ref_t) 
                                     (XENMEM_SHARING_OP_FIELD_GET_GREF(
                                         mso.u.share.client_gfn));
-                if ( mem_sharing_gref_to_gfn(cd, gref, &cgfn) < 0 )
+                rc = mem_sharing_gref_to_gfn(cd->grant_table, gref, &cgfn,
+                                             NULL);
+                if ( rc < 0 )
                 {
                     rcu_unlock_domain(cd);
-                    rc = -EINVAL;
                     goto out;
                 }
-            } else {
-                cgfn  = mso.u.share.client_gfn;
             }
+            else
+                cgfn = _gfn(mso.u.share.client_gfn);
 
             sh = mso.u.share.source_handle;
             ch = mso.u.share.client_handle;
 
-            rc = mem_sharing_share_pages(d, sgfn, sh, cd, cgfn, ch); 
+            rc = mem_sharing_share_pages(d, gfn_x(sgfn), sh,
+                                         cd, gfn_x(cgfn), ch);
 
             rcu_unlock_domain(cd);
         }
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3438,6 +3438,53 @@ void grant_table_init_vcpu(struct vcpu *
     v->maptrack_tail = MAPTRACK_TAIL;
 }
 
+#ifdef CONFIG_HAS_MEM_SHARING
+int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
+                            gfn_t *gfn, uint16_t *status)
+{
+    int rc = 0;
+    uint16_t flags = 0;
+
+    grant_read_lock(gt);
+
+    if ( gt->gt_version < 1 )
+        rc = -EINVAL;
+    else if ( ref >= nr_grant_entries(gt) )
+        rc = -ENOENT;
+    else if ( gt->gt_version == 1 )
+    {
+        const grant_entry_v1_t *sha1 = &shared_entry_v1(gt, ref);
+
+        flags = sha1->flags;
+        *gfn = _gfn(sha1->frame);
+    }
+    else
+    {
+        const grant_entry_v2_t *sha2 = &shared_entry_v2(gt, ref);
+
+        flags = sha2->hdr.flags;
+        if ( flags & GTF_sub_page )
+           *gfn = _gfn(sha2->sub_page.frame);
+        else
+           *gfn = _gfn(sha2->full_page.frame);
+    }
+
+    if ( (flags & GTF_type_mask) != GTF_permit_access )
+        rc = -ENXIO;
+    else if ( !rc && status )
+    {
+        if ( gt->gt_version == 1 )
+            *status = flags;
+        else
+            *status = status_entry(gt, ref);
+    }
+
+    grant_read_unlock(gt);
+
+    return rc;
+}
+#endif
+
 static void gnttab_usage_print(struct domain *rd)
 {
     int first = 1;
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -149,4 +149,7 @@ static inline unsigned int grant_to_stat
         GRANT_STATUS_PER_PAGE;
 }
 
+int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
+                            gfn_t *gfn, uint16_t *status);
+
 #endif /* __XEN_GRANT_TABLE_H__ */

Attachment: x86-memshr-gref-check.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel