[BUG] assertion failure in do_grant_table_op()

[BUG] assertion failure in do_grant_table_op()

[Jann Horn]

A fuzzer based on AFL and TriforceAFL discovered an assertion
violation in Xen 4.9.1.

The issue is that, when `opaque_out` is non-zero,
do_grant_table_op() assumes that the hypercall was preempted and a
continuation is generated. However, `opaque_out` also ends up being
non-zero if the guest called GNTTABOP_cache_flush with
`opaque_in != 0` and `count == 0`, in which case there is no more
work to do.

In release builds, this is not an issue: A guest that performs such
a nonsensical hypercall goes into an endless hypercall-calling loop,
which the guest can detect as a soft kernel lockup. This does not
interfere with the normal operation of the hypervisor and does not
even interfere with other tasks running in the guest if the guest
kernel supports preemption.


Reproducer:



root@pv-guest:~/borkmod2# cat borker.c
#include <linux/module.h>
#include <linux/kernel.h>

static int __init init_mod(void) {
  asm volatile (
    "mov $20, %%rax\n\t" /*__HYPERVISOR_grant_table_op*/
    "mov $0x800c, %%rdi\n\t" /*GNTTABOP_cache_flush|0x8000*/
    "mov $0, %%rsi\n\t"
    "mov $0, %%rdx\n\t"
    "syscall\n\t"
  : //out
  : //in
  : //clobber
    "cc","memory","rax","rdi","rsi","rdx","rcx","r11"
  );
  return -EINVAL;
}

module_init(init_mod);
root@pv-guest:~/borkmod2# cat Makefile
obj-m := borker.o
KDIR := /lib/modules/$(shell uname -r)/build
PWD := $(shell pwd)

all:
$(MAKE) -C $(KDIR) M=$(PWD) modules

clean:
$(MAKE) -C $(KDIR) M=$(PWD) clean

root@pv-guest:~/borkmod2# make
make -C /lib/modules/4.9.0-4-amd64/build M=/root/borkmod2 modules
make[1]: Entering directory '/usr/src/linux-headers-4.9.0-4-amd64'
  Building modules, stage 2.
  MODPOST 1 modules
make[1]: Leaving directory '/usr/src/linux-headers-4.9.0-4-amd64'
root@pv-guest:~/borkmod2# insmod borker.ko



Resulting panic on a debug build:



(XEN) Assertion 'rc < count' failed at grant_table.c:3273
(XEN) ----[ Xen-4.9.1  x86_64  debug=y   Not tainted ]----
(XEN) CPU:    0
(XEN) RIP:    e008:[<ffff82d08021579b>] do_grant_table_op+0x1e2c/0x2272
(XEN) RFLAGS: 0000000000010246   CONTEXT: hypervisor (d1v0)
(XEN) rax: 0000000000000000   rbx: ffff8300bfc57f18   rcx: ffff82d080378680
(XEN) rdx: ffff07ffffffffff   rsi: 0000000000000000   rdi: 000000000000000c
(XEN) rbp: ffff8300bfc57e68   rsp: ffff8300bfc57d88   r8:  0000000000000000
(XEN) r9:  deadbeefdeadf00d   r10: 0000000000007ff0   r11: 0000000000000246
(XEN) r12: 0000000000008000   r13: 0000000000000014   r14: 0000000000000000
(XEN) r15: 000000000000000c   cr0: 0000000080050033   cr4: 00000000001506e4
(XEN) cr3: 000000012282d000   cr2: ffff880014786918
(XEN) fsb: 00007fd847e48700   gsb: ffff880018c00000   gss: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around <ffff82d08021579b> (do_grant_table_op+0x1e2c/0x2272):
(XEN)  ff ff ff e9 3c f1 ff ff <0f> 0b 0f 0b 48 c7 c0 ea ff ff ff e9 75 f1 ff ff
(XEN) Xen stack trace from rsp=ffff8300bfc57d88:
(XEN)    ffff8300bfc57e68 ffff82d08028fb8f 000000000000006e ffff880018c0b8e0
(XEN)    0000000d00000000 ffffffff81059d42 000000000000e033 0000000000011002
(XEN)    ffffc9004029fd70 ffff8300bfc57de8 ffff82d000000000 000000008058fdd8
(XEN)    0000000000000000 00007ff0ffffffea ffff880018c0c160 0000000200000000
(XEN)    ffffffff81059d40 0000000033d80000 0000000000011002 000000000000000f
(XEN)    0000000000122831 ffff880018c182a8 0000000000011002 ffff8300bfc57f18
(XEN)    ffff8300bfc22000 0000000000000014 ffff82d08021396f deadbeefdeadf00d
(XEN)    ffff8300bfc57f08 ffff82d08035d14f 0300000000000000 000000000000800c
(XEN)    0000000000000000 0000000000000000 deadbeefdeadf00d deadbeefdeadf00d
(XEN)    0000000000000000 0000000000000000 0000000000000000 ffffffffffffffff
(XEN)    0000000000000000 0000000100000000 0000000000000000 ffff8300bfc22000
(XEN)    ffff88001417b000 ffff880013d46300 ffffffffc0070000 ffffffffc0070050
(XEN)    00007cff403a80c7 ffff82d080360ff6 ffff880013a15100 ffff880013a156b8
(XEN)    ffff880013a15100 ffff880018c18d10 ffffffffc0096000 0000000000000000
(XEN)    0000000000000246 0000000000007ff0 0000000000000013 000000000001f958
(XEN)    0000000000000014 ffffffffc009601e 0000000000000000 0000000000000000
(XEN)    000000000000800c 0001010000000000 ffffffffc009601e 000000000000e033
(XEN)    0000000000000246 ffffc90040817cd8 000000000000e02b 000000000009dfa3
(XEN)    003bb8b800000001 003bbd140009dfa3 000000000009dfb0 003bbcec00000000
(XEN)    ffff8300bfc22000 0000000000000000 00000000001506e4
(XEN) Xen call trace:
(XEN)    [<ffff82d08021579b>] do_grant_table_op+0x1e2c/0x2272
(XEN)    [<ffff82d08035d14f>] pv_hypercall+0x150/0x460
(XEN)    [<ffff82d080360ff6>] entry.o#test_all_events+0/0x30
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion 'rc < count' failed at grant_table.c:3273
(XEN) ****************************************
(XEN)
(XEN) Reboot in five seconds...

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

