[PATCHv4 for-3.13 00/10] create_flow/destroy_flow fixes for v3.13

[PATCHv4 for-3.13 00/10] create_flow/destroy_flow fixes for v3.13

[Yann Droneaud]

Hi Roland,

Please find the fourth revision of a patchset against create_flow/destroy_flow
and associated extended command scheme.

These are fixes that *must* be applied before making the new uverbs
widely available in v3.13:

- patches 1, 2 and 3 address a warning reported by sparse.
- patch 4 handle an uncommon type of extended command.
- patch 5, 6 and 7 ensure that commands will be extensible:
  - one patch add a missing check of comp_mask;
  - the other patches add checks on reserved fields.
- patch 8 fix an error path which was not setting the error code.
- patch 9 ensure the response buffer of an extended command
  is a valid memory region for userspace. Doing the check earlier,
  on the *full* response buffer, might be safer.
- patch 10 prevent out of bound access and underflow.

Please review, test and apply for v3.13.

Changes from patchset v3 [1]:

- add a patch to hold 'response' pointer to remove the redundant cast
  introduced by patch to remove implicit cast from INIT_UDATA()
- rework some commit messages to address review made by Roland.

Changes from patchset v2 [2]:

- add a patch to check the input length in extended uverbs,
  create_flow and destroy_flow.

Changes from patchset v1 [3]:

- add patch to verify early response buffer using access_ok(),
  just like vfs_read().

Regards.

[1] [PATCHv3 for-3.13 0/9] create_flow/destroy_flow fixes for v3.13
    http://marc.info/?i=cover.1386798254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

[2] [PATCH v2 for v3.13 0/8] create_flow/destroy_flow fixes for v3.13
    http://marc.info/?i=cover.1385981934.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

[3] [PATCH for v3.13 0/7] create_flow/destroy_flow fixes for v3.13
    http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org


Yann Droneaud (10):
  IB/uverbs: move cast from u64 to void __user pointer to it's own
    variable
  IB/core: const'ify inbuf in struct ib_udata
  IB/uverbs: remove implicit cast in INIT_UDATA()
  IB/uverbs: set outbuf to NULL when no core response space is provided
  IB/uverbs: check reserved field in extended command header
  IB/uverbs: check comp_mask in destroy_flow
  IB/uverbs: check reserved fields in create_flow
  IB/uverbs: set error code when fail to consume all flow_spec items
  IB/uverbs: check access to userspace response buffer in extended
    command
  IB/uverbs: check input length in flow steering uverbs

 drivers/infiniband/core/uverbs.h      |  12 +--
 drivers/infiniband/core/uverbs_cmd.c  | 175 +++++++++++++++++++++++-----------
 drivers/infiniband/core/uverbs_main.c |  21 +++-
 include/rdma/ib_verbs.h               |   2 +-
 4 files changed, 144 insertions(+), 66 deletions(-)

-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 01/10] IB/uverbs: move cast from u64 to void __user pointer to it's own variable

[Yann Droneaud]

Use a dedicated variable to hold address of the response buffer after
'conversion' from u64 to void __user *, so that this value could be used for
INIT_UDATA() and copy_to_user(), reducing the visual clutter introduced by
the cast.

This variable will be used when implicit cast will be removed from
INIT_UDATA() macro, which is required in order to remove a sparse warning.

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 158 +++++++++++++++++++++++------------
 1 file changed, 104 insertions(+), 54 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 65f6e7dc380c..971e16c970b9 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -285,6 +285,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_get_context      cmd;
 	struct ib_uverbs_get_context_resp resp;
+	char __user			 *response;
 	struct ib_udata                   udata;
 	struct ib_device                 *ibdev = file->device->ib_dev;
 	struct ib_ucontext		 *ucontext;
@@ -297,6 +298,8 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	mutex_lock(&file->mutex);
 
 	if (file->ucontext) {
@@ -305,7 +308,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 	}
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	ucontext = ibdev->alloc_ucontext(ibdev, &udata);
@@ -339,8 +342,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 		goto err_fd;
 	}
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_file;
 	}
@@ -383,6 +385,7 @@ ssize_t ib_uverbs_query_device(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_query_device      cmd;
 	struct ib_uverbs_query_device_resp resp;
+	char __user			  *response;
 	struct ib_device_attr              attr;
 	int                                ret;
 
@@ -392,6 +395,8 @@ ssize_t ib_uverbs_query_device(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	ret = ib_query_device(file->device->ib_dev, &attr);
 	if (ret)
 		return ret;
@@ -439,8 +444,7 @@ ssize_t ib_uverbs_query_device(struct ib_uverbs_file *file,
 	resp.local_ca_ack_delay        = attr.local_ca_ack_delay;
 	resp.phys_port_cnt	       = file->device->ib_dev->phys_port_cnt;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		return -EFAULT;
 
 	return in_len;
@@ -452,6 +456,7 @@ ssize_t ib_uverbs_query_port(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_query_port      cmd;
 	struct ib_uverbs_query_port_resp resp;
+	char __user			*response;
 	struct ib_port_attr              attr;
 	int                              ret;
 
@@ -461,6 +466,8 @@ ssize_t ib_uverbs_query_port(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	ret = ib_query_port(file->device->ib_dev, cmd.port_num, &attr);
 	if (ret)
 		return ret;
@@ -489,8 +496,7 @@ ssize_t ib_uverbs_query_port(struct ib_uverbs_file *file,
 	resp.link_layer      = rdma_port_get_link_layer(file->device->ib_dev,
 							cmd.port_num);
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		return -EFAULT;
 
 	return in_len;
@@ -502,6 +508,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_alloc_pd      cmd;
 	struct ib_uverbs_alloc_pd_resp resp;
+	char __user		      *response;
 	struct ib_udata                udata;
 	struct ib_uobject             *uobj;
 	struct ib_pd                  *pd;
@@ -513,8 +520,10 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	uobj = kmalloc(sizeof *uobj, GFP_KERNEL);
@@ -543,8 +552,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,
 	memset(&resp, 0, sizeof resp);
 	resp.pd_handle = uobj->id;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_copy;
 	}
@@ -696,6 +704,7 @@ ssize_t ib_uverbs_open_xrcd(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_open_xrcd	cmd;
 	struct ib_uverbs_open_xrcd_resp	resp;
+	char __user		       *response;
 	struct ib_udata			udata;
 	struct ib_uxrcd_object         *obj;
 	struct ib_xrcd                 *xrcd = NULL;
@@ -710,8 +719,10 @@ ssize_t ib_uverbs_open_xrcd(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof  resp);
 
 	mutex_lock(&file->device->xrcd_tree_mutex);
@@ -783,8 +794,7 @@ ssize_t ib_uverbs_open_xrcd(struct ib_uverbs_file *file,
 		atomic_inc(&xrcd->usecnt);
 	}
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_copy;
 	}
@@ -910,6 +920,7 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_reg_mr      cmd;
 	struct ib_uverbs_reg_mr_resp resp;
+	char __user		    *response;
 	struct ib_udata              udata;
 	struct ib_uobject           *uobj;
 	struct ib_pd                *pd;
@@ -922,8 +933,10 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	if ((cmd.start & ~PAGE_MASK) != (cmd.hca_va & ~PAGE_MASK))
@@ -969,8 +982,7 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file,
 	resp.rkey      = mr->rkey;
 	resp.mr_handle = uobj->id;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_copy;
 	}
@@ -1045,6 +1057,7 @@ ssize_t ib_uverbs_alloc_mw(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_alloc_mw      cmd;
 	struct ib_uverbs_alloc_mw_resp resp;
+	char __user		      *response;
 	struct ib_uobject             *uobj;
 	struct ib_pd                  *pd;
 	struct ib_mw                  *mw;
@@ -1056,6 +1069,8 @@ ssize_t ib_uverbs_alloc_mw(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof(cmd)))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	uobj = kmalloc(sizeof(*uobj), GFP_KERNEL);
 	if (!uobj)
 		return -ENOMEM;
@@ -1089,8 +1104,7 @@ ssize_t ib_uverbs_alloc_mw(struct ib_uverbs_file *file,
 	resp.rkey      = mw->rkey;
 	resp.mw_handle = uobj->id;
 
-	if (copy_to_user((void __user *)(unsigned long)cmd.response,
-			 &resp, sizeof(resp))) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_copy;
 	}
@@ -1165,6 +1179,7 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_create_comp_channel	   cmd;
 	struct ib_uverbs_create_comp_channel_resp  resp;
+	char __user				  *response;
 	struct file				  *filp;
 	int ret;
 
@@ -1174,6 +1189,8 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	ret = get_unused_fd_flags(O_CLOEXEC);
 	if (ret < 0)
 		return ret;
@@ -1185,8 +1202,7 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file,
 		return PTR_ERR(filp);
 	}
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		put_unused_fd(resp.fd);
 		fput(filp);
 		return -EFAULT;
@@ -1202,6 +1218,7 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_create_cq      cmd;
 	struct ib_uverbs_create_cq_resp resp;
+	char __user		       *response;
 	struct ib_udata                 udata;
 	struct ib_ucq_object           *obj;
 	struct ib_uverbs_event_file    *ev_file = NULL;
@@ -1214,8 +1231,10 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	if (cmd.comp_vector >= file->device->num_comp_vectors)
@@ -1266,8 +1285,7 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,
 	resp.cq_handle = obj->uobject.id;
 	resp.cqe       = cq->cqe;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_copy;
 	}
@@ -1303,6 +1321,7 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_resize_cq	cmd;
 	struct ib_uverbs_resize_cq_resp	resp;
+	char __user		       *response;
 	struct ib_udata                 udata;
 	struct ib_cq			*cq;
 	int				ret = -EINVAL;
@@ -1310,8 +1329,10 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	cq = idr_read_cq(cmd.cq_handle, file->ucontext, 0);
@@ -1324,8 +1345,7 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file,
 
 	resp.cqe = cq->cqe;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp.cqe))
