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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.12.17 at 12:12, <george.dunlap@xxxxxxxxxx> 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@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.

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

Jan


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