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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel