On Thu, Mar 8, 2018 at 8:10 PM, Jason Gunthorpe <jgg@xxxxxxxxxxxx> wrote:
> On Thu, Mar 08, 2018 at 07:19:28PM +0200, Leon Romanovsky wrote:
>> From: Matan Barak <matanb@xxxxxxxxxxxx>
>>
>> Previously, we've used UVERBS_ATTR_SPEC_F_MIN_SZ for extending existing
>> attributes. The behavior of this flag was the kernel accepts anything
>> bigger than the minimum size it specified. This is unsafe, since in
>> order to safely extend an attribute, we need to make sure unknown size
>> is zeroed. Replacing UVERBS_ATTR_SPEC_F_MIN_SZ with
>> UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO, which essentially checks that the
>> unknown size is zero. In addition, attributes are now decorated with
>> UVERBS_ATTR_TYPE and UVERBS_ATTR_STRUCT, so we can provide the minimum
>> and known length.
>>
>> Users of this flag needs to use copy_from_or_zero functions/macros.
>>
>> Reviewed-by: Yishai Hadas <yishaih@xxxxxxxxxxxx>
>> Signed-off-by: Matan Barak <matanb@xxxxxxxxxxxx>
>> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>> drivers/infiniband/core/uverbs_ioctl.c | 12 ++++-
>> drivers/infiniband/core/uverbs_ioctl_merge.c | 2 +-
>> drivers/infiniband/core/uverbs_std_types.c | 20 ++++----
>> include/rdma/ib_verbs.h | 13 +++--
>> include/rdma/uverbs_ioctl.h | 71 +++++++++++++++++++++-------
>> 5 files changed, 87 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
>> index 82a1775ba657..76828fd7fdba 100644
>> +++ b/drivers/infiniband/core/uverbs_ioctl.c
>> @@ -68,9 +68,17 @@ static int uverbs_process_attr(struct ib_device *ibdev,
>>
>> switch (spec->type) {
>> case UVERBS_ATTR_TYPE_PTR_IN:
>> + if (uattr->len > spec->ptr.len &&
>> + spec->flags & UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO &&
>> + uattr->len > sizeof(((struct ib_uverbs_attr *)0)->data) &&
>> + !ib_is_buffer_cleared(u64_to_user_ptr(uattr->data) + spec->ptr.len,
>> + uattr->len - spec->ptr.len))
>> + return -EOPNOTSUPP;
>
> This doesn't look right..
>
> For instance if we started out with a 4 byte field then it will be
> inlined in data, and if we expand to 8 bytes it will still be inlined,
> but no check for trailing 0 will be done on the inlined data.
>
Yeah, this is correct.
This currently only allows you to extend size < sizeof(u64) [inline]
to a pointer.
I'll add the non zero checks to the inline path.
> Also, are we concerned about the race here? Another user space thread
> can be changing the data after we check for 0? I think that is OK
> since this check is just an ABI compat feature, not related to
> correctness? But lets be certain of that and add a comment..
>
Sure, this is only a compat feature. The uverbs_copy_from flavored
functions only copies min(user_given_data_size, kernel_known_size)
bytes to the kernel.
Therefore, even if another thread garbled this memory, it won't leak
into the kernel.
> Jason
Matan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
