Re: [PATCH v3 06/17] IB/core: Add support for extended query device caps

[Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

On 17/12/2014 16:00, Yann Droneaud wrote:
> Hi,
> 
> Le mercredi 17 décembre 2014 à 08:54 +0200, Haggai Eran a écrit :
>> On 16/12/2014 14:33, Yann Droneaud wrote:
>>> Le jeudi 11 décembre 2014 à 17:04 +0200, Haggai Eran a écrit :
>>>>  static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, size_t len)
>>>>  {
>>>> -	return copy_to_user(udata->outbuf, src, len) ? -EFAULT : 0;
>>>> +	size_t copy_sz;
>>>> +
>>>> +	copy_sz = min_t(size_t, len, udata->outlen);
>>>> +	return copy_to_user(udata->outbuf, src, copy_sz) ? -EFAULT : 0;
>>>>  }
>>>
>>>
>>> This is not the place to do this: as I'm guessing the purpose of this 
>>> change from the patch in '[PATCH v3 07/17] IB/core: Add flags for on 
>>> demand paging support', you're trying to handle uverbs call from 
>>> a userspace program using a previous, shorter ABI.
>>
>> Yes, that was my intention.
>>
>>>
>>> But that's hidding bug where userspace will get it wrong at passing the 
>>> correct buffer / size for all others uverb calls.
>>>
>>> That cannot work that way.
>>>
>>> In a previous patchset [1], I've suggested to add a check in 
>>> ib_copy_{from,to}_udata()[2][3] in order to check the input/output
>>> buffer size to not read/write past userspace provided buffer
>>> boundaries: in case of mismatch an error would be returned to
>>> userspace.
>>>
>>> With the suggested change here, buffer overflow won't happen,
>>> but the error is silently ignored, allowing uverb to return a
>>> partial result, which is likely not expected by userspace as
>>> it's a bit difficult to handle it gracefully.
>>>
>>> So this has to be removed, and a check on userspace response
>>> buffer must be added to ib_uverbs_ex_query_device() instead.
>>
>> I agree that we shouldn't silently ignore bugs in userspace, but I'm not
>> sure the alternative is maintainable. If we have in the future N new
>> extensions to this verb, will we need to validate the user space given
>> output buffer is one of the N possible sizes?
>>
> 
> Yes.

It would very easy for someone to forget one of the possible sizes, and
thus blocking support for an older version of libibverbs. Such a bug
would be hard to detect because it requires testing all previous
versions of libibverbs. I think the problem you are trying to solve -
userspace accidentally setting a smaller response size then required -
will be detected immediately when one attempts to use that code.

> 
> Additionnaly the size should be checked related to the flags set in the
> "comp_mask": eg. requiring IB_USER_VERBS_EX_QUERY_DEVICE_ODP but not
> providing the expected response buffer should be an error.

In a query verb like EX_QUERY_DEVICE, I would expect the user-space code
to request all bits in the comp_mask, since there's very little benefit
from requesting a specific set (only a slightly shorter response for the
system call). The kernel would ignore bits it doesn't know, and the
user-space would ignore bits it doesn't know in the response.

Regards,
Haggai
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html