[PATCH 0/2] gnttab: improve GNTTABOP_cache_flush handling

[Jan Beulich]

1: correct GNTTABOP_cache_flush empty batch handling
2: improve GNTTABOP_cache_flush locking

Compile tested only, as this is being used by ARM only. I'd therefore
appreciate an ARM person to take a close look and/or try it out.

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


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

[PATCH 1/2] gnttab: correct GNTTABOP_cache_flush empty batch handling

[Jan Beulich]

Jann validly points out that with a caller bogusly requesting a zero-
element batch with non-zero high command bits (the ones used for
continuation encoding), the assertion right before the call to
hypercall_create_continuation() would trigger. A similar situation would
arise afaict for non-empty batches with op and/or length zero in every
element.

While we want the former to succeed (as we do elsewhere for similar
no-op requests), the latter can clearly be converted to an error, as
this is a state that can't be the result of a prior operation.

Take the opportunity and also correct the order of argument checks:
We shouldn't accept zero-length elements with unknown bits set in "op".
Also constify cache_flush()'s first parameter.

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3208,7 +3208,7 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_P
     return 0;
 }
 
-static int cache_flush(gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
+static int cache_flush(const gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
 {
     struct domain *d, *owner;
     struct page_info *page;
@@ -3218,19 +3218,17 @@ static int cache_flush(gnttab_cache_flus
 
     if ( (cflush->offset >= PAGE_SIZE) ||
          (cflush->length > PAGE_SIZE) ||
-         (cflush->offset + cflush->length > PAGE_SIZE) )
+         (cflush->offset + cflush->length > PAGE_SIZE) ||
+         (cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN)) )
         return -EINVAL;
 
     if ( cflush->length == 0 || cflush->op == 0 )
-        return 0;
+        return !*cur_ref ? 0 : -EILSEQ;
 
     /* currently unimplemented */
     if ( cflush->op & GNTTAB_CACHE_SOURCE_GREF )
         return -EOPNOTSUPP;
 
-    if ( cflush->op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) )
-        return -EINVAL;
-
     d = rcu_lock_current_domain();
     mfn = cflush->a.dev_bus_addr >> PAGE_SHIFT;
 
@@ -3310,6 +3308,9 @@ gnttab_cache_flush(XEN_GUEST_HANDLE_PARA
         *cur_ref = 0;
         guest_handle_add_offset(uop, 1);
     }
+
+    *cur_ref = 0;
+
     return 0;
 }
 




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

[PATCH 2/2] gnttab: improve GNTTABOP_cache_flush locking

[Jan Beulich]

