[Xen-changelog] [xen master] common/gnttab: Explicitly default to gnttab v1 during domain creation

[patchbot@xxxxxxx]

commit 1237659523813696af4083279a7b871d407cbd48
Author:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
AuthorDate: Wed Aug 8 15:54:30 2018 +0100
Commit:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Fri Aug 10 13:27:24 2018 +0100

    common/gnttab: Explicitly default to gnttab v1 during domain creation
    
    For reasons which appear to be exclusively down to poor review of the grant
    table v2 code, a grant table's version field was wasn't initialised during
    creation.
    
    A number of problems (including XSAs) have occurred in the past trying 
trying
    to use a grant table which hasn't been properly set up, and various areas of
    the code cope with v0 by defaulting to v1.
    
    In particular, the toolstack using GNTTABOP_setup_table to be able to fill 
in
    the store/console grants has a side effect of switching to v1.
    
    In hindsight however, this "fixup if we see 0" is a very poor, with a
    substantial degree of risk.  Explicitly default to grant table v1 during
    domain create, and let the rest of the code work safely in the knowledge 
that
    the version is sensibly set.
    
    Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
    Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
---
 xen/common/grant_table.c | 39 ++++++++-------------------------------
 1 file changed, 8 insertions(+), 31 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index d9ec711c73..009b1e2626 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -50,8 +50,8 @@ struct grant_table {
     /* Lock protecting the maptrack limit */
     spinlock_t            maptrack_lock;
     /*
-     * The defined versions are 1 and 2.  Set to 0 if we don't know
-     * what version to use yet.
+     * Defaults to v1.  May be changed with GNTTABOP_set_version.  All other
+     * values are invalid.
      */
     unsigned int          gt_version;
     /* Resource limits of the domain. */
@@ -222,7 +222,6 @@ nr_maptrack_frames(struct grant_table *t)
 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
@@ -1302,20 +1301,6 @@ unmap_common(
 
     grant_read_lock(rgt);
 
-    if ( rgt->gt_version == 0 )
-    {
-        /*
-         * This ought to be impossible, as such a mapping should not have
-         * been established (see the nr_grant_entries(rgt) bounds check in
-         * gnttab_map_grant_ref()). Doing this check only in
-         * gnttab_unmap_common_complete() - as it used to be done - would,
-         * however, be too late.
-         */
-        rc = GNTST_bad_gntref;
-        flags = 0;
-        goto unlock_out;
-    }
-
     op->rd = rd;
     op->ref = map->ref;
 
@@ -1932,9 +1917,6 @@ gnttab_setup_table(
         goto unlock;
     }
 
-    if ( gt->gt_version == 0 )
-        gt->gt_version = 1;
-
     if ( (op.nr_frames > nr_grant_frames(gt) ||
           ((gt->gt_version > 1) &&
            (grant_to_status_frames(op.nr_frames) > nr_status_frames(gt)))) &&
@@ -2991,15 +2973,11 @@ 
gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
 
     switch ( gt->gt_version )
     {
-    case 0:
-        if ( op.version == 2 )
-        {
     case 1:
-            /* XXX: We could maybe shrink the active grant table here. */
-            res = gnttab_populate_status_frames(currd, gt, 
nr_grant_frames(gt));
-            if ( res < 0)
-                goto out_unlock;
-        }
+        /* XXX: We could maybe shrink the active grant table here. */
+        res = gnttab_populate_status_frames(currd, gt, nr_grant_frames(gt));
+        if ( res < 0)
+            goto out_unlock;
         break;
     case 2:
         for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )
@@ -3594,6 +3572,8 @@ grant_table_create(
     percpu_rwlock_resource_init(&t->lock, grant_rwlock);
     spin_lock_init(&t->maptrack_lock);
 
+    t->gt_version = 1;
+
     /* Okay, install the structure. */
     t->domain = d;
     d->grant_table = t;
@@ -3869,9 +3849,6 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, 
gfn_t gfn,
 
     grant_write_lock(gt);
 
-    if ( gt->gt_version == 0 )
-        gt->gt_version = 1;
-
     if ( gt->gt_version == 2 &&
          (idx & XENMAPIDX_grant_table_status) )
     {
--
generated by git-patchbot for /home/xen/git/xen.git#master

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/xen-changelog