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.
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..
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
