[PATCH v3 0/2] XSA-226

[PATCH v3 0/2] XSA-226

[Jan Beulich]

XSA-226 went out with just a workaround patch. The pair of patches
here became ready too late to be reasonably included in the XSA.
Nevertheless they aim at fixing the underlying issues, ideally making
the workaround unnecessary.

1: gnttab: don't use possibly unbounded tail calls
2: gnttab: fix transitive grant handling

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


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

[PATCH v3 1/2] gnttab: don't use possibly unbounded tail calls

[Jan Beulich]

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 XSA-226.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Zap *page prior to returning ERESTART. Fix i == 0 case in the exit
    path being added to gnttab_copy().

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2105,8 +2105,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 )
@@ -2257,10 +2259,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);
@@ -2268,9 +2271,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
@@ -2576,7 +2578,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;
     }
 
@@ -2586,7 +2588,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;
     }
 
@@ -2608,7 +2610,7 @@ static long gnttab_copy(
     {
         if ( i && hypercall_preempt_check() )
         {
-            rc = i;
+            rc = count - i;
             break;
         }
 
@@ -2618,13 +2620,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;
@@ -3162,6 +3171,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);
         }



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

[PATCH v3 2/2] gnttab: fix transitive grant handling

[Jan Beulich]

Processing of transitive grants must not use the fast path, or else
reference counting breaks due to the skipped recursive call to
__acquire_grant_for_copy() (its __release_grant_for_copy()
counterpart occurs independent of original pin count). Furthermore
after re-acquiring temporarily dropped locks we need to verify no grant
properties changed if the original pin count was non-zero; checking
just the pin counts is sufficient only for well-behaved guests. As a
result, __release_grant_for_copy() needs to mirror that new behavior.

Furthermore a __release_grant_for_copy() invocation was missing on the
retry path of __acquire_grant_for_copy(), and gnttab_set_version() also
needs to bail out upon encountering a transitive grant.

This is part of XSA-226.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: gnttab_set_version() also needs fixing.
v2: Also adjust __release_grant_for_copy(), as pointed out by Andrew.
    Call __release_grant_for_copy() on __acquire_grant_for_copy() error
    path. Also re-validate grant table version. Use _set_status_v2()
    directly.
