From: Jan Beulich <jbeulich@suse.com>

Subject: gnttab: don't use possibly unbounded tail calls

There is no guarantee that the compiler would actually translate them

to branches instead of calls, so only ones with a known recursion limit

are okay:

- __release_grant_for_copy() can call itself only once, as

  __acquire_grant_for_copy() won't permit use of multi-level transitive

  grants,

- __acquire_grant_for_copy() is fine to call itself with the last

  argument false, as that prevents further recursion,

- __acquire_grant_for_copy() must not call itself to recover from an

  observed change to the active entry's pin count

This is part of CVE-2017-12135 / XSA-226.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/compat/grant_table.c

+++ b/xen/common/compat/grant_table.c

@@ -258,9 +258,9 @@ int compat_grant_table_op(unsigned int cmd,

                 rc = gnttab_copy(guest_handle_cast(nat.uop, gnttab_copy_t), n);

             if ( rc > 0 )

             {

-                ASSERT(rc < n);

-                i -= n - rc;

-                n = rc;

+                ASSERT(rc <= n);

+                i -= rc;

+                n -= rc;

             }

             if ( rc >= 0 )

             {

--- a/xen/common/grant_table.c

+++ b/xen/common/grant_table.c

@@ -2103,8 +2103,10 @@ __release_grant_for_copy(

     if ( td != rd )

     {

-        /* Recursive calls, but they're tail calls, so it's

-           okay. */

+        /*

+         * Recursive calls, but they're bounded (acquire permits only a single

+         * level of transitivity), so it's okay.

+         */

         if ( released_write )

             __release_grant_for_copy(td, trans_gref, 0);

         else if ( released_read )

@@ -2255,10 +2257,11 @@ __acquire_grant_for_copy(

                 return rc;

             }

-            /* We dropped the lock, so we have to check that nobody

-               else tried to pin (or, for that matter, unpin) the

-               reference in *this* domain.  If they did, just give up

-               and try again. */

+            /*

+             * We dropped the lock, so we have to check that nobody else tried

+             * to pin (or, for that matter, unpin) the reference in *this*

+             * domain.  If they did, just give up and tell the caller to retry.

+             */

             if ( act->pin != old_pin )

             {

                 __fixup_status_for_copy_pin(act, status);

@@ -2266,9 +2269,8 @@ __acquire_grant_for_copy(

                 active_entry_release(act);

                 grant_read_unlock(rgt);

                 put_page(*page);

-                return __acquire_grant_for_copy(rd, gref, ldom, readonly,

-                                                frame, page, page_off, length,

-                                                allow_transitive);

+                *page = NULL;

+                return ERESTART;

             }

             /* The actual remote remote grant may or may not be a

@@ -2574,7 +2576,7 @@ static int gnttab_copy_one(const struct

     {

         gnttab_copy_release_buf(src);

         rc = gnttab_copy_claim_buf(op, &op->source, src, GNTCOPY_source_gref);

-        if ( rc < 0 )

+        if ( rc )

             goto out;

     }

@@ -2584,7 +2586,7 @@ static int gnttab_copy_one(const struct

     {

         gnttab_copy_release_buf(dest);

         rc = gnttab_copy_claim_buf(op, &op->dest, dest, GNTCOPY_dest_gref);

-        if ( rc < 0 )

+        if ( rc )

             goto out;

     }

@@ -2593,6 +2595,14 @@ static int gnttab_copy_one(const struct

     return rc;

 }

+/*

+ * gnttab_copy(), other than the various other helpers of

+ * do_grant_table_op(), returns (besides possible error indicators)

+ * "count - i" rather than "i" to ensure that even if no progress

+ * was made at all (perhaps due to gnttab_copy_one() returning a

+ * positive value) a non-zero value is being handed back (zero needs

+ * to be avoided, as that means "success, all done").

+ */

 static long gnttab_copy(

     XEN_GUEST_HANDLE_PARAM(gnttab_copy_t) uop, unsigned int count)

 {

@@ -2606,7 +2616,7 @@ static long gnttab_copy(

     {

         if ( i && hypercall_preempt_check() )

         {

-            rc = i;

+            rc = count - i;

             break;

         }

@@ -2616,13 +2626,20 @@ static long gnttab_copy(

             break;

         }

-        op.status = gnttab_copy_one(&op, &dest, &src;;

-        if ( op.status != GNTST_okay )

+        rc = gnttab_copy_one(&op, &dest, &src;;

+        if ( rc > 0 )

+        {

+            rc = count - i;

+            break;

+        }

+        if ( rc != GNTST_okay )

         {

             gnttab_copy_release_buf(&src;;

             gnttab_copy_release_buf(&dest);

         }

+        op.status = rc;

+        rc = 0;

         if ( unlikely(__copy_field_to_guest(uop, &op, status)) )

         {

             rc = -EFAULT;

@@ -3160,6 +3177,7 @@ do_grant_table_op(

         rc = gnttab_copy(copy, count);

         if ( rc > 0 )

         {

+            rc = count - rc;

             guest_handle_add_offset(copy, rc);

             uop = guest_handle_cast(copy, void);

         }