+	if (copy_to_user(response, &resp, sizeof(resp.cqe)))
 		ret = -EFAULT;
 
 out:
@@ -1439,6 +1459,7 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_destroy_cq      cmd;
 	struct ib_uverbs_destroy_cq_resp resp;
+	char __user			*response;
 	struct ib_uobject		*uobj;
 	struct ib_cq               	*cq;
 	struct ib_ucq_object        	*obj;
@@ -1448,6 +1469,8 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	uobj = idr_write_uobj(&ib_uverbs_cq_idr, cmd.cq_handle, file->ucontext);
 	if (!uobj)
 		return -EINVAL;
@@ -1478,8 +1501,7 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
 
 	put_uobj(uobj);
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		return -EFAULT;
 
 	return in_len;
@@ -1491,6 +1513,7 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_create_qp      cmd;
 	struct ib_uverbs_create_qp_resp resp;
+	char __user		       *response;
 	struct ib_udata                 udata;
 	struct ib_uqp_object           *obj;
 	struct ib_device	       *device;
@@ -1512,8 +1535,10 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 	if (cmd.qp_type == IB_QPT_RAW_PACKET && !capable(CAP_NET_RAW))
 		return -EPERM;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	obj = kzalloc(sizeof *obj, GFP_KERNEL);
@@ -1626,8 +1651,7 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 	resp.max_send_wr     = attr.cap.max_send_wr;
 	resp.max_inline_data = attr.cap.max_inline_data;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_copy;
 	}
