IB/uverbs: Safely extend existing attributes

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@mellanox.com>
Signed-off-by: Matan Barak <matanb@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>

diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 82a1775ba657d27b89a8061040ff0b8828c7acda..1e6bf2488584bc12b8e1bf1f451f1f373a2b015e 100644 (file)
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -35,6 +35,17 @@
 #include "rdma_core.h"
 #include "uverbs.h"
 
+static bool uverbs_is_attr_cleared(const struct ib_uverbs_attr *uattr,
+                                  u16 len)
+{
+       if (uattr->len > sizeof(((struct ib_uverbs_attr *)0)->data))
+               return ib_is_buffer_cleared(u64_to_user_ptr(uattr->data) + len,
+                                           uattr->len - len);
+
+       return !memchr_inv((const void *)&uattr->data + len,
+                          0, uattr->len - len);
+}
+
 static int uverbs_process_attr(struct ib_device *ibdev,
                               struct ib_ucontext *ucontext,
                               const struct ib_uverbs_attr *uattr,
@@ -68,9 +79,20 @@ static int uverbs_process_attr(struct ib_device *ibdev,
 
        switch (spec->type) {
        case UVERBS_ATTR_TYPE_PTR_IN:
+               /* 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 &&
+                   spec->flags & UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO &&
+                   !uverbs_is_attr_cleared(uattr, spec->ptr.len))
+                       return -EOPNOTSUPP;
+
+       /* fall through */
        case UVERBS_ATTR_TYPE_PTR_OUT:
-               if (uattr->len < spec->ptr.len ||
-                   (!(spec->flags & UVERBS_ATTR_SPEC_F_MIN_SZ) &&
+               if (uattr->len < spec->ptr.min_len ||
+                   (!(spec->flags & UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO) &&
                     uattr->len > spec->ptr.len))
                        return -EINVAL;
 

diff --git a/drivers/infiniband/core/uverbs_ioctl_merge.c b/drivers/infiniband/core/uverbs_ioctl_merge.c
index 62e1eb1d2a28ad1bd854699c62a1da6149443286..0f88a1919d51b0ddfdd087ca4007d4891e1188be 100644 (file)
--- a/drivers/infiniband/core/uverbs_ioctl_merge.c
+++ b/drivers/infiniband/core/uverbs_ioctl_merge.c
@@ -379,7 +379,7 @@ static struct uverbs_method_spec *build_method_with_attrs(const struct uverbs_me
                                 "ib_uverbs: Tried to merge attr (%d) but it's an object with new/destroy access but isn't mandatory\n",
                                 min_id) ||
                            WARN(IS_ATTR_OBJECT(attr) &&
-                                attr->flags & UVERBS_ATTR_SPEC_F_MIN_SZ,
+                                attr->flags & UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO,
                                 "ib_uverbs: Tried to merge attr (%d) but it's an object with min_sz flag\n",
                                 min_id)) {
                                res = -EINVAL;

diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index e4a4b184a6bc11fdbab58711b35a9ae8d30d3697..0a2d8532de212017f1994e732d8714be41531255 100644 (file)
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -216,9 +216,11 @@ static int uverbs_hot_unplug_completion_event_file(struct ib_uobject_file *uobj_
  * spec.
  */
 static const struct uverbs_attr_def uverbs_uhw_compat_in =
-       UVERBS_ATTR_PTR_IN_SZ(UVERBS_ATTR_UHW_IN, 0, UA_FLAGS(UVERBS_ATTR_SPEC_F_MIN_SZ));
+       UVERBS_ATTR_PTR_IN_SZ(UVERBS_ATTR_UHW_IN, UVERBS_ATTR_SIZE(0, USHRT_MAX),
+                             UA_FLAGS(UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO));
 static const struct uverbs_attr_def uverbs_uhw_compat_out =
-       UVERBS_ATTR_PTR_OUT_SZ(UVERBS_ATTR_UHW_OUT, 0, UA_FLAGS(UVERBS_ATTR_SPEC_F_MIN_SZ));
+       UVERBS_ATTR_PTR_OUT_SZ(UVERBS_ATTR_UHW_OUT, UVERBS_ATTR_SIZE(0, USHRT_MAX),
+                              UA_FLAGS(UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO));
 
 static void create_udata(struct uverbs_attr_bundle *ctx,
                         struct ib_udata *udata)
@@ -350,17 +352,19 @@ static DECLARE_UVERBS_NAMED_METHOD(UVERBS_METHOD_CQ_CREATE,
        &UVERBS_ATTR_IDR(UVERBS_ATTR_CREATE_CQ_HANDLE, UVERBS_OBJECT_CQ,
                         UVERBS_ACCESS_NEW,
                         UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
-       &UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_CQE, u32,
+       &UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_CQE,
+                           UVERBS_ATTR_TYPE(u32),
                            UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
-       &UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_USER_HANDLE, u64,
+       &UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_USER_HANDLE,
+                           UVERBS_ATTR_TYPE(u64),
                            UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
        &UVERBS_ATTR_FD(UVERBS_ATTR_CREATE_CQ_COMP_CHANNEL,
                        UVERBS_OBJECT_COMP_CHANNEL,
                        UVERBS_ACCESS_READ),
-       &UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_COMP_VECTOR, u32,
+       &UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_COMP_VECTOR, UVERBS_ATTR_TYPE(u32),
                            UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
-       &UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_FLAGS, u32),
-       &UVERBS_ATTR_PTR_OUT(UVERBS_ATTR_CREATE_CQ_RESP_CQE, u32,
+       &UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_FLAGS, UVERBS_ATTR_TYPE(u32)),
+       &UVERBS_ATTR_PTR_OUT(UVERBS_ATTR_CREATE_CQ_RESP_CQE, UVERBS_ATTR_TYPE(u32),
                             UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
        &uverbs_uhw_compat_in, &uverbs_uhw_compat_out);
 
@@ -394,7 +398,7 @@ static DECLARE_UVERBS_NAMED_METHOD(UVERBS_METHOD_CQ_DESTROY,
                         UVERBS_ACCESS_DESTROY,
                         UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
        &UVERBS_ATTR_PTR_OUT(UVERBS_ATTR_DESTROY_CQ_RESP,
-                            struct ib_uverbs_destroy_cq_resp,
+                            UVERBS_ATTR_TYPE(struct ib_uverbs_destroy_cq_resp),
                             UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)));
 
 DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_COMP_CHANNEL,

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 2357f2b296107bb9610c403b8f2a0623a5dfe953..e9288d0f627e5d432437579990b7a9c617f3abe4 100644 (file)
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2446,11 +2446,9 @@ 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;
 }
 
-static inline bool ib_is_udata_cleared(struct ib_udata *udata,
-                                      size_t offset,
-                                      size_t len)
+static inline bool ib_is_buffer_cleared(const void __user *p,
+                                       size_t len)
 {
-       const void __user *p = udata->inbuf + offset;
        bool ret;
        u8 *buf;
 
@@ -2466,6 +2464,13 @@ static inline bool ib_is_udata_cleared(struct ib_udata *udata,
        return ret;
 }
 
+static inline bool ib_is_udata_cleared(struct ib_udata *udata,
+                                      size_t offset,
+                                      size_t len)
+{
+       return ib_is_buffer_cleared(udata->inbuf + offset, len);
+}
+
 /**
  * ib_modify_qp_is_ok - Check that the supplied attribute mask
  * contains all required attributes and no attributes not allowed for

diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index cd7c3e40c6ccb33fe439368a627accc9ad6490e5..c4ee65b20bb7f56e6d4a605becf41ae9b552b15f 100644 (file)
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -62,8 +62,8 @@ enum uverbs_obj_access {
 
 enum {
        UVERBS_ATTR_SPEC_F_MANDATORY    = 1U << 0,
-       /* Support extending attributes by length */
-       UVERBS_ATTR_SPEC_F_MIN_SZ       = 1U << 1,
+       /* Support extending attributes by length, validate all unknown size == zero  */
+       UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO = 1U << 1,
 };
 
 /* Specification of a single attribute inside the ioctl message */
@@ -79,7 +79,10 @@ struct uverbs_attr_spec {
                        enum uverbs_attr_type           type;
                        /* Combination of bits from enum UVERBS_ATTR_SPEC_F_XXXX */
                        u8                              flags;
+                       /* Current known size to kernel */
                        u16                             len;
+                       /* User isn't allowed to provide something < min_len */
+                       u16                             min_len;
                } ptr;
                struct {
                        enum uverbs_attr_type           type;
@@ -177,30 +180,41 @@ struct uverbs_object_tree_def {
 };
 
 #define UA_FLAGS(_flags)  .flags = _flags
-#define __UVERBS_ATTR0(_id, _len, _type, ...)                           \
+#define __UVERBS_ATTR0(_id, _type, _fld, _attr, ...)              \
        ((const struct uverbs_attr_def)                           \
-        {.id = _id, .attr = {{.ptr = {.type = _type, .len = _len, .flags = 0, } }, } })
-#define __UVERBS_ATTR1(_id, _len, _type, _flags)                        \
+        {.id = _id, .attr = {{._fld = {.type = _type, _attr, .flags = 0, } }, } })
+#define __UVERBS_ATTR1(_id, _type, _fld, _attr, _extra1, ...)      \
        ((const struct uverbs_attr_def)                           \
-        {.id = _id, .attr = {{.ptr = {.type = _type, .len = _len, _flags } },} })
-#define __UVERBS_ATTR(_id, _len, _type, _flags, _n, ...)               \
-       __UVERBS_ATTR##_n(_id, _len, _type, _flags)
+        {.id = _id, .attr = {{._fld = {.type = _type, _attr, _extra1 } },} })
+#define __UVERBS_ATTR2(_id, _type, _fld, _attr, _extra1, _extra2)    \
+       ((const struct uverbs_attr_def)                           \
+        {.id = _id, .attr = {{._fld = {.type = _type, _attr, _extra1, _extra2 } },} })
+#define __UVERBS_ATTR(_id, _type, _fld, _attr, _extra1, _extra2, _n, ...)      \
+       __UVERBS_ATTR##_n(_id, _type, _fld, _attr, _extra1, _extra2)
+
+#define UVERBS_ATTR_TYPE(_type)                                        \
+       .min_len = sizeof(_type), .len = sizeof(_type)
+#define UVERBS_ATTR_STRUCT(_type, _last)                       \
+       .min_len = ((uintptr_t)(&((_type *)0)->_last + 1)), .len = sizeof(_type)
+#define UVERBS_ATTR_SIZE(_min_len, _len)                       \
+       .min_len = _min_len, .len = _len
+
 /*
  * In new compiler, UVERBS_ATTR could be simplified by declaring it as
  * [_id] = {.type = _type, .len = _len, ##__VA_ARGS__}
  * But since we support older compilers too, we need the more complex code.
  */
-#define UVERBS_ATTR(_id, _len, _type, ...)                             \
-       __UVERBS_ATTR(_id, _len, _type, ##__VA_ARGS__, 1, 0)
+#define UVERBS_ATTR(_id, _type, _fld, _attr, ...)                      \
+       __UVERBS_ATTR(_id, _type, _fld, _attr, ##__VA_ARGS__, 2, 1, 0)
 #define UVERBS_ATTR_PTR_IN_SZ(_id, _len, ...)                          \
-       UVERBS_ATTR(_id, _len, UVERBS_ATTR_TYPE_PTR_IN, ##__VA_ARGS__)
+       UVERBS_ATTR(_id, UVERBS_ATTR_TYPE_PTR_IN, ptr, _len, ##__VA_ARGS__)
 /* If sizeof(_type) <= sizeof(u64), this will be inlined rather than a pointer */
 #define UVERBS_ATTR_PTR_IN(_id, _type, ...)                            \
-       UVERBS_ATTR_PTR_IN_SZ(_id, sizeof(_type), ##__VA_ARGS__)
+       UVERBS_ATTR_PTR_IN_SZ(_id, _type, ##__VA_ARGS__)
 #define UVERBS_ATTR_PTR_OUT_SZ(_id, _len, ...)                         \
-       UVERBS_ATTR(_id, _len, UVERBS_ATTR_TYPE_PTR_OUT, ##__VA_ARGS__)
+       UVERBS_ATTR(_id, UVERBS_ATTR_TYPE_PTR_OUT, ptr, _len, ##__VA_ARGS__)
 #define UVERBS_ATTR_PTR_OUT(_id, _type, ...)                           \
-       UVERBS_ATTR_PTR_OUT_SZ(_id, sizeof(_type), ##__VA_ARGS__)
+       UVERBS_ATTR_PTR_OUT_SZ(_id, _type, ##__VA_ARGS__)
 
 /*
  * In new compiler, UVERBS_ATTR_IDR (and FD) could be simplified by declaring
@@ -396,8 +410,8 @@ static inline int _uverbs_copy_from(void *to,
 
        /*
         * Validation ensures attr->ptr_attr.len >= size. If the caller is
-        * using UVERBS_ATTR_SPEC_F_MIN_SZ then it must call copy_from with
-        * the right size.
+        * using UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO then it must call
+        * uverbs_copy_from_or_zero.
         */
        if (unlikely(size < attr->ptr_attr.len))
                return -EINVAL;
@@ -411,9 +425,37 @@ static inline int _uverbs_copy_from(void *to,
        return 0;
 }
 
+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;
+
+       if (size > min_size)
+               memset(to + min_size, 0, size - min_size);
+
+       return 0;
+}
+
 #define uverbs_copy_from(to, attrs_bundle, idx)                                      \
        _uverbs_copy_from(to, attrs_bundle, idx, sizeof(*to))
 
+#define uverbs_copy_from_or_zero(to, attrs_bundle, idx)                              \
+       _uverbs_copy_from_or_zero(to, attrs_bundle, idx, sizeof(*to))
+
 /* =================================================
  *      Definitions -> Specs infrastructure
  * =================================================