Dropping the lock before returning from grant_map_exists() means handing
possibly stale information back to the caller. Return back the pointer
to the active entry instead, for the caller to release the lock once
done.

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

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -786,10 +786,10 @@ static int _set_status(unsigned gt_versi
         return _set_status_v2(domid, readonly, mapflag, shah, act, status);
 }
 
-static int grant_map_exists(const struct domain *ld,
-                            struct grant_table *rgt,
-                            unsigned long mfn,
-                            grant_ref_t *cur_ref)
+static struct active_grant_entry *grant_map_exists(const struct domain *ld,
+                                                   struct grant_table *rgt,
+                                                   unsigned long mfn,
+                                                   grant_ref_t *cur_ref)
 {
     grant_ref_t ref, max_iter;
 
@@ -805,28 +805,20 @@ static int grant_map_exists(const struct
                    nr_grant_entries(rgt));
     for ( ref = *cur_ref; ref < max_iter; ref++ )
     {
-        struct active_grant_entry *act;
-        bool_t exists;
-
-        act = active_entry_acquire(rgt, ref);
-
-        exists = act->pin
-            && act->domid == ld->domain_id
-            && act->frame == mfn;
+        struct active_grant_entry *act = active_entry_acquire(rgt, ref);
 
+        if ( act->pin && act->domid == ld->domain_id && act->frame == mfn )
+            return act;
         active_entry_release(act);
-
-        if ( exists )
-            return 0;
     }
 
     if ( ref < nr_grant_entries(rgt) )
     {
         *cur_ref = ref;
-        return 1;
+        return NULL;
     }
 
-    return -EINVAL;
+    return ERR_PTR(-EINVAL);
 }
 
 #define MAPKIND_READ 1