@@ -1685,6 +1709,7 @@ ssize_t ib_uverbs_open_qp(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_open_qp        cmd;
 	struct ib_uverbs_create_qp_resp resp;
+	char __user		       *response;
 	struct ib_udata                 udata;
 	struct ib_uqp_object           *obj;
 	struct ib_xrcd		       *xrcd;
@@ -1699,8 +1724,10 @@ ssize_t ib_uverbs_open_qp(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	obj = kmalloc(sizeof *obj, GFP_KERNEL);
@@ -1742,8 +1769,7 @@ ssize_t ib_uverbs_open_qp(struct ib_uverbs_file *file,
 	resp.qpn       = qp->qp_num;
 	resp.qp_handle = obj->uevent.uobject.id;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_remove;
 	}
@@ -1780,6 +1806,7 @@ ssize_t ib_uverbs_query_qp(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_query_qp      cmd;
 	struct ib_uverbs_query_qp_resp resp;
+	char __user		       *response;
 	struct ib_qp                   *qp;
 	struct ib_qp_attr              *attr;
 	struct ib_qp_init_attr         *init_attr;
@@ -1788,6 +1815,8 @@ ssize_t ib_uverbs_query_qp(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	attr      = kmalloc(sizeof *attr, GFP_KERNEL);
 	init_attr = kmalloc(sizeof *init_attr, GFP_KERNEL);
 	if (!attr || !init_attr) {
@@ -1863,8 +1892,7 @@ ssize_t ib_uverbs_query_qp(struct ib_uverbs_file *file,
 	resp.max_inline_data        = init_attr->cap.max_inline_data;
 	resp.sq_sig_all             = init_attr->sq_sig_type == IB_SIGNAL_ALL_WR;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		ret = -EFAULT;
 
 out:
@@ -1986,6 +2014,7 @@ ssize_t ib_uverbs_destroy_qp(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_destroy_qp      cmd;
 	struct ib_uverbs_destroy_qp_resp resp;
+	char __user			*response;
 	struct ib_uobject		*uobj;
 	struct ib_qp               	*qp;
 	struct ib_uqp_object        	*obj;
@@ -1994,6 +2023,8 @@ ssize_t ib_uverbs_destroy_qp(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	memset(&resp, 0, sizeof resp);
 
 	uobj = idr_write_uobj(&ib_uverbs_qp_idr, cmd.qp_handle, file->ucontext);
@@ -2031,8 +2062,7 @@ ssize_t ib_uverbs_destroy_qp(struct ib_uverbs_file *file,
 
 	put_uobj(uobj);
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		return -EFAULT;
 
 	return in_len;
@@ -2044,6 +2074,7 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_post_send      cmd;
 	struct ib_uverbs_post_send_resp resp;
+	char __user		       *response;
 	struct ib_uverbs_send_wr       *user_wr;
 	struct ib_send_wr              *wr = NULL, *last, *next, *bad_wr;
 	struct ib_qp                   *qp;
@@ -2061,6 +2092,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 	if (cmd.wqe_size < sizeof (struct ib_uverbs_send_wr))
 		return -EINVAL;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	user_wr = kmalloc(cmd.wqe_size, GFP_KERNEL);
 	if (!user_wr)
 		return -ENOMEM;
@@ -2176,8 +2209,7 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 				break;
 		}
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		ret = -EFAULT;
 
 out_put:
@@ -2288,6 +2320,7 @@ ssize_t ib_uverbs_post_recv(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_post_recv      cmd;
 	struct ib_uverbs_post_recv_resp resp;
+	char __user		       *response;
 	struct ib_recv_wr              *wr, *next, *bad_wr;
 	struct ib_qp                   *qp;
 	ssize_t                         ret = -EINVAL;
@@ -2295,6 +2328,8 @@ ssize_t ib_uverbs_post_recv(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	wr = ib_uverbs_unmarshall_recv(buf + sizeof cmd,
 				       in_len - sizeof cmd, cmd.wr_count,
 				       cmd.sge_count, cmd.wqe_size);
@@ -2317,8 +2352,7 @@ ssize_t ib_uverbs_post_recv(struct ib_uverbs_file *file,
 				break;
 		}
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		ret = -EFAULT;
 
 out:
@@ -2337,6 +2371,7 @@ ssize_t ib_uverbs_post_srq_recv(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_post_srq_recv      cmd;
 	struct ib_uverbs_post_srq_recv_resp resp;
+	char __user			   *response;
 	struct ib_recv_wr                  *wr, *next, *bad_wr;
 	struct ib_srq                      *srq;
 	ssize_t                             ret = -EINVAL;
@@ -2344,6 +2379,8 @@ ssize_t ib_uverbs_post_srq_recv(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	wr = ib_uverbs_unmarshall_recv(buf + sizeof cmd,
 				       in_len - sizeof cmd, cmd.wr_count,
 				       cmd.sge_count, cmd.wqe_size);
@@ -2366,8 +2403,7 @@ ssize_t ib_uverbs_post_srq_recv(struct ib_uverbs_file *file,
 				break;
 		}
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		ret = -EFAULT;
 
 out:
@@ -2386,6 +2422,7 @@ ssize_t ib_uverbs_create_ah(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_create_ah	 cmd;
 	struct ib_uverbs_create_ah_resp	 resp;
+	char __user			*response;
 	struct ib_uobject		*uobj;
 	struct ib_pd			*pd;
 	struct ib_ah			*ah;
@@ -2398,6 +2435,8 @@ ssize_t ib_uverbs_create_ah(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	uobj = kmalloc(sizeof *uobj, GFP_KERNEL);
 	if (!uobj)
 		return -ENOMEM;
@@ -2438,8 +2477,7 @@ ssize_t ib_uverbs_create_ah(struct ib_uverbs_file *file,
 
 	resp.ah_handle = uobj->id;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_copy;
 	}
@@ -2823,6 +2861,7 @@ static int __uverbs_create_xsrq(struct ib_uverbs_file *file,
 				struct ib_udata *udata)
 {
 	struct ib_uverbs_create_srq_resp resp;
+	char __user			*response;
 	struct ib_usrq_object           *obj;
 	struct ib_pd                    *pd;
 	struct ib_srq                   *srq;
@@ -2830,6 +2869,8 @@ static int __uverbs_create_xsrq(struct ib_uverbs_file *file,
 	struct ib_srq_init_attr          attr;
 	int ret;
 
+	response = (void __user *)(unsigned long)cmd->response;
+
 	obj = kmalloc(sizeof *obj, GFP_KERNEL);
 	if (!obj)
 		return -ENOMEM;
@@ -2905,8 +2946,7 @@ static int __uverbs_create_xsrq(struct ib_uverbs_file *file,
 	if (cmd->srq_type == IB_SRQT_XRC)
 		resp.srqn = srq->ext.xrc.srq_num;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd->response,
-			 &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof(resp))) {
 		ret = -EFAULT;
 		goto err_copy;
 	}
@@ -2958,6 +2998,7 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file,
 	struct ib_uverbs_create_srq      cmd;
 	struct ib_uverbs_create_xsrq     xcmd;
 	struct ib_uverbs_create_srq_resp resp;
+	char __user			*response;
 	struct ib_udata                  udata;
 	int ret;
 
@@ -2967,6 +3008,8 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	xcmd.response	 = cmd.response;
 	xcmd.user_handle = cmd.user_handle;
 	xcmd.srq_type	 = IB_SRQT_BASIC;
@@ -2976,7 +3019,7 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file,
 	xcmd.srq_limit	 = cmd.srq_limit;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	ret = __uverbs_create_xsrq(file, &xcmd, &udata);
@@ -2991,6 +3034,7 @@ ssize_t ib_uverbs_create_xsrq(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_create_xsrq     cmd;
 	struct ib_uverbs_create_srq_resp resp;
+	char __user			*response;
 	struct ib_udata                  udata;
 	int ret;
 
@@ -3000,8 +3044,10 @@ ssize_t ib_uverbs_create_xsrq(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	ret = __uverbs_create_xsrq(file, &cmd, &udata);
@@ -3047,6 +3093,7 @@ ssize_t ib_uverbs_query_srq(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_query_srq      cmd;
 	struct ib_uverbs_query_srq_resp resp;
+	char __user		       *response;
 	struct ib_srq_attr              attr;
 	struct ib_srq                   *srq;
 	int                             ret;
@@ -3057,6 +3104,8 @@ ssize_t ib_uverbs_query_srq(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	srq = idr_read_srq(cmd.srq_handle, file->ucontext);
 	if (!srq)
 		return -EINVAL;
@@ -3074,8 +3123,7 @@ ssize_t ib_uverbs_query_srq(struct ib_uverbs_file *file,
 	resp.max_sge   = attr.max_sge;
 	resp.srq_limit = attr.srq_limit;
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		return -EFAULT;
 
 	return in_len;
@@ -3087,6 +3135,7 @@ ssize_t ib_uverbs_destroy_srq(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_destroy_srq      cmd;
 	struct ib_uverbs_destroy_srq_resp resp;
+	char __user			 *response;
 	struct ib_uobject		 *uobj;
 	struct ib_srq               	 *srq;
 	struct ib_uevent_object        	 *obj;
@@ -3097,6 +3146,8 @@ ssize_t ib_uverbs_destroy_srq(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	uobj = idr_write_uobj(&ib_uverbs_srq_idr, cmd.srq_handle, file->ucontext);
 	if (!uobj)
 		return -EINVAL;
@@ -3131,8 +3182,7 @@ ssize_t ib_uverbs_destroy_srq(struct ib_uverbs_file *file,
 
 	put_uobj(uobj);
 
-	if (copy_to_user((void __user *) (unsigned long) cmd.response,
-			 &resp, sizeof resp))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		ret = -EFAULT;
 
 	return ret ? ret : in_len;
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 02/10] IB/core: const'ify inbuf in struct ib_udata

[Yann Droneaud]

Userspace input buffer is not modified by kernel, let it be 'const'.

It's a prerequisite to remove the implicit cast from INIT_UDATA(),
which is needed to remove a sparse warning.

Without this change on struct ib_udata, compiler will complain
when input buffer (const) will be stored in inbuf (non-const).

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs.h | 2 +-
 include/rdma/ib_verbs.h          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index bdc842e9faef..9879568aed8c 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -49,7 +49,7 @@
 
 #define INIT_UDATA(udata, ibuf, obuf, ilen, olen)			\
 	do {								\
-		(udata)->inbuf  = (void __user *) (ibuf);		\
+		(udata)->inbuf  = (const void __user *) (ibuf);		\
 		(udata)->outbuf = (void __user *) (obuf);		\
 		(udata)->inlen  = (ilen);				\
 		(udata)->outlen = (olen);				\
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 979874c627ee..61e1935c91b1 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -978,7 +978,7 @@ struct ib_uobject {
 };
 
 struct ib_udata {
-	void __user *inbuf;
+	const void __user *inbuf;
 	void __user *outbuf;
 	size_t       inlen;
 	size_t       outlen;
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 03/10] IB/uverbs: remove implicit cast in INIT_UDATA()

[Yann Droneaud]

Currently, INIT_UDATA() does an implicit cast to a pointer,
so that 'response' address, eg. output buffer, can be used
as is to initialize a struct ib_udata:

        do {                                                    \
                (udata)->inbuf  = (void __user *) (ibuf);       \
                (udata)->outbuf = (void __user *) (obuf);       \
                (udata)->inlen  = (ilen);                       \
                (udata)->outlen = (olen);                       \
        } while (0)

...

        INIT_UDATA(&udata, buf + sizeof cmd,
                   (unsigned long) cmd.response + sizeof resp,
                   in_len - sizeof cmd, out_len - sizeof  resp);

...

Unfortunately, sparse reports an error if literal 0 is used
to initialize inbuf or outbuf, for example in:

        INIT_UDATA(&ucore,
                   (hdr.in_words) ? buf : 0,
                   (unsigned long)ex_hdr.response,
                   hdr.in_words * 8,
                   hdr.out_words * 8);

It was reported by kbuild test robot in message[1]:

  From: kbuild test robot <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  Subject: "drivers/infiniband/core/uverbs_main.c:683:17:
      sparse: Using plain integer as NULL pointer",
  Message-Id: <528b3984.SVGs20ZWpcuR/Jls%fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Replacing 0 by NULL in ib_uverbs_write() might turn off sparse warning,
but it trigger a GCC warning:

drivers/infiniband/core/uverbs_main.c: In function 'ib_uverbs_write':
drivers/infiniband/core/uverbs_main.c:685:200: warning: pointer/integer type mismatch in conditional expression [enabled by default]

So fixing the sparse warning require broad changes in uverbs.

By removing the implicit cast INIT_UDATA(), this patch fixes the warnings
reported by sparse and allows the compiler to report a warning in case a
plain integer get used to initialize an udata pointer.

This patch requires
- calls to INIT_UDATA() to be modified to pass a pointer instead of an
  unsigned long,
- struct ib_udata to be modified to have a const void __user *inbuf field[2],
  otherwise compiler will report warnings regarding const to non const
  conversion:

drivers/infiniband/core/uverbs_main.c: In function ‘ib_uverbs_write’:
drivers/infiniband/core/uverbs_main.c:682:24: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
drivers/infiniband/core/uverbs_main.c:688:22: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_get_context’:
drivers/infiniband/core/uverbs_cmd.c:307:23: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_alloc_pd’:
drivers/infiniband/core/uverbs_cmd.c:516:23: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
...

[1] https://lists.01.org/pipermail/kbuild-all/2013-November/002120.html

[2] https://patchwork.kernel.org/patch/2846202/
    http://marc.info/?i=3050a98379b4342ea59d59aeaf1ce162171df928.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs.h      | 12 ++++++------
 drivers/infiniband/core/uverbs_main.c | 13 ++++++++-----
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 9879568aed8c..0dca1975d59d 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -47,12 +47,12 @@
 #include <rdma/ib_umem.h>
 #include <rdma/ib_user_verbs.h>
 
-#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)			\
-	do {								\
-		(udata)->inbuf  = (const void __user *) (ibuf);		\
-		(udata)->outbuf = (void __user *) (obuf);		\
-		(udata)->inlen  = (ilen);				\
-		(udata)->outlen = (olen);				\
+#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)	\
+	do {						\
+		(udata)->inbuf  = (ibuf);		\
+		(udata)->outbuf = (obuf);		\
+		(udata)->inlen  = (ilen);		\
+		(udata)->outlen = (olen);		\
 	} while (0)
 
 /*
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 34386943ebcf..14d864371050 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -635,6 +635,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		__u32 command;
 
 		struct ib_uverbs_ex_cmd_hdr ex_hdr;
+		char __user *response;
 		struct ib_udata ucore;
 		struct ib_udata uhw;
 		int err;
@@ -668,7 +669,9 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count)
 			return -EINVAL;
 
-		if (ex_hdr.response) {
+		response = (char __user *)(unsigned long)ex_hdr.response;
+
+		if (response) {
 			if (!hdr.out_words && !ex_hdr.provider_out_words)
 				return -EINVAL;
 		} else {
@@ -677,14 +680,14 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		}
 
 		INIT_UDATA(&ucore,
-			   (hdr.in_words) ? buf : 0,
-			   (unsigned long)ex_hdr.response,
+			   (hdr.in_words) ? buf : NULL,
+			   response,
 			   hdr.in_words * 8,
 			   hdr.out_words * 8);
 
 		INIT_UDATA(&uhw,
-			   (ex_hdr.provider_in_words) ? buf + ucore.inlen : 0,
-			   (ex_hdr.provider_out_words) ? (unsigned long)ex_hdr.response + ucore.outlen : 0,
+			   (ex_hdr.provider_in_words) ? buf + ucore.inlen : NULL,
+			   (ex_hdr.provider_out_words) ? response + ucore.outlen : NULL,
 			   ex_hdr.provider_in_words * 8,
 			   ex_hdr.provider_out_words * 8);
 
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 04/10] IB/uverbs: set outbuf to NULL when no core response space is provided

[Yann Droneaud]

In the currently uncommon case of core (eg. uverbs) response
space being omitted, but hw (eg. provider) response space being
available, outbuf get defined to "response" while it must be NULL.

This patch takes care of setting ucore->outbuf to NULL
if hdr.out_words is equal to 0.

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 14d864371050..6c4fc6338b26 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -681,7 +681,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 
 		INIT_UDATA(&ucore,
 			   (hdr.in_words) ? buf : NULL,
-			   response,
+			   (hdr.out_words) ? response : NULL,
 			   hdr.in_words * 8,
 			   hdr.out_words * 8);
 
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 05/10] IB/uverbs: check reserved field in extended command header

[Yann Droneaud]

As noted by Daniel Vetter in its article "Botching up ioctls"[1]

  "Check *all* unused fields and flags and all the padding for
   whether it's 0, and reject the ioctl if that's not the case.
   Otherwise your nice plan for future extensions is going right
   down the gutters since someone *will* submit an ioctl struct
   with random stack garbage in the yet unused parts. Which then
   bakes in the ABI that those fields can never be used for
   anything else but garbage."

It's important to ensure that reserved fields are set to known
value, so that it will be possible to use them latter to extend
the ABI.

The same reasonning apply to comp_mask field present in newer
uverbs command: per commit 22878dbc9173, unsupported values in
comp_mask are rejected.

[1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 6c4fc6338b26..8652c13f6ea2 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -669,6 +669,9 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count)
 			return -EINVAL;
 
+		if (ex_hdr.cmd_hdr_reserved)
+			return -EINVAL;
+
 		response = (char __user *)(unsigned long)ex_hdr.response;
 
 		if (response) {
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 06/10] IB/uverbs: check comp_mask in destroy_flow

[Yann Droneaud]

Just like the check added to create_flow in 22878dbc9173,
comp_mask must be checked in destroy_flow too.

Since only empty comp_mask is currently supported,
any other value must be rejected.

This check was silently added in a previous patch[1] to move
comp_mask in extended command header, part of previous patchset[2]
against create/destroy_flow uverbs. The idea of moving comp_mask
to the header was discarded for the final patchset[3].

Unfortunately the check added in destroy_flow uverb was not
integrated in the final patchset.

[1] http://marc.info/?i=40175eda10d670d098204da6aa4c327a0171ae5f.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[2] http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[3] http://marc.info/?i=cover.1383773832.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 971e16c970b9..a23874e959a1 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2833,6 +2833,9 @@ int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
 	if (ret)
 		return ret;
 
+	if (cmd.comp_mask)
+		return -EINVAL;
+
 	uobj = idr_write_uobj(&ib_uverbs_rule_idr, cmd.flow_handle,
 			      file->ucontext);
 	if (!uobj)
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 07/10] IB/uverbs: check reserved fields in create_flow

[Yann Droneaud]

As noted by Daniel Vetter in its article "Botching up ioctls"[1]

  "Check *all* unused fields and flags and all the padding for
   whether it's 0, and reject the ioctl if that's not the case.
   Otherwise your nice plan for future extensions is going right
   down the gutters since someone *will* submit an ioctl struct
   with random stack garbage in the yet unused parts. Which then
   bakes in the ABI that those fields can never be used for
   anything else but garbage."

It's important to ensure that reserved fields are set to known
value, so that it will be possible to use them latter to extend
the ABI.

The same reasonning apply to comp_mask field present in newer
uverbs command: per commit 22878dbc9173, unsupported values in
comp_mask are rejected.

[1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index a23874e959a1..f50f1f3dc509 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2631,6 +2631,9 @@ out_put:
 static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
 				union ib_flow_spec *ib_spec)
 {
+	if (kern_spec->reserved)
+		return -EINVAL;
+
 	ib_spec->type = kern_spec->type;
 
 	switch (ib_spec->type) {
@@ -2709,6 +2712,10 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
 		return -EINVAL;
 
+	if (cmd.flow_attr.reserved[0] ||
+	    cmd.flow_attr.reserved[1])
+		return -EINVAL;
+
 	if (cmd.flow_attr.num_of_specs) {
 		kern_flow_attr = kmalloc(sizeof(*kern_flow_attr) + cmd.flow_attr.size,
 					 GFP_KERNEL);
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 08/10] IB/uverbs: set error code when fail to consume all flow_spec items

[Yann Droneaud]

If the flow_spec items parsed count does not match the number
of items declared in the flow_attr command, or if not all
bytes are used for flow_spec items (eg. trailing garbage),
a log message is reported and the function leave through
the error path. Unfortunately the error code is currently
not set.

This patch set error code to -EINVAL in such cases, so
that the error is reported to userspace instead of silently
fail.

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index f50f1f3dc509..1288a7ce254a 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2776,6 +2776,7 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {
 		pr_warn("create flow failed, flow %d: %d bytes left from uverb cmd\n",
 			i, cmd.flow_attr.size);
+		err = -EINVAL;
 		goto err_free;
 	}
 	flow_id = ib_create_flow(qp, flow_attr, IB_FLOW_DOMAIN_USER);
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 09/10] IB/uverbs: check access to userspace response buffer in extended command

[Yann Droneaud]

This patch adds a check on the output buffer with access_ok(VERIFY_WRITE,
...) to ensure the whole buffer is in userspace memory before using the
pointer in uverbs functions. If the buffer or a subset of it is not valid,
returns -EFAULT to the caller.

This will also catch invalid buffer before the final call to copy_to_user()
which happen late in most uverb functions.

Just like the check in read(2) syscall, it's a sanity check to detect
invalid parameters provided by userspace. This particular check was added
in vfs_read() by Linus Torvalds for v2.6.12 with following commit message:

https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/?id=fd770e66c9a65b14ce114e171266cf6f393df502

  Make read/write always do the full "access_ok()" tests.

  The actual user copy will do them too, but only for the
  range that ends up being actually copied. That hides
  bugs when the range has been clamped by file size or other
  issues.

Note: there's no need to check input buffer since vfs_write() already does
access_ok(VERIFY_READ, ...) as part of write() syscall.

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 8652c13f6ea2..0be1dd86f768 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -677,6 +677,11 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		if (response) {
 			if (!hdr.out_words && !ex_hdr.provider_out_words)
 				return -EINVAL;
+
+			if (!access_ok(VERIFY_WRITE,
+				       response,
+				       (hdr.out_words + ex_hdr.provider_out_words) * 8))
+				return -EFAULT;
 		} else {
 			if (hdr.out_words || ex_hdr.provider_out_words)
 				return -EINVAL;
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 for-3.13 10/10] IB/uverbs: check input length in flow steering uverbs

[Yann Droneaud]

Since ib_copy_from_udata() doesn't check yet the available
input data length before accessing userspace memory,
an explicit check of this length is required to prevent:

- reading past the user provided buffer,
- underflow when subtracting the expected command size
  from the input length.

This will ensure the newly added flow steering uverbs
don't try to process truncated commands.

Link: http://marc.info/?i=cover.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 1288a7ce254a..4a53d3f01dfc 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2687,6 +2687,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	void *ib_spec;
 	int i;
 
+	if (ucore->inlen < sizeof(cmd))
+		return -EINVAL;
+
 	if (ucore->outlen < sizeof(resp))
 		return -ENOSPC;
 
@@ -2837,6 +2840,9 @@ int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
 	struct ib_uobject		*uobj;
 	int				ret;
 
+	if (ucore->inlen < sizeof(cmd))
+		return -EINVAL;
+
 	ret = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
 	if (ret)
 		return ret;
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 for-3.13 00/10] create_flow/destroy_flow fixes for v3.13

[Or Gerlitz]

On Tue, Dec 17, 2013 at 11:58 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> Hi Roland,
>
> Please find the fourth revision of a patchset against create_flow/destroy_flow
> and associated extended command scheme.

Yann, note that V3 is already applied, so you need to change incremental changes
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 for-3.13 00/10] create_flow/destroy_flow fixes for v3.13

[Yann Droneaud]

Hi Or, Roland.

Le mardi 17 décembre 2013 à 12:35 +0200, Or Gerlitz a écrit :
> On Tue, Dec 17, 2013 at 11:58 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> >
> > Please find the fourth revision of a patchset against create_flow/destroy_flow
> > and associated extended command scheme.
> 
> Yann, note that V3 is already applied, so you need to change incremental changes

Thanks for pointing this, Or.

Unfortunately, some patches in V4 were replacement for patches in V3.
In this particular case, I've rework the commit messages in V4, so these
changes might be lost.

In this other hand, V3 is not applied asis in for-next branch of
Roland's tree. In fact Roland seems to have dropped this patch:
[PATCHv3 for-3.13 2/9] IB/uverbs: remove implicit cast in INIT_UDATA()
http://marc.info/?i=a6da021658e9a1cee5cf9f6db61694711cc2852d.1386798254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

This, then, require a conflict fix when applying next patch:
[PATCHv3 for-3.13 3/9] IB/uverbs: set outbuf to NULL when no core
response space is provided
http://marc.info/?i=d4ae6494fb2dc1f8a81e8d8e90d8084b37d15ba2.1386798254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

And another for this later patch:
[PATCHv3 for-3.13 8/9] IB/uverbs: check access to userspace response
 buffer in extended command
http://marc.info/?i=701fc3ee1136bbb4a0fd3d560c3ec1ee3bb3b828.1386798254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

And I'm a bit disappointed about this particular patch applied with a
conflict fix:

https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/commit/?h=for-next&id=2f92baf8313756fd32da4d2a24abc67c8f4f7026

	if (!access_ok(VERIFY_WRITE,
		       (const void __user *) (unsigned long) ex_hdr.response,
		       (hdr.out_words + ex_hdr.provider_out_words) * 8))
		return -EFAULT;

https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/tree/drivers/infiniband/core/uverbs_main.c?h=for-next&id=2f92baf8313756fd32da4d2a24abc67c8f4f7026#n679

Checking for write access on a pointer to const doesn't sound right.
It's harmless, and, if sparse/coccinelle doesn't have a check for such,
it should be added to report a warning.

BTW, I would prefer being responsible of my own mistakes :)

I try to be helpful and open to fix my patchset so that they are easier
to merge. In this particular case, the fix to my commit was not
'agreed': I haven't the chance to review it before being commited since
it wasn't discussed by mail.

So I hope the infiniband/for-next branch could be rewrote (it's a branch
for linux-next, do people care of it being rebased ?) to include my new
commit messages and discard the not so incorrect fix to access_ok()
call. And perhaps include the patch adding a 'response' pointer as
'common pattern' of uverbs function.

Just tell me what you want me to do to ease the integration of my fixes.

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 for-3.13 00/10] create_flow/destroy_flow fixes for v3.13

[Or Gerlitz]

On 19/12/2013 15:21, Yann Droneaud wrote:
> [...] So I hope the infiniband/for-next branch could be rewrote
I am OK with that.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 for-3.13 00/10] create_flow/destroy_flow fixes for v3.13

[Roland Dreier]

> Unfortunately, some patches in V4 were replacement for patches in V3.
> In this particular case, I've rework the commit messages in V4, so these
> changes might be lost.

> And I'm a bit disappointed about this particular patch applied with a
> conflict fix:
>
> https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/commit/?h=for-next&id=2f92baf8313756fd32da4d2a24abc67c8f4f7026
>
>         if (!access_ok(VERIFY_WRITE,
>                        (const void __user *) (unsigned long) ex_hdr.response,
>                        (hdr.out_words + ex_hdr.provider_out_words) * 8))
>                 return -EFAULT;
>
> https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/tree/drivers/infiniband/core/uverbs_main.c?h=for-next&id=2f92baf8313756fd32da4d2a24abc67c8f4f7026#n679
>
> Checking for write access on a pointer to const doesn't sound right.
> It's harmless, and, if sparse/coccinelle doesn't have a check for such,
> it should be added to report a warning.

Sorry for that mistake.  I've fixed that conflict fix to get rid of
the const.  I've also pulled in the changelog updates.

However, I still prefer not moving all the casts out into every place
where we use INIT_UDATA(), so I'd prefer something like the patch
below.  I integrated that into the series and pushed out the result
(since everyone seems OK with rebasing for-next).

Please let me know what you think.

From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
Date: Thu, 19 Dec 2013 08:37:03 -0800
Subject: [PATCH] IB/uverbs: Set pointers to NULL if length is 0 in
 INIT_UDATA()

Trying to have a ternary operator to choose between NULL (or 0) and the
real pointer value in invocations leads to an impossible choice between
a sparse error about a literal 0 used as a NULL pointer, and a gcc
warning about "pointer/integer type mismatch in conditional expression."

Rather than clutter the source with more casts, move the ternary
operator into the INIT_UDATA() macro, which makes it easier to use and
simplifies its callers.

Reported-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/core/uverbs.h      | 12 ++++++------
 drivers/infiniband/core/uverbs_main.c | 11 ++++-------
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 9879568aed8c..d2defbbdb5da 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -47,12 +47,12 @@
 #include <rdma/ib_umem.h>
 #include <rdma/ib_user_verbs.h>

-#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)            \
-    do {                                \
-        (udata)->inbuf  = (const void __user *) (ibuf);        \
-        (udata)->outbuf = (void __user *) (obuf);        \
-        (udata)->inlen  = (ilen);                \
-        (udata)->outlen = (olen);                \
+#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)                \
+    do {                                    \
+        (udata)->inbuf  = (ilen) ? (const void __user *) (ibuf) : NULL;    \
+        (udata)->outbuf = (olen) ? (void __user *) (obuf) : NULL;    \
+        (udata)->inlen  = (ilen);                    \
+        (udata)->outlen = (olen);                    \
     } while (0)

 /*
diff --git a/drivers/infiniband/core/uverbs_main.c
b/drivers/infiniband/core/uverbs_main.c
index 34386943ebcf..76b6f217a13e 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -676,15 +676,12 @@ static ssize_t ib_uverbs_write(struct file
*filp, const char __user *buf,
                 return -EINVAL;
         }

-        INIT_UDATA(&ucore,
-               (hdr.in_words) ? buf : 0,
-               (unsigned long)ex_hdr.response,
-               hdr.in_words * 8,
-               hdr.out_words * 8);
+        INIT_UDATA(&ucore, buf, (unsigned long)ex_hdr.response,
+               hdr.in_words * 8, hdr.out_words * 8);

         INIT_UDATA(&uhw,
-               (ex_hdr.provider_in_words) ? buf + ucore.inlen : 0,
-               (ex_hdr.provider_out_words) ? (unsigned
long)ex_hdr.response + ucore.outlen : 0,
+               buf + ucore.inlen,
+               (unsigned long) ex_hdr.response + ucore.outlen,
                ex_hdr.provider_in_words * 8,
                ex_hdr.provider_out_words * 8);
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 for-3.13 00/10] create_flow/destroy_flow fixes for v3.13

[Yann Droneaud]

Hi Roland,

Le jeudi 19 décembre 2013 à 09:08 -0800, Roland Dreier a écrit :
> > Unfortunately, some patches in V4 were replacement for patches in V3.
> > In this particular case, I've rework the commit messages in V4, so these
> > changes might be lost.
> 
> > And I'm a bit disappointed about this particular patch applied with a
> > conflict fix:
> >
> > https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/commit/?h=for-next&id=2f92baf8313756fd32da4d2a24abc67c8f4f7026
> >
> >         if (!access_ok(VERIFY_WRITE,
> >                        (const void __user *) (unsigned long) ex_hdr.response,
> >                        (hdr.out_words + ex_hdr.provider_out_words) * 8))
> >                 return -EFAULT;
> >
> > https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/tree/drivers/infiniband/core/uverbs_main.c?h=for-next&id=2f92baf8313756fd32da4d2a24abc67c8f4f7026#n679
> >
> > Checking for write access on a pointer to const doesn't sound right.
> > It's harmless, and, if sparse/coccinelle doesn't have a check for such,
> > it should be added to report a warning.
> 
> Sorry for that mistake.  I've fixed that conflict fix to get rid of
> the const.  I've also pulled in the changelog updates.
> 
> However, I still prefer not moving all the casts out into every place
> where we use INIT_UDATA(), so I'd prefer something like the patch
> below.  I integrated that into the series and pushed out the result
> (since everyone seems OK with rebasing for-next).
> 
> Please let me know what you think.
> 

I'm a bit puzzled :) Sure it address the warning problem from sparse and
gcc. It's even handling setting NULL for response if out_words is 0,
which was part of another patch[1] in the patchset. So I would say OK.

But I'm concerned about this global change of behavior of INIT_UDATA().

I felt it was OK to handle the case of out_words equal to 0 in the new
uverbs, since there's no current user of this, there's nothing to broke.
It was a limited, locally change.

But when changing globally the behavor of INIT_UDATA(), what about
existing program which pass a non-NULL address for the response and set
out_words to 0 in a command.

libibverbs doesn't seems to do this, but who knows how one might try to
access to uverbs ?

Since currently ib_copy_to_udata() doesn't check struct ib_udata outlen
(I have patch for that[2]), some provider (hw/) might be using
ib_copy_to_udata() without paying attention to outlen.

If outbuf is set to NULL when outlen is 0, then it might broke, and this
could be called a regression.

I haven't double check each and every user of ib_copy_to_udata(), even
if I've found some ugly hack regarding udata, I'm looking at you
ipath[3] and qib[4], I haven't payed yet attention to user of
ib_copy_to_udata() without check for outlen first.

In this other hand, with my patch[2] from the patchset[5], I've done
some tests, and haven't found any regression, but my testbed was very
limited.

So I think we must double check here before applying a global change to
INIT_UDATA() and/or be ready to pay the price of regression in some
vendor software (I'm not aware of) ...

[1] [PATCHv4 for-3.13 04/10] IB/uverbs: set outbuf to NULL when no core
response space is provided
http://marc.info/?i=330b13a0884b6cc03e287a32f8f49c1aac6bdbed.1387273677.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

[2] [PATCH 04/22] infiniband: ib_copy_to_udata(): check output length
http://marc.info/?i=d27716a3a1c180f832d153a7402f65ea8a75b734.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

[3] ipath_srq.c:254 ipath_modify_srq()
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/ipath/ipath_srq.c?id=v3.13-rc4#n254

[4] qib_srq.c:250 qib_modify_srq()
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/qib/qib_srq.c?id=v3.13-rc4#n250

[5] [PATCH 00/22] infiniband: improve userspace input check
http://marc.info/?i=cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 for-3.13 00/10] create_flow/destroy_flow fixes for v3.13

[Yann Droneaud]

Hi,

Le jeudi 19 décembre 2013 à 19:29 +0100, Yann Droneaud a écrit :
> Le jeudi 19 décembre 2013 à 09:08 -0800, Roland Dreier a écrit :
> > 
> > However, I still prefer not moving all the casts out into every place
> > where we use INIT_UDATA(), so I'd prefer something like the patch
> > below.  I integrated that into the series and pushed out the result
> > (since everyone seems OK with rebasing for-next).
> > 
> > Please let me know what you think.
> > 
> 
> I'm a bit puzzled :) Sure it address the warning problem from sparse and
> gcc. It's even handling setting NULL for response if out_words is 0,
> which was part of another patch[1] in the patchset. So I would say OK.
> 
> But I'm concerned about this global change of behavior of INIT_UDATA().
> 
> I felt it was OK to handle the case of out_words equal to 0 in the new
> uverbs, since there's no current user of this, there's nothing to broke.
> It was a limited, locally change.
> 
> But when changing globally the behavor of INIT_UDATA(), what about
> existing program which pass a non-NULL address for the response and set
> out_words to 0 in a command.
> 
> libibverbs doesn't seems to do this, but who knows how one might try to
> access to uverbs ?
> 
> Since currently ib_copy_to_udata() doesn't check struct ib_udata outlen
> (I have patch for that[2]), some provider (hw/) might be using
> ib_copy_to_udata() without paying attention to outlen.
> 
> If outbuf is set to NULL when outlen is 0, then it might broke, and this
> could be called a regression.
> 
> I haven't double check each and every user of ib_copy_to_udata(), even
> if I've found some ugly hack regarding udata, I'm looking at you
> ipath[3] and qib[4], I haven't payed yet attention to user of
> ib_copy_to_udata() without check for outlen first.
> 

I took some time to read again the source code.

I've asked about the consequences for users of ib_copy_to_udata(), but
ib_copy_from_udata() is also concerned by this issue, but very unlikely.

To demonstrate how it is possible to have valid (!) uverbs calls
building a struct ib_udata with either outlen set 0 or inlen set 0,
let's define some 'vendor' command and response structure in userspace:

    #include <infiniband/kern-abi.h>

    struct ibv_cmd_hdr {
        __u32 command;
        __u16 in_words;
        __u16 out_words;
    };

    struct vendor_create_cq {
        struct ibv_create_cq ibv_cmd;
        /* vendor defined fields */	
    };
 
    struct vendor_create_cq_resp resp {
        struct ibv_create_cq_resp ibv_resp;
        /* vendor defined fields */	
    };

Now, let's build a request through a custom 'libibverbs/libvendor-rdma'
which might be valid under current kernel, but will set outlen to 0
while response/outbuf would be != NULL:

    struct ibv_cq *vendor_create_cq_broken_outlen(...)
    {
        struct vendor_create_cq      cmd;
        struct vendor_create_cq_resp resp;
        ...
        IBV_INIT_CMD_RESP(&cmd,
                          sizeof(cmd),
                          CREATE_CQ,
                          &resp,
                          sizeof(struct ibv_create_cq_resp)); /* <-- */
        ...
        write(context->cmd_fd,
              &cmd,
              sizeof(cmd));
        ...
    }

Now let's build a request which might be valid with current kernel
but will set inlen to 0 while it will be possible for vendor
driver (provider, under hw/) to read vendor fields:

    struct ibv_cq *vendor_create_cq_broken_inlen(...)
    {
        struct vendor_create_cq      cmd;
        struct vendor_create_cq_resp resp;
        size_t cmd_size = sizeof(struct ibv_create_cq) - 
                          sizeof(struct ibv_cmd_hdr); /* <-- */
        ...
        assert(cmd_size >= sizeof(struct ibv_cmd_hdr));
        ...
        IBV_INIT_CMD_RESP(&cmd,
                          cmd_size,
                          CREATE_CQ,
                          &resp,
                          sizeof(resp));
        ...
        write(context->cmd_fd,
              &cmd,
              cmd_size);
        ...
    }

It's harder to get it wrong here, because struct ib_udata inlen, for
current uverbs (eg. not the extended one, currently only flow steering
related), have not subtracted the command header length, while the inbuf
pointer is added the command header length. This is a bit schizophrenic.
It's making bound checking a little bit harder, see for example
mthca_reg_user_mr(), which is the only function doing it right.
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/mthca/mthca_provider.c?id=v3.13-rc4#n988

(I also have a patch to subtract the size of header from input length,
 so that my patches[1] to add proper bound checking become effective at
 checking the bounds).

So in order to trigger INIT_UDATA() to set inbuf to NULL, the total size
of the command must be equal to the size of command, leaving apart the
size of the header, while still required to be greater than the header
to be accepted by ib_uverbs_write().

We can safely state that it is very unlikely that any unknown userspace
program is doing this. But keep it mind it's possible.

So if INIT_UDATA() is changed to set inbuf to NULL if inlen is equal
to 0, and to set outbuf to NULL if outlen is equal to 0, then, code
using, respectively, ib_copy_from_udata() and ib_copy_to_udata() 
without checking the size will break for the previous code snippet.

I believe these are corner cases, and, in french, I would say it's "tiré
par les cheveux" [2], but when looking at infiniband/hw/, we can see
several (hint, *a lot of*) places where bound checking is not done.

(That's why I've suggested adding checks in ib_copy_{from,to}_udata(),
 in my patchset[1][3][4], but it require subtracting the header length, 
 fix mthca to not do it, fix ipath and qib to not swap the pointer 
 behind the scene, and a lot of testing/reviewing to catch possible 
 regression, but sometime these a desirable, but I digress).

Here's a list of calls to ib_copy_{from,to}_data() for which there's no
or incorrect checking of {input,output} length:

- drivers/infiniband/hw/cxgb3/iwch_provider.c:162, iwch_create_cq()

ib_copy_from_udata() with no check on input length.

- drivers/infiniband/hw/cxgb3/iwch_provider.c:232, iwch_create_cq()

ib_copy_to_udata(), with a check on output length,
                    but will copy even if output length is 0.

- drivers/infiniband/hw/cxgb3/iwch_provider.c:436, iwch_allocate_pd()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/cxgb3/iwch_provider.c:706, iwch_reg_user_mr()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/cxgb3/iwch_provider.c:1026, iwch_create_qp()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/amso1100/c2_provider.c:171, c2_alloc_pd()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx4/cq.c:195, mlx4_ib_create_cq()

ib_copy_from_udata() with no check on input length.

- drivers/infiniband/hw/mlx4/cq.c:240, mlx4_ib_create_cq()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx4/cq.c:302, mlx4_alloc_resize_umem()
  (called through mlx4_ib_resize_cq())

ib_copy_from_udata() with no check on input length.

- drivers/infiniband/hw/mlx4/srq.c:111, mlx4_ib_create_srq()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/mlx4/srq.c:193, mlx4_ib_create_srq()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx4/main.c:627, mlx4_ib_alloc_ucontext()
  drivers/infiniband/hw/mlx4/main.c:629, mlx4_ib_alloc_ucontext()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx4/main.c:696, mlx4_ib_alloc_pd()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx4/qp.c:677, create_qp_common()
  (called through mlx4_ib_create_qp())

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/ehca/ehca_qp.c:906, internal_create_qp()
  (called through ehca_create_qp())

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/ehca/ehca_cq.c:284, ehca_create_cq()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/nes/nes_verbs.c:651, nes_alloc_ucontext()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/nes/nes_verbs.c:682, nes_alloc_ucontext()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/nes/nes_verbs.c:817, nes_alloc_pd()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/nes/nes_verbs.c:1185, nes_create_qp()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/nes/nes_verbs.c:1392, nes_create_qp()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/nes/nes_verbs.c:1574, nes_create_cq()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/nes/nes_verbs.c:1771, nes_create_cq()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/nes/nes_verbs.c:2346, nes_reg_user_mr()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/usnic/usnic_ib_verbs.c:114,
usnic_ib_fill_create_qp_resp() (called through usnic_ib_create_qp())

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/cxgb4/cq.c:943 c4iw_create_cq()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/cxgb4/provider.c:226, c4iw_allocate_pd()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/cxgb4/qp.c:1683, c4iw_create_qp()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mthca/mthca_provider.c:333,
mthca_alloc_ucontext()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mthca/mthca_provider.c:389, mthca_alloc_pd()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mthca/mthca_provider.c:453, mthca_create_srq()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/mthca/mthca_provider.c:479, mthca_create_srq()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mthca/mthca_provider.c:535, mthca_create_qp()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/mthca/mthca_provider.c:658, mthca_create_cq()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/mthca/mthca_provider.c:696, mthca_create_cq()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mthca/mthca_provider.c:791, mthca_resize_cq()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/mlx5/cq.c:526, create_cq_user()
  (called through mlx5_ib_create_cq())

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/mlx5/cq.c:707, mlx5_ib_create_cq()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx5/srq.c:87, create_srq_user()
  (called through mlx5_ib_create_srq())

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/mlx5/cq.c:307, mlx5_ib_create_srq()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx5/main.c:552, mlx5_ib_alloc_ucontext()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/mlx5/main.c:621, mlx5_ib_alloc_ucontext()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx5/main.c:798, mlx5_ib_alloc_pd()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx5/qp.c:500, create_qp_user()
  (called through create_qp_common())

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/mlx5/qp.c:567, create_qp_user()
  (called through create_qp_common())

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/mlx5/qp.c:752, create_qp_common()
  (called through mlx5_ib_create_qp())

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:405,
ocrdma_alloc_ucontext()

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:517,
ocrdma_copy_pd_uresp()
  (called through ocrdma_alloc_pd())

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:870,
ocrdma_copy_cq_uresp()
  (called through ocrdma_create_cq())

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:901, ocrdma_create_cq()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:1109,
ocrdma_copy_qp_uresp()
  (called through ocrdma_create_qp())

ib_copy_to_udata(), with no check on output length.

- drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:1213, ocrdma_create_qp()

ib_copy_from_udata(), with no check on input length.

- drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:1669,
ocrdma_copy_srq_uresp()
  (called through ocrdma_create_srq())

ib_copy_to_udata(), with no check on output length.


> So I think we must double check here before applying a global change 
> to INIT_UDATA() and/or be ready to pay the price of regression in some
> vendor software (I'm not aware of) ...

As you can see, they're plenty of location where having the size equal
to 0 but pointer != NULL might be legal and could be used.

So in the end, we must determinate where is the kernel ABI: is write(2)
syscall or libibverbs ?

In the first case, allowing INIT_UDATA() to set the pointer to NULL if
size is 0 will break the API/ABI as it could break existing (buggy)
programs (but we don't know if such exist).
In the second case, it won't break anything, since libibverbs could be
assumed to do the right thing (but I haven't checked it a lot).

Regards.

[1] [PATCH 00/22] infiniband: improve userspace input check
    http://marc.info/?i=cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

[2] http://en.wiktionary.org/wiki/tir%C3%A9_par_les_cheveux

[3] [PATCH 03/22] infiniband: ib_copy_from_udata(): check input length
http://marc.info/?i=2bf102a41c51f61965ee09df827abe8fefb523a9.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

[4] [PATCH 04/22] infiniband: ib_copy_to_udata(): check output length
http://marc.info/?i=d27716a3a1c180f832d153a7402f65ea8a75b734.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html