[Xen-changelog] [xen stable-4.1] Fix rcu domain locking for transitive grants

[patchbot@xxxxxxx]

commit a12ed396652c22988e0e7a3f5bd57c882872da8e
Author:     Paul Durrant <paul.durrant@xxxxxxxxxx>
AuthorDate: Thu Apr 18 17:38:17 2013 +0200
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Thu Apr 18 17:38:17 2013 +0200

    Fix rcu domain locking for transitive grants
    
    When acquiring a transitive grant for copy then the owning domain
    needs to be locked down as well as the granting domain. This was being
    done, but the unlocking was not. The acquire code now stores the
    struct domain * of the owning domain (rather than the domid) in the
    active entry in the granting domain. The release code then does the
    unlock on the owning domain.  Note that I believe I also fixed a bug
    where, for non-transitive grants the active entry contained a
    reference to the acquiring domain rather than the granting
    domain. From my reading of the code this would stop the release code
    for transitive grants from terminating its recursion correctly.
    
    Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
    master commit: f544bf377ee829e1342abd818ac30478c6f3a134
    master date: 2011-03-08 16:30:30 +0000
    
    Also, for non-transitive grants we now avoid incorrectly recursing
    in __release_grant_for_copy.
    
    This is CVE-2013-1964 / XSA-50.
    
    Reported-by: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
    Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
    Tested-by: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
---
 xen/common/grant_table.c      |   55 ++++++++++++++++++----------------------
 xen/include/xen/grant_table.h |    2 +-
 2 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a180aef..e0f91ab 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -598,7 +598,7 @@ __gnttab_map_grant_ref(
             act->start = 0;
             act->length = PAGE_SIZE;
             act->is_sub_page = 0;
-            act->trans_dom = rd->domain_id;
+            act->trans_domain = rd;
             act->trans_gref = op->ref;
         }
     }
@@ -1629,11 +1629,10 @@ __release_grant_for_copy(
     struct active_grant_entry *act;
     unsigned long r_frame;
     uint16_t *status;
-    domid_t trans_domid;
     grant_ref_t trans_gref;
     int released_read;
     int released_write;
-    struct domain *trans_dom;
+    struct domain *td;
 
     released_read = 0;
     released_write = 0;
@@ -1647,15 +1646,13 @@ __release_grant_for_copy(
     if (rd->grant_table->gt_version == 1)
     {
         status = &sha->flags;
-        trans_domid = rd->domain_id;
-        /* Shut the compiler up.  This'll never be used, because
-           trans_domid == rd->domain_id, but gcc doesn't know that. */
-        trans_gref = 0x1234567;
+        td = rd;
+        trans_gref = gref;
     }
     else
     {
         status = &status_entry(rd->grant_table, gref);
-        trans_domid = act->trans_dom;
+        td = act->trans_domain;
         trans_gref = act->trans_gref;
     }
 
@@ -1683,21 +1680,16 @@ __release_grant_for_copy(
 
     spin_unlock(&rd->grant_table->lock);
 
-    if ( trans_domid != rd->domain_id )
+    if ( td != rd )
     {
-        if ( released_write || released_read )
-        {
-            trans_dom = rcu_lock_domain_by_id(trans_domid);
-            if ( trans_dom != NULL )
-            {
-                /* Recursive calls, but they're tail calls, so it's
-                   okay. */
-                if ( released_write )
-                    __release_grant_for_copy(trans_dom, trans_gref, 0);
-                else if ( released_read )
-                    __release_grant_for_copy(trans_dom, trans_gref, 1);
-            }
-        }
+        /* Recursive calls, but they're tail calls, so it's
+           okay. */
+        if ( released_write )
+            __release_grant_for_copy(td, trans_gref, 0);
+        else if ( released_read )
+            __release_grant_for_copy(td, trans_gref, 1);
+
+       rcu_unlock_domain(td);
     }
 }
 
@@ -1734,7 +1726,7 @@ __acquire_grant_for_copy(
     uint32_t old_pin;
     domid_t trans_domid;
     grant_ref_t trans_gref;
-    struct domain *rrd;
+    struct domain *td;
     unsigned long gfn;
     unsigned long grant_frame;
     unsigned trans_page_off;
@@ -1788,8 +1780,8 @@ __acquire_grant_for_copy(
                                status) ) != GNTST_okay )
              goto unlock_out;
 
-        trans_domid = ld->domain_id;
-        trans_gref = 0;
+        td = rd;
+        trans_gref = gref;
         if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
         {
             if ( !allow_transitive )
@@ -1811,14 +1803,15 @@ __acquire_grant_for_copy(
                that you don't need to go out of your way to avoid it
                in the guest. */
 
-            rrd = rcu_lock_domain_by_id(trans_domid);
-            if ( rrd == NULL )
+            /* We need to leave the rrd locked during the grant copy */
+            td = rcu_lock_domain_by_id(trans_domid);
+            if ( td == NULL )
                 PIN_FAIL(unlock_out_clear, GNTST_general_error,
                          "transitive grant referenced bad domain %d\n",
                          trans_domid);
             spin_unlock(&rd->grant_table->lock);
 
-            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
+            rc = __acquire_grant_for_copy(td, trans_gref, rd,
                                           readonly, &grant_frame,
                                           &trans_page_off, &trans_length,
                                           0, &ignore);
@@ -1826,6 +1819,7 @@ __acquire_grant_for_copy(
             spin_lock(&rd->grant_table->lock);
             if ( rc != GNTST_okay ) {
                 __fixup_status_for_copy_pin(act, status);
+                rcu_unlock_domain(td);
                 spin_unlock(&rd->grant_table->lock);
                 return rc;
             }
@@ -1837,6 +1831,7 @@ __acquire_grant_for_copy(
             if ( act->pin != old_pin )
             {
                 __fixup_status_for_copy_pin(act, status);
+                rcu_unlock_domain(td);
                 spin_unlock(&rd->grant_table->lock);
                 return __acquire_grant_for_copy(rd, gref, ld, readonly,
                                                 frame, page_off, length,
@@ -1848,7 +1843,7 @@ __acquire_grant_for_copy(
                sub-page, but we always treat it as one because that
                blocks mappings of transitive grants. */
             is_sub_page = 1;
-            *owning_domain = rrd;
+            *owning_domain = td;
             act->gfn = -1ul;
         }
         else if ( sha1 )
@@ -1894,7 +1889,7 @@ __acquire_grant_for_copy(
             act->is_sub_page = is_sub_page;
             act->start = trans_page_off;
             act->length = trans_length;
-            act->trans_dom = trans_domid;
+            act->trans_domain = td;
             act->trans_gref = trans_gref;
             act->frame = grant_frame;
         }
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 20c8354..c161705 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -32,7 +32,7 @@
 struct active_grant_entry {
     u32           pin;    /* Reference count information.             */
     domid_t       domid;  /* Domain being granted access.             */
-    domid_t       trans_dom;
+    struct domain *trans_domain;
     uint32_t      trans_gref;
     unsigned long frame;  /* Frame being granted.                     */
     unsigned long gfn;    /* Guest's idea of the frame being granted. */
--
generated by git-patchbot for /home/xen/git/xen.git#stable-4.1

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog