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