>-----Original Message----- >From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- >owner@xxxxxxxxxxxxxxx] On Behalf Of Leon Romanovsky >Sent: Thursday, May 3, 2018 9:37 AM >To: Doug Ledford <dledford@xxxxxxxxxx>; Jason Gunthorpe ><jgg@xxxxxxxxxxxx> >Cc: Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux- >rdma@xxxxxxxxxxxxxxx>; Matan Barak <matanb@xxxxxxxxxxxx>; Yishai >Hadas <yishaih@xxxxxxxxxxxx> >Subject: [PATCH rdma-next 05/21] IB/uverbs: Add validations in attr_get and >copy_{to,from} functions > >From: Matan Barak <matanb@xxxxxxxxxxxx> > >Add validations to make sure the kernel developers used the correct >functions. We'll WARN_ON if [s]he doesn't. > >Signed-off-by: Matan Barak <matanb@xxxxxxxxxxxx> >Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> >--- > drivers/infiniband/core/uverbs_ioctl.c | 1 + > include/rdma/uverbs_ioctl.h | 66 >++++++++++++++++++++++++++++++++++ > 2 files changed, 67 insertions(+) > >diff --git a/drivers/infiniband/core/uverbs_ioctl.c >b/drivers/infiniband/core/uverbs_ioctl.c >index 8c93970dc8f1..1b030ccbde78 100644 >--- a/drivers/infiniband/core/uverbs_ioctl.c >+++ b/drivers/infiniband/core/uverbs_ioctl.c >@@ -255,6 +255,7 @@ static int uverbs_handle_method(struct >ib_uverbs_attr __user *uattr_ptr, > if (num_given_buckets <= 0) > return -EINVAL; > >+ attr_bundle->method = method_spec; > attr_bundle->num_buckets = num_given_buckets; > ret = uverbs_validate_kernel_mandatory(method_spec, >attr_bundle); > if (ret) >diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h >index 86b3a780bbef..64b5f336cace 100644 >--- a/include/rdma/uverbs_ioctl.h >+++ b/include/rdma/uverbs_ioctl.h >@@ -362,6 +362,7 @@ struct uverbs_attr_bundle_hash { > }; > > struct uverbs_attr_bundle { >+ const struct uverbs_method_spec *method; > size_t num_buckets; > struct uverbs_attr_bundle_hash hash[]; > }; >@@ -426,14 +427,28 @@ static inline const struct uverbs_attr >*uverbs_attr_get(const struct uverbs_attr > return &attrs_bundle->hash[idx_bucket].attrs[idx & >~UVERBS_ID_NS_MASK]; > } > >+#define uverbs_validate_spec_debug(_method, _idx, _name, _cond) ({\ Since it appears that these macros will be used all the time, is calling them _debug appropriate? >+ const struct uverbs_attr_spec *_name; \ >+ \ >+ _name = uverbs_get_spec(_method, idx); \ >+ WARN_ON(IS_ERR(_name)) ? PTR_ERR(_name) : \ >+ WARN_ON(!(_cond)) ? -EINVAL : 0; \ >+}) >+ > static inline int uverbs_attr_get_enum_id(const struct uverbs_attr_bundle >*attrs_bundle, > u16 idx) > { > const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx); >+ int ret; > > if (IS_ERR(attr)) > return PTR_ERR(attr); > >+ ret = uverbs_validate_spec_debug(attrs_bundle->method, idx, spec, >+ spec->type == >UVERBS_ATTR_TYPE_ENUM_IN); >+ if (ret) >+ return ret; >+ > return attr->ptr_attr.enum_id; > } > >@@ -449,16 +464,53 @@ static inline void *uverbs_attr_get_obj(const struct >uverbs_attr_bundle *attrs_b > return uobj->object; > } > >+static inline const struct uverbs_attr_spec * >+uverbs_get_actual_spec(const struct uverbs_attr_bundle *attrs_bundle, >u16 idx) >+{ >+ const struct uverbs_attr_spec *spec = >+ uverbs_get_spec(attrs_bundle->method, idx); >+ int enum_id; >+ >+ if (IS_ERR(spec)) >+ return spec; >+ >+ if (likely(spec->type != UVERBS_ATTR_TYPE_ENUM_IN)) >+ return spec; >+ >+ enum_id = uverbs_attr_get_enum_id(attrs_bundle, idx); >+ if (enum_id < 0) >+ return ERR_PTR(enum_id); >+ >+ /* We already know that enum_id is correct, because we didn't fail at >+ * the parsing stage. >+ */ >+ return &spec->enum_def.ids[enum_id]; >+} >+ >+#define uverbs_validate_actual_spec_debug(_method, _idx, _name, >_cond) ({\ So we have validate_spec(), and validate_actual_spec(). And debug macros of each. I assume that they are doing two different things. Are they? Maybe comes comments or a different naming convention would be better? Mike >+ const struct uverbs_attr_spec *_name; \ >+ \ >+ _name = uverbs_get_actual_spec(_method, idx); \ >+ WARN_ON(IS_ERR(_name)) ? PTR_ERR(_name) : \ >+ WARN_ON(!(_cond)) ? -EINVAL : 0; \ >+}) >+ > static inline int uverbs_copy_to(const struct uverbs_attr_bundle >*attrs_bundle, > size_t idx, const void *from, size_t size) > { > const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx); > u16 flags; > size_t min_size; >+ int ret; > > if (IS_ERR(attr)) > return PTR_ERR(attr); > >+ ret = uverbs_validate_actual_spec_debug(attrs_bundle, idx, spec, >+ spec->type == >UVERBS_ATTR_TYPE_PTR_OUT); >+ if (ret) >+ return ret; >+ > min_size = min_t(size_t, attr->ptr_attr.len, size); > if (copy_to_user(u64_to_user_ptr(attr->ptr_attr.data), from, >min_size)) > return -EFAULT; >@@ -481,10 +533,17 @@ static inline int _uverbs_copy_from(void *to, > size_t size) > { > const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx); >+ int ret; > > if (IS_ERR(attr)) > return PTR_ERR(attr); > >+ ret = uverbs_validate_actual_spec_debug(attrs_bundle, idx, spec, >+ spec->type == >UVERBS_ATTR_TYPE_PTR_IN && >+ spec->flags & >~UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO); >+ if (ret) >+ return ret; >+ > /* > * Validation ensures attr->ptr_attr.len >= size. If the caller is > * using UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO then it must call >@@ -509,10 +568,17 @@ static inline int _uverbs_copy_from_or_zero(void >*to, > { > const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx); > size_t min_size; >+ int ret; > > if (IS_ERR(attr)) > return PTR_ERR(attr); > >+ ret = uverbs_validate_actual_spec_debug(attrs_bundle, idx, spec, >+ spec->type == >UVERBS_ATTR_TYPE_PTR_IN && >+ spec->flags & >UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO); >+ if (ret) >+ return ret; >+ > min_size = min_t(size_t, size, attr->ptr_attr.len); > > if (uverbs_attr_ptr_is_inline(attr)) >-- >2.14.3 > >-- >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
