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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

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@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

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@xxxxxxxxxx>

> --- 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel