On Wed, Mar 14, 2018 at 04:59:18PM +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 | 15 +++++-
> 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, 90 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
> index 82a1775ba657..2aa54dfe4757 100644
> +++ b/drivers/infiniband/core/uverbs_ioctl.c
> @@ -68,9 +68,20 @@ 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)) ||
> + (uattr->len <= sizeof(((struct ib_uverbs_attr *)0)->data) &&
> + memchr_inv((const void *)&uattr->data + spec->ptr.len,
> + 0, uattr->len - spec->ptr.len))))
> + return -EOPNOTSUPP;
Wow, can we write that crazy if more clearly please? :(
bool uverbs_is_attr_cleared(uattr, size)
{
if (uattr->len <= sizeof(((struct ib_uverbs_attr *)0)->data))
return memchr_inv((const void *)&uattr->data + size,
0, uattr->len - size) == NULL;
return ib_is_buffer_cleared(u64_to_user_ptr(uattr->data) + size, uattr->len - size);
}
[..]
if (spec->flags & UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO) {
/*
* Ensure that any data provided by userspace beyond the known
* struct is zero. Userspace that knows how to use some future
* longer struct will fail here if used with an old kernel and
* non-zero content, making ABI compat/discovery simpler.
*/
if (uattr->len > spec->ptr.len)
if (!uverbs_is_attr_cleared(uattr, spec->ptr.len))
return -EOPNOTSUPP;
} else
if (uattr->len != spec->ptr.len)
return -EOPNOTSUPP;
> +static inline int _uverbs_copy_from_or_zero(void *to,
> + const struct uverbs_attr_bundle *attrs_bundle,
> + size_t idx,
> + size_t size)
> +{
> + const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
> + size_t min_size;
> +
> + if (IS_ERR(attr))
> + return PTR_ERR(attr);
> +
> + min_size = min_t(size_t, size, attr->ptr_attr.len);
> +
> + if (uverbs_attr_ptr_is_inline(attr))
> + memcpy(to, &attr->ptr_attr.data, min_size);
> + else if (copy_from_user(to, u64_to_user_ptr(attr->ptr_attr.data),
> + min_size))
> + return -EFAULT;
> +
> + return 0;
> +}
So this function is called 'copy_from_or_zero', am I blind? Where is
the 'or_zero' bit?
I expect to see
memset(to + min_size, 0, size - min_size);
yes?
Jason
--
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