@@ -3213,6 +3205,7 @@ static int cache_flush(const gnttab_cach
     struct domain *d, *owner;
     struct page_info *page;
     unsigned long mfn;
+    struct active_grant_entry *act = NULL;
     void *v;
     int ret;
 
@@ -3250,13 +3243,13 @@ static int cache_flush(const gnttab_cach
     {
         grant_read_lock(owner->grant_table);
 
-        ret = grant_map_exists(d, owner->grant_table, mfn, cur_ref);
-        if ( ret != 0 )
+        act = grant_map_exists(d, owner->grant_table, mfn, cur_ref);
+        if ( IS_ERR_OR_NULL(act) )
         {
             grant_read_unlock(owner->grant_table);
             rcu_unlock_domain(d);
             put_page(page);
-            return ret;
+            return act ? PTR_ERR(act) : 1;
         }
     }
 
@@ -3273,7 +3266,11 @@ static int cache_flush(const gnttab_cach
         ret = 0;
 
     if ( d != owner )
+    {
+        active_entry_release(act);
         grant_read_unlock(owner->grant_table);
+    }
+
     unmap_domain_page(v);
     put_page(page);
 




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

Re: [PATCH 1/2] gnttab: correct GNTTABOP_cache_flush empty batch handling

[Andre Przywara]

Hi,

On 30/11/17 14:31, Jan Beulich wrote:
> Jann validly points out that with a caller bogusly requesting a zero-
> element batch with non-zero high command bits (the ones used for
> continuation encoding), the assertion right before the call to
> hypercall_create_continuation() would trigger. A similar situation would
> arise afaict for non-empty batches with op and/or length zero in every
> element.
> 
> While we want the former to succeed (as we do elsewhere for similar
> no-op requests), the latter can clearly be converted to an error, as
> this is a state that can't be the result of a prior operation.
> 
> Take the opportunity and also correct the order of argument checks:
> We shouldn't accept zero-length elements with unknown bits set in "op".
> Also constify cache_flush()'s first parameter.
> 
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Took me a while to wrap my head around it, because the actual fix is
just the "*cur_ref = 0;" line, I think.
But this looks correct to me.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3208,7 +3208,7 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_P
>      return 0;
>  }
>  
> -static int cache_flush(gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
> +static int cache_flush(const gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
>  {
>      struct domain *d, *owner;
>      struct page_info *page;
> @@ -3218,19 +3218,17 @@ static int cache_flush(gnttab_cache_flus
>  
>      if ( (cflush->offset >= PAGE_SIZE) ||
>           (cflush->length > PAGE_SIZE) ||
> -         (cflush->offset + cflush->length > PAGE_SIZE) )
> +         (cflush->offset + cflush->length > PAGE_SIZE) ||
> +         (cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN)) )
>          return -EINVAL;
>  
>      if ( cflush->length == 0 || cflush->op == 0 )
> -        return 0;
> +        return !*cur_ref ? 0 : -EILSEQ;
>  
>      /* currently unimplemented */
>      if ( cflush->op & GNTTAB_CACHE_SOURCE_GREF )
>          return -EOPNOTSUPP;
>  
> -    if ( cflush->op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) )
> -        return -EINVAL;
> -
>      d = rcu_lock_current_domain();
>      mfn = cflush->a.dev_bus_addr >> PAGE_SHIFT;
>  
> @@ -3310,6 +3308,9 @@ gnttab_cache_flush(XEN_GUEST_HANDLE_PARA
>          *cur_ref = 0;
>          guest_handle_add_offset(uop, 1);
>      }
> +
> +    *cur_ref = 0;
> +
>      return 0;
>  }
>  

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

Re: [PATCH 2/2] gnttab: improve GNTTABOP_cache_flush locking

[Andre Przywara]

Hi,

On 30/11/17 14:32, Jan Beulich wrote:
> Dropping the lock before returning from grant_map_exists() means handing
> possibly stale information back to the caller. Return back the pointer
> to the active entry instead, for the caller to release the lock once
> done.

I don't know enough about grant tables to reason about the deeper
meaning of this patch, but at least I can confirm that the amended
locking scheme seems to be correct (now).
I just wonder if it's worthwhile to add a comment that the function
takes a lock, but leaves it up to the caller to drop it. Since there is
only one caller, this might be overkill, though.

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

Reviewed-by: Andre Przywara <andre.przywara@linaro.org>

Cheers,
Andre.

> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -786,10 +786,10 @@ static int _set_status(unsigned gt_versi
>          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
>  }
>  
> -static int grant_map_exists(const struct domain *ld,
> -                            struct grant_table *rgt,
> -                            unsigned long mfn,
> -                            grant_ref_t *cur_ref)
> +static struct active_grant_entry *grant_map_exists(const struct domain *ld,
> +                                                   struct grant_table *rgt,
> +                                                   unsigned long mfn,
> +                                                   grant_ref_t *cur_ref)
>  {
>      grant_ref_t ref, max_iter;
>  
> @@ -805,28 +805,20 @@ static int grant_map_exists(const struct
>                     nr_grant_entries(rgt));
>      for ( ref = *cur_ref; ref < max_iter; ref++ )
>      {
> -        struct active_grant_entry *act;
> -        bool_t exists;
> -
> -        act = active_entry_acquire(rgt, ref);
> -
> -        exists = act->pin
> -            && act->domid == ld->domain_id
> -            && act->frame == mfn;
> +        struct active_grant_entry *act = active_entry_acquire(rgt, ref);
>  
> +        if ( act->pin && act->domid == ld->domain_id && act->frame == mfn )
> +            return act;
>          active_entry_release(act);
> -
> -        if ( exists )
> -            return 0;
>      }
>  
>      if ( ref < nr_grant_entries(rgt) )
>      {
>          *cur_ref = ref;
> -        return 1;
> +        return NULL;
>      }
>  
> -    return -EINVAL;
> +    return ERR_PTR(-EINVAL);
>  }
>  
>  #define MAPKIND_READ 1
> @@ -3213,6 +3205,7 @@ static int cache_flush(const gnttab_cach
>      struct domain *d, *owner;
>      struct page_info *page;
>      unsigned long mfn;
> +    struct active_grant_entry *act = NULL;
>      void *v;
>      int ret;
>  
> @@ -3250,13 +3243,13 @@ static int cache_flush(const gnttab_cach
>      {
>          grant_read_lock(owner->grant_table);
>  
> -        ret = grant_map_exists(d, owner->grant_table, mfn, cur_ref);
> -        if ( ret != 0 )
> +        act = grant_map_exists(d, owner->grant_table, mfn, cur_ref);
> +        if ( IS_ERR_OR_NULL(act) )
>          {
>              grant_read_unlock(owner->grant_table);
>              rcu_unlock_domain(d);
>              put_page(page);
> -            return ret;
> +            return act ? PTR_ERR(act) : 1;
>          }
>      }
>  
> @@ -3273,7 +3266,11 @@ static int cache_flush(const gnttab_cach
>          ret = 0;
>  
>      if ( d != owner )
> +    {
> +        active_entry_release(act);
>          grant_read_unlock(owner->grant_table);
> +    }
> +
>      unmap_domain_page(v);
>      put_page(page);
>  

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

Re: [PATCH 1/2] gnttab: correct GNTTABOP_cache_flush empty batch handling

[Stefano Stabellini]

On Thu, 30 Nov 2017, Jan Beulich wrote:
> Jann validly points out that with a caller bogusly requesting a zero-
> element batch with non-zero high command bits (the ones used for
> continuation encoding), the assertion right before the call to
> hypercall_create_continuation() would trigger. A similar situation would
> arise afaict for non-empty batches with op and/or length zero in every
> element.
> 
> While we want the former to succeed (as we do elsewhere for similar
> no-op requests), the latter can clearly be converted to an error, as
> this is a state that can't be the result of a prior operation.
> 
> Take the opportunity and also correct the order of argument checks:
> We shouldn't accept zero-length elements with unknown bits set in "op".
> Also constify cache_flush()'s first parameter.
> 
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3208,7 +3208,7 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_P
>      return 0;
>  }
>  
> -static int cache_flush(gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
> +static int cache_flush(const gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
>  {
>      struct domain *d, *owner;
>      struct page_info *page;
> @@ -3218,19 +3218,17 @@ static int cache_flush(gnttab_cache_flus
>  
>      if ( (cflush->offset >= PAGE_SIZE) ||
>           (cflush->length > PAGE_SIZE) ||
> -         (cflush->offset + cflush->length > PAGE_SIZE) )
> +         (cflush->offset + cflush->length > PAGE_SIZE) ||
> +         (cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN)) )
>          return -EINVAL;
>  
>      if ( cflush->length == 0 || cflush->op == 0 )
> -        return 0;
> +        return !*cur_ref ? 0 : -EILSEQ;
>  
>      /* currently unimplemented */
>      if ( cflush->op & GNTTAB_CACHE_SOURCE_GREF )
>          return -EOPNOTSUPP;
>  
> -    if ( cflush->op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) )
> -        return -EINVAL;
> -
>      d = rcu_lock_current_domain();
>      mfn = cflush->a.dev_bus_addr >> PAGE_SHIFT;
>  
> @@ -3310,6 +3308,9 @@ gnttab_cache_flush(XEN_GUEST_HANDLE_PARA
>          *cur_ref = 0;
>          guest_handle_add_offset(uop, 1);
>      }
> +
> +    *cur_ref = 0;
> +
>      return 0;
>  }
>  
> 
> 
> 

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

Re: [PATCH 2/2] gnttab: improve GNTTABOP_cache_flush locking

[Stefano Stabellini]

On Thu, 30 Nov 2017, Jan Beulich wrote:
> Dropping the lock before returning from grant_map_exists() means handing
> possibly stale information back to the caller. Return back the pointer
> to the active entry instead, for the caller to release the lock once
> done.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -786,10 +786,10 @@ static int _set_status(unsigned gt_versi
>          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
>  }
>  
> -static int grant_map_exists(const struct domain *ld,
> -                            struct grant_table *rgt,
> -                            unsigned long mfn,
> -                            grant_ref_t *cur_ref)
> +static struct active_grant_entry *grant_map_exists(const struct domain *ld,
> +                                                   struct grant_table *rgt,
> +                                                   unsigned long mfn,
> +                                                   grant_ref_t *cur_ref)
>  {
>      grant_ref_t ref, max_iter;
>  
> @@ -805,28 +805,20 @@ static int grant_map_exists(const struct
>                     nr_grant_entries(rgt));
>      for ( ref = *cur_ref; ref < max_iter; ref++ )
>      {
> -        struct active_grant_entry *act;
> -        bool_t exists;
> -
> -        act = active_entry_acquire(rgt, ref);
> -
> -        exists = act->pin
> -            && act->domid == ld->domain_id
> -            && act->frame == mfn;
> +        struct active_grant_entry *act = active_entry_acquire(rgt, ref);
>  
> +        if ( act->pin && act->domid == ld->domain_id && act->frame == mfn )
> +            return act;
>          active_entry_release(act);
> -
> -        if ( exists )
> -            return 0;
>      }
>  
>      if ( ref < nr_grant_entries(rgt) )
>      {
>          *cur_ref = ref;
> -        return 1;
> +        return NULL;
>      }
>  
> -    return -EINVAL;
> +    return ERR_PTR(-EINVAL);
>  }
>  
>  #define MAPKIND_READ 1
> @@ -3213,6 +3205,7 @@ static int cache_flush(const gnttab_cach
>      struct domain *d, *owner;
>      struct page_info *page;
>      unsigned long mfn;
> +    struct active_grant_entry *act = NULL;
>      void *v;
>      int ret;
>  
> @@ -3250,13 +3243,13 @@ static int cache_flush(const gnttab_cach
>      {
>          grant_read_lock(owner->grant_table);
>  
> -        ret = grant_map_exists(d, owner->grant_table, mfn, cur_ref);
> -        if ( ret != 0 )
> +        act = grant_map_exists(d, owner->grant_table, mfn, cur_ref);
> +        if ( IS_ERR_OR_NULL(act) )
>          {
>              grant_read_unlock(owner->grant_table);
>              rcu_unlock_domain(d);
>              put_page(page);
> -            return ret;
> +            return act ? PTR_ERR(act) : 1;
>          }
>      }
>  
> @@ -3273,7 +3266,11 @@ static int cache_flush(const gnttab_cach
>          ret = 0;
>  
>      if ( d != owner )
> +    {
> +        active_entry_release(act);
>          grant_read_unlock(owner->grant_table);
> +    }
> +
>      unmap_domain_page(v);
>      put_page(page);
>  
> 
> 
> 

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

Re: [PATCH 1/2] gnttab: correct GNTTABOP_cache_flush empty batch handling

[Jan Beulich]

>>> On 01.12.17 at 16:31, <andre.przywara@linaro.org> wrote:
> On 30/11/17 14:31, Jan Beulich wrote:
>> Jann validly points out that with a caller bogusly requesting a zero-
>> element batch with non-zero high command bits (the ones used for
>> continuation encoding), the assertion right before the call to
>> hypercall_create_continuation() would trigger. A similar situation would
>> arise afaict for non-empty batches with op and/or length zero in every
>> element.
>> 
>> While we want the former to succeed (as we do elsewhere for similar
>> no-op requests), the latter can clearly be converted to an error, as
>> this is a state that can't be the result of a prior operation.
>> 
>> Take the opportunity and also correct the order of argument checks:
>> We shouldn't accept zero-length elements with unknown bits set in "op".
>> Also constify cache_flush()'s first parameter.
>> 
>> Reported-by: Jann Horn <jannh@google.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Took me a while to wrap my head around it, because the actual fix is
> just the "*cur_ref = 0;" line, I think.
> But this looks correct to me.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

I guess this was meant to be Reviewed-by?

Jan


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

Re: [PATCH 2/2] gnttab: improve GNTTABOP_cache_flush locking

[Jan Beulich]

>>> On 01.12.17 at 16:31, <andre.przywara@linaro.org> wrote:
> On 30/11/17 14:32, Jan Beulich wrote:
>> Dropping the lock before returning from grant_map_exists() means handing
>> possibly stale information back to the caller. Return back the pointer
>> to the active entry instead, for the caller to release the lock once
>> done.
> 
> I don't know enough about grant tables to reason about the deeper
> meaning of this patch, but at least I can confirm that the amended
> locking scheme seems to be correct (now).
> I just wonder if it's worthwhile to add a comment that the function
> takes a lock, but leaves it up to the caller to drop it. Since there is
> only one caller, this might be overkill, though.

Well, the function returning an active entry pointer is imo
sufficient documentation of that fact.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andre Przywara <andre.przywara@linaro.org>

Thanks, Jan


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

Re: [PATCH 1/2] gnttab: correct GNTTABOP_cache_flush empty batch handling

[Jan Beulich]

>>> On 01.12.17 at 22:38, <sstabellini@kernel.org> wrote:
> On Thu, 30 Nov 2017, Jan Beulich wrote:
>> Jann validly points out that with a caller bogusly requesting a zero-
>> element batch with non-zero high command bits (the ones used for
>> continuation encoding), the assertion right before the call to
>> hypercall_create_continuation() would trigger. A similar situation would
>> arise afaict for non-empty batches with op and/or length zero in every
>> element.
>> 
>> While we want the former to succeed (as we do elsewhere for similar
>> no-op requests), the latter can clearly be converted to an error, as
>> this is a state that can't be the result of a prior operation.
>> 
>> Take the opportunity and also correct the order of argument checks:
>> We shouldn't accept zero-length elements with unknown bits set in "op".
>> Also constify cache_flush()'s first parameter.
>> 
>> Reported-by: Jann Horn <jannh@google.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks. Since this and the other patch mainly affect ARM, I'd like
to have your opinion please regarding their backporting.

Jan


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

Re: [PATCH 1/2] gnttab: correct GNTTABOP_cache_flush empty batch handling

[George Dunlap]

On 11/30/2017 02:31 PM, Jan Beulich wrote:
> Jann validly points out that with a caller bogusly requesting a zero-
> element batch with non-zero high command bits (the ones used for
> continuation encoding), the assertion right before the call to
> hypercall_create_continuation() would trigger. A similar situation would
> arise afaict for non-empty batches with op and/or length zero in every
> element.
> 
> While we want the former to succeed (as we do elsewhere for similar
> no-op requests), the latter can clearly be converted to an error, as
> this is a state that can't be the result of a prior operation.
> 
> Take the opportunity and also correct the order of argument checks:
> We shouldn't accept zero-length elements with unknown bits set in "op".
> Also constify cache_flush()'s first parameter.
> 
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3208,7 +3208,7 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_P
>      return 0;
>  }
>  
> -static int cache_flush(gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
> +static int cache_flush(const gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
>  {
>      struct domain *d, *owner;
>      struct page_info *page;
> @@ -3218,19 +3218,17 @@ static int cache_flush(gnttab_cache_flus
>  
>      if ( (cflush->offset >= PAGE_SIZE) ||
>           (cflush->length > PAGE_SIZE) ||
> -         (cflush->offset + cflush->length > PAGE_SIZE) )
> +         (cflush->offset + cflush->length > PAGE_SIZE) ||
> +         (cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN)) )
>          return -EINVAL;
>  
>      if ( cflush->length == 0 || cflush->op == 0 )
> -        return 0;
> +        return !*cur_ref ? 0 : -EILSEQ;

Sorry -- after spending 10 minutes looking through this code I still
have no idea what this is about.  That would indicate it needs some sort
of comment; or at very least a changelog entry that describes the
mechanism as well as the intended outcome.

-George

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

Re: [PATCH 1/2] gnttab: correct GNTTABOP_cache_flush empty batch handling

[Jan Beulich]

>>> On 04.12.17 at 12:12, <george.dunlap@citrix.com> wrote:
> On 11/30/2017 02:31 PM, Jan Beulich wrote:
>> Jann validly points out that with a caller bogusly requesting a zero-
>> element batch with non-zero high command bits (the ones used for
>> continuation encoding), the assertion right before the call to
>> hypercall_create_continuation() would trigger. A similar situation would
>> arise afaict for non-empty batches with op and/or length zero in every
>> element.
>> 
>> While we want the former to succeed (as we do elsewhere for similar
>> no-op requests), the latter can clearly be converted to an error, as
>> this is a state that can't be the result of a prior operation.

Does the latter part of this ...

>> Take the opportunity and also correct the order of argument checks:
>> We shouldn't accept zero-length elements with unknown bits set in "op".
>> Also constify cache_flush()'s first parameter.
>> 
>> Reported-by: Jann Horn <jannh@google.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -3208,7 +3208,7 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_P
>>      return 0;
>>  }
>>  
>> -static int cache_flush(gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
>> +static int cache_flush(const gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
>>  {
>>      struct domain *d, *owner;
>>      struct page_info *page;
>> @@ -3218,19 +3218,17 @@ static int cache_flush(gnttab_cache_flus
>>  
>>      if ( (cflush->offset >= PAGE_SIZE) ||
>>           (cflush->length > PAGE_SIZE) ||
>> -         (cflush->offset + cflush->length > PAGE_SIZE) )
>> +         (cflush->offset + cflush->length > PAGE_SIZE) ||
>> +         (cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN)) )
>>          return -EINVAL;
>>  
>>      if ( cflush->length == 0 || cflush->op == 0 )
>> -        return 0;
>> +        return !*cur_ref ? 0 : -EILSEQ;
> 
> Sorry -- after spending 10 minutes looking through this code I still
> have no idea what this is about.  That would indicate it needs some sort
> of comment; or at very least a changelog entry that describes the
> mechanism as well as the intended outcome.

... really not sufficiently describe the change to the last line above?

Jan


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

Re: [PATCH 2/2] gnttab: improve GNTTABOP_cache_flush locking

[George Dunlap]

On 12/04/2017 09:02 AM, Jan Beulich wrote:
>>>> On 01.12.17 at 16:31, <andre.przywara@linaro.org> wrote:
>> On 30/11/17 14:32, Jan Beulich wrote:
>>> Dropping the lock before returning from grant_map_exists() means handing
>>> possibly stale information back to the caller. Return back the pointer
>>> to the active entry instead, for the caller to release the lock once
>>> done.
>>
>> I don't know enough about grant tables to reason about the deeper
>> meaning of this patch, but at least I can confirm that the amended
>> locking scheme seems to be correct (now).
>> I just wonder if it's worthwhile to add a comment that the function
>> takes a lock, but leaves it up to the caller to drop it. Since there is
>> only one caller, this might be overkill, though.
> 
> Well, the function returning an active entry pointer is imo
> sufficient documentation of that fact.

I agree with this in principle.  But it still seems like function name
doesn't describe what the function does anymore.  What about renaming it
to "grant_map_find_entry()" or something?

 -George

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

Re: [PATCH 2/2] gnttab: improve GNTTABOP_cache_flush locking

[Jan Beulich]

>>> On 04.12.17 at 12:31, <george.dunlap@citrix.com> wrote:
> On 12/04/2017 09:02 AM, Jan Beulich wrote:
>>>>> On 01.12.17 at 16:31, <andre.przywara@linaro.org> wrote:
>>> On 30/11/17 14:32, Jan Beulich wrote:
>>>> Dropping the lock before returning from grant_map_exists() means handing
>>>> possibly stale information back to the caller. Return back the pointer
>>>> to the active entry instead, for the caller to release the lock once
>>>> done.
>>>
>>> I don't know enough about grant tables to reason about the deeper
>>> meaning of this patch, but at least I can confirm that the amended
>>> locking scheme seems to be correct (now).
>>> I just wonder if it's worthwhile to add a comment that the function
>>> takes a lock, but leaves it up to the caller to drop it. Since there is
>>> only one caller, this might be overkill, though.
>> 
>> Well, the function returning an active entry pointer is imo
>> sufficient documentation of that fact.
> 
> I agree with this in principle.  But it still seems like function name
> doesn't describe what the function does anymore.  What about renaming it
> to "grant_map_find_entry()" or something?

I don't mind, feel free to do so (the patch has gone in already with
Stefano's ack).

Jan


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

Re: [PATCH 1/2] gnttab: correct GNTTABOP_cache_flush empty batch handling

[Andre Przywara]

Hi,

On 04/12/17 09:00, Jan Beulich wrote:
>>>> On 01.12.17 at 16:31, <andre.przywara@linaro.org> wrote:
>> On 30/11/17 14:31, Jan Beulich wrote:
>>> Jann validly points out that with a caller bogusly requesting a zero-
>>> element batch with non-zero high command bits (the ones used for
>>> continuation encoding), the assertion right before the call to
>>> hypercall_create_continuation() would trigger. A similar situation would
>>> arise afaict for non-empty batches with op and/or length zero in every
>>> element.
>>>
>>> While we want the former to succeed (as we do elsewhere for similar
>>> no-op requests), the latter can clearly be converted to an error, as
>>> this is a state that can't be the result of a prior operation.
>>>
>>> Take the opportunity and also correct the order of argument checks:
>>> We shouldn't accept zero-length elements with unknown bits set in "op".
>>> Also constify cache_flush()'s first parameter.
>>>
>>> Reported-by: Jann Horn <jannh@google.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Took me a while to wrap my head around it, because the actual fix is
>> just the "*cur_ref = 0;" line, I think.
>> But this looks correct to me.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> 
> I guess this was meant to be Reviewed-by?

Sure, seems my mind was still smoking from chasing all possible call
chains ;-)

Cheers,
Andre.

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

Re: [PATCH 1/2] gnttab: correct GNTTABOP_cache_flush empty batch handling

[Stefano Stabellini]

On Mon, 4 Dec 2017, Jan Beulich wrote:
> >>> On 01.12.17 at 22:38, <sstabellini@kernel.org> wrote:
> > On Thu, 30 Nov 2017, Jan Beulich wrote:
> >> Jann validly points out that with a caller bogusly requesting a zero-
> >> element batch with non-zero high command bits (the ones used for
> >> continuation encoding), the assertion right before the call to
> >> hypercall_create_continuation() would trigger. A similar situation would
> >> arise afaict for non-empty batches with op and/or length zero in every
> >> element.
> >> 
> >> While we want the former to succeed (as we do elsewhere for similar
> >> no-op requests), the latter can clearly be converted to an error, as
> >> this is a state that can't be the result of a prior operation.
> >> 
> >> Take the opportunity and also correct the order of argument checks:
> >> We shouldn't accept zero-length elements with unknown bits set in "op".
> >> Also constify cache_flush()'s first parameter.
> >> 
> >> Reported-by: Jann Horn <jannh@google.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Thanks. Since this and the other patch mainly affect ARM, I'd like
> to have your opinion please regarding their backporting.

Yes, I think they could be backported.

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