---
I was tempted to replace the open coded barrier(), but then thought
it's better to not do unrelated cleanup (other than formatting of the
code being moved). There's also "rrd" mentioned in a comment, which I
would have corrected if only I knew what is being meant - the code
subsequent to the comment deals with td ...

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2052,13 +2052,8 @@ __release_grant_for_copy(
     unsigned long r_frame;
     uint16_t *status;
     grant_ref_t trans_gref;
-    int released_read;
-    int released_write;
     struct domain *td;
 
-    released_read = 0;
-    released_write = 0;
-
     grant_read_lock(rgt);
 
     act = active_entry_acquire(rgt, gref);
@@ -2088,17 +2083,11 @@ __release_grant_for_copy(
 
         act->pin -= GNTPIN_hstw_inc;
         if ( !(act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) )
-        {
-            released_write = 1;
             gnttab_clear_flag(_GTF_writing, status);
-        }
     }
 
     if ( !act->pin )
-    {
         gnttab_clear_flag(_GTF_reading, status);
-        released_read = 1;
-    }
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2106,13 +2095,10 @@ __release_grant_for_copy(
     if ( td != rd )
     {
         /*
-         * Recursive calls, but they're bounded (acquire permits only a single
+         * Recursive call, but it is 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 )
-            __release_grant_for_copy(td, trans_gref, 1);
+        __release_grant_for_copy(td, trans_gref, readonly);
 
         rcu_unlock_domain(td);
     }
@@ -2186,8 +2172,108 @@ __acquire_grant_for_copy(
                  act->domid, ldom, act->pin);
 
     old_pin = act->pin;
-    if ( !act->pin ||
-         (!readonly && !(act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask))) )
+    if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
+    {
+        if ( (!old_pin || (!readonly &&
+                           !(old_pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)))) &&
+             (rc = _set_status_v2(ldom, readonly, 0, shah, act,
+                                  status)) != GNTST_okay )
+             goto unlock_out;
+
+        if ( !allow_transitive )
+            PIN_FAIL(unlock_out_clear, GNTST_general_error,
+                     "transitive grant when transitivity not allowed\n");
+
+        trans_domid = sha2->transitive.trans_domid;
+        trans_gref = sha2->transitive.gref;
+        barrier(); /* Stop the compiler from re-loading
+                      trans_domid from shared memory */
+        if ( trans_domid == rd->domain_id )
+            PIN_FAIL(unlock_out_clear, GNTST_general_error,
+                     "transitive grants cannot be self-referential\n");
+
+        /*
+         * We allow the trans_domid == ldom case, which corresponds to a
+         * grant being issued by one domain, sent to another one, and then
+         * transitively granted back to the original domain.  Allowing it
+         * is easy, and means that you don't need to go out of your way to
+         * avoid it in the guest.
+         */
+
+        /* 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);
+
+        /*
+         * __acquire_grant_for_copy() could take the lock on the
+         * remote table (if rd == td), so we have to drop the lock
+         * here and reacquire.
+         */
+        active_entry_release(act);
+        grant_read_unlock(rgt);
+
+        rc = __acquire_grant_for_copy(td, trans_gref, rd->domain_id,
+                                      readonly, &grant_frame, page,
+                                      &trans_page_off, &trans_length, 0);
+
+        grant_read_lock(rgt);
+        act = active_entry_acquire(rgt, gref);
+
+        if ( rc != GNTST_okay )
+        {
+            __fixup_status_for_copy_pin(act, status);
+            rcu_unlock_domain(td);
+            active_entry_release(act);
+            grant_read_unlock(rgt);
+            return rc;
+        }
+
+        /*
+         * We dropped the lock, so we have to check that the grant didn't
+         * change, and that nobody else tried to pin/unpin it. If anything
+         * changed, just give up and tell the caller to retry.
+         */
+        if ( rgt->gt_version != 2 ||
+             act->pin != old_pin ||
+             (old_pin && (act->domid != ldom || act->frame != grant_frame ||
+                          act->start != trans_page_off ||
+                          act->length != trans_length ||
+                          act->trans_domain != td ||
+                          act->trans_gref != trans_gref ||
+                          !act->is_sub_page)) )
+        {
+            __release_grant_for_copy(td, trans_gref, readonly);
+            __fixup_status_for_copy_pin(act, status);
+            rcu_unlock_domain(td);
+            active_entry_release(act);
+            grant_read_unlock(rgt);
+            put_page(*page);
+            *page = NULL;
+            return ERESTART;
+        }
+
+        if ( !old_pin )
+        {
+            act->domid = ldom;
+            act->start = trans_page_off;
+            act->length = trans_length;
+            act->trans_domain = td;
+            act->trans_gref = trans_gref;
+            act->frame = grant_frame;
+            act->gfn = -1ul;
+            /*
+             * The actual remote remote grant may or may not be a sub-page,
+             * but we always treat it as one because that blocks mappings of
+             * transitive grants.
+             */
+            act->is_sub_page = 1;
+        }
+    }
+    else if ( !old_pin ||
+              (!readonly && !(old_pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask))) )
     {
         if ( (rc = _set_status(rgt->gt_version, ldom,
                                readonly, 0, shah, act,
@@ -2208,79 +2294,6 @@ __acquire_grant_for_copy(
             trans_page_off = 0;
             trans_length = PAGE_SIZE;
         }
-        else if ( (shah->flags & GTF_type_mask) == GTF_transitive )
-        {
-            if ( !allow_transitive )
-                PIN_FAIL(unlock_out_clear, GNTST_general_error,
-                         "transitive grant when transitivity not allowed\n");
-
-            trans_domid = sha2->transitive.trans_domid;
-            trans_gref = sha2->transitive.gref;
-            barrier(); /* Stop the compiler from re-loading
-                          trans_domid from shared memory */
-            if ( trans_domid == rd->domain_id )
-                PIN_FAIL(unlock_out_clear, GNTST_general_error,
-                         "transitive grants cannot be self-referential\n");
-
-            /* We allow the trans_domid == ldom case, which
-               corresponds to a grant being issued by one domain, sent
-               to another one, and then transitively granted back to
-               the original domain.  Allowing it is easy, and means
-               that you don't need to go out of your way to avoid it
-               in the guest. */
-
-            /* 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);
-
-            /*
-             * __acquire_grant_for_copy() could take the lock on the
-             * remote table (if rd == td), so we have to drop the lock
-             * here and reacquire
-             */
-            active_entry_release(act);
-            grant_read_unlock(rgt);
-
-            rc = __acquire_grant_for_copy(td, trans_gref, rd->domain_id,
-                                          readonly, &grant_frame, page,
-                                          &trans_page_off, &trans_length, 0);
-
-            grant_read_lock(rgt);
-            act = active_entry_acquire(rgt, gref);
-
-            if ( rc != GNTST_okay ) {
-                __fixup_status_for_copy_pin(act, status);
-                rcu_unlock_domain(td);
-                active_entry_release(act);
-                grant_read_unlock(rgt);
-                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 tell the caller to retry.
-             */
-            if ( act->pin != old_pin )
-            {
-                __fixup_status_for_copy_pin(act, status);
-                rcu_unlock_domain(td);
-                active_entry_release(act);
-                grant_read_unlock(rgt);
-                put_page(*page);
-                *page = NULL;
-                return ERESTART;
-            }
-
-            /* The actual remote remote grant may or may not be a
-               sub-page, but we always treat it as one because that
-               blocks mappings of transitive grants. */
-            is_sub_page = 1;
-            act->gfn = -1ul;
-        }
         else if ( !(sha2->hdr.flags & GTF_sub_page) )
         {
             rc = __get_paged_frame(sha2->full_page.frame, &grant_frame, page, readonly, rd);
@@ -2707,10 +2717,13 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARA
     case 2:
         for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )
         {
-            if ( ((shared_entry_v2(gt, i).hdr.flags & GTF_type_mask) ==
-                  GTF_permit_access) &&
-                 (shared_entry_v2(gt, i).full_page.frame >> 32) )
+            switch ( shared_entry_v2(gt, i).hdr.flags & GTF_type_mask )
             {
+            case GTF_permit_access:
+                 if ( !(shared_entry_v2(gt, i).full_page.frame >> 32) )
+                     break;
+                 /* fall through */
+            case GTF_transitive:
                 gdprintk(XENLOG_WARNING,
                          "tried to change grant table version to 1 with non-representable entries\n");
                 res = -ERANGE;



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

Re: [PATCH v3 0/2] XSA-226

[Steven Haigh]

[-- Attachment #1.1: Type: text/plain, Size: 691 bytes --]

On Tuesday, 15 August 2017 11:43:59 PM AEST Jan Beulich wrote:
> XSA-226 went out with just a workaround patch. The pair of patches
> here became ready too late to be reasonably included in the XSA.
> Nevertheless they aim at fixing the underlying issues, ideally making
> the workaround unnecessary.
> 
> 1: gnttab: don't use possibly unbounded tail calls
> 2: gnttab: fix transitive grant handling
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

If this turns out to be all good and accepted, is it possible to reissue 
xsa226 with the proper fixes?

-- 
Steven Haigh

📧 netwiz@crc.id.au       💻 http://www.crc.id.au
📞 +61 (3) 9001 6090    📱 0412 935 897

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v3 0/2] XSA-226

[Jan Beulich]

>>> On 15.08.17 at 16:08, <netwiz@crc.id.au> wrote:
> On Tuesday, 15 August 2017 11:43:59 PM AEST Jan Beulich wrote:
>> XSA-226 went out with just a workaround patch. The pair of patches
>> here became ready too late to be reasonably included in the XSA.
>> Nevertheless they aim at fixing the underlying issues, ideally making
>> the workaround unnecessary.
>> 
>> 1: gnttab: don't use possibly unbounded tail calls
>> 2: gnttab: fix transitive grant handling
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> If this turns out to be all good and accepted, is it possible to reissue 
> xsa226 with the proper fixes?

I think that would be likely to happen.

Jan


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

Re: [PATCH v3 1/2] gnttab: don't use possibly unbounded tail calls

[Andrew Cooper]

On 15/08/17 14:49, Jan Beulich wrote:
> @@ -2608,7 +2610,7 @@ static long gnttab_copy(
>      {
>          if ( i && hypercall_preempt_check() )
>          {
> -            rc = i;
> +            rc = count - i;

Somewhere, probably as a comment for gnttab_copy(), we should have a
comment explaining why the return value is different from all other ops,
which will also go somewhat to explaining the "rc = count - rc;" logic.

I think it would also be wise to have an early exit in
do_grant_table_op() for passing a count of 0.  As far as I can tell, we
will get all the way into the subop handler before discovering a count of 0.

Otherwise, LGTM.

~Andrew

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

Re: [PATCH v3 2/2] gnttab: fix transitive grant handling

[Andrew Cooper]

On 15/08/17 14:49, Jan Beulich wrote:
> Processing of transitive grants must not use the fast path, or else
> reference counting breaks due to the skipped recursive call to
> __acquire_grant_for_copy() (its __release_grant_for_copy()
> counterpart occurs independent of original pin count). Furthermore
> after re-acquiring temporarily dropped locks we need to verify no grant
> properties changed if the original pin count was non-zero; checking
> just the pin counts is sufficient only for well-behaved guests. As a
> result, __release_grant_for_copy() needs to mirror that new behavior.
>
> Furthermore a __release_grant_for_copy() invocation was missing on the
> retry path of __acquire_grant_for_copy(), and gnttab_set_version() also
> needs to bail out upon encountering a transitive grant.
>
> This is part of XSA-226.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v3 1/2] gnttab: don't use possibly unbounded tail calls

[Jan Beulich]

>>> On 15.08.17 at 17:04, <andrew.cooper3@citrix.com> wrote:
> On 15/08/17 14:49, Jan Beulich wrote:
>> @@ -2608,7 +2610,7 @@ static long gnttab_copy(
>>      {
>>          if ( i && hypercall_preempt_check() )
>>          {
>> -            rc = i;
>> +            rc = count - i;
> 
> Somewhere, probably as a comment for gnttab_copy(), we should have a
> comment explaining why the return value is different from all other ops,
> which will also go somewhat to explaining the "rc = count - rc;" logic.

Sure.

> I think it would also be wise to have an early exit in
> do_grant_table_op() for passing a count of 0.  As far as I can tell, we
> will get all the way into the subop handler before discovering a count of 0.

Well, that would collide with {get,set}_version which don't currently
honor count (and hence existing callers may be passing zero here).
Otherwise I would agree with what you propose.

Jan


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

Re: [PATCH v3 1/2] gnttab: don't use possibly unbounded tail calls

[Andrew Cooper]

On 16/08/17 10:52, Jan Beulich wrote:
>>>> On 15.08.17 at 17:04, <andrew.cooper3@citrix.com> wrote:
>> On 15/08/17 14:49, Jan Beulich wrote:
>>> @@ -2608,7 +2610,7 @@ static long gnttab_copy(
>>>      {
>>>          if ( i && hypercall_preempt_check() )
>>>          {
>>> -            rc = i;
>>> +            rc = count - i;
>> Somewhere, probably as a comment for gnttab_copy(), we should have a
>> comment explaining why the return value is different from all other ops,
>> which will also go somewhat to explaining the "rc = count - rc;" logic.
> Sure.
>
>> I think it would also be wise to have an early exit in
>> do_grant_table_op() for passing a count of 0.  As far as I can tell, we
>> will get all the way into the subop handler before discovering a count of 0.
> Well, that would collide with {get,set}_version which don't currently
> honor count (and hence existing callers may be passing zero here).
> Otherwise I would agree with what you propose.

Lovely :(

We've also got a number of other issues, like the fact that count, being
unsigned int, gets silently truncated in the non-compat case.

~Andrew

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

Re: [PATCH v3 1/2] gnttab: don't use possibly unbounded tail calls

[Jan Beulich]

>>> On 16.08.17 at 12:01, <andrew.cooper3@citrix.com> wrote:
> On 16/08/17 10:52, Jan Beulich wrote:
>>>>> On 15.08.17 at 17:04, <andrew.cooper3@citrix.com> wrote:
>>> On 15/08/17 14:49, Jan Beulich wrote:
>>>> @@ -2608,7 +2610,7 @@ static long gnttab_copy(
>>>>      {
>>>>          if ( i && hypercall_preempt_check() )
>>>>          {
>>>> -            rc = i;
>>>> +            rc = count - i;
>>> Somewhere, probably as a comment for gnttab_copy(), we should have a
>>> comment explaining why the return value is different from all other ops,
>>> which will also go somewhat to explaining the "rc = count - rc;" logic.
>> Sure.
>>
>>> I think it would also be wise to have an early exit in
>>> do_grant_table_op() for passing a count of 0.  As far as I can tell, we
>>> will get all the way into the subop handler before discovering a count of 0.
>> Well, that would collide with {get,set}_version which don't currently
>> honor count (and hence existing callers may be passing zero here).
>> Otherwise I would agree with what you propose.
> 
> Lovely :(
> 
> We've also got a number of other issues, like the fact that count, being
> unsigned int, gets silently truncated in the non-compat case.

Truncated? I don't see any such case (nor why this would be
non-compat specific). There is a check right at the start of the
function to make sure huge values can't be mistaken as error
values returned by the helper functions. You're not referring
to the fact that a caller might be passing an unsigned long
count, are you? That would be a problem with any unsigned
int hypercall parameters (e.g. also with "cmd" here), but I
don't view this as a problem at all: These are defined to take
32-bit parameters only. For example, Linux'es 
HYPERVISOR_grant_table_op() also properly has both as
unsigned int.

Jan


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