In the original code there was a potential integer overflow if you
passed in a large cmd.ne. The calls to kmalloc() would allocate smaller
buffers than intended, leading to memory corruption.
There was also an information leak.
Documentation/infiniband/user_verbs.txt suggests this function is meant
for unprivileged access.
Jason Gunthorpe suggested that I should modify it to pass the data to
the user bit by bit and avoid the kmalloc() entirely.
CC: stable@xxxxxxxxxx
Signed-off-by: Dan Carpenter <error27@xxxxxxxxx>
---
Please, please, check this. I've think I've done it right, but I don't
have the hardware and can not test it.
It's strange to me that we return "in_len" on success.
struct ib_uverbs_poll_cq_resp is used by userspace libraries right?
Otherwise I could delete it.
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 6fcfbeb..b0788b6 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -891,68 +891,89 @@ out:
return ret ? ret : in_len;
}
+static int copy_header_to_user(void __user *dest, u32 count)
+{
+ u32 header[2]; /* the second u32 is reserved */
+
+ memset(header, 0, sizeof(header));
+ if (copy_to_user(dest, header, sizeof(header)))
+ return -EFAULT;
+ return 0;
+}
+
+static int copy_wc_to_user(void __user *dest, struct ib_wc *wc)
+{
+ struct ib_uverbs_wc tmp;
+
+ memset(&tmp, 0, sizeof(tmp));
+
+ tmp.wr_id = wc->wr_id;
+ tmp.status = wc->status;
+ tmp.opcode = wc->opcode;
+ tmp.vendor_err = wc->vendor_err;
+ tmp.byte_len = wc->byte_len;
+ tmp.ex.imm_data = (__u32 __force) wc->ex.imm_data;
+ tmp.qp_num = wc->qp->qp_num;
+ tmp.src_qp = wc->src_qp;
+ tmp.wc_flags = wc->wc_flags;
+ tmp.pkey_index = wc->pkey_index;
+ tmp.slid = wc->slid;
+ tmp.sl = wc->sl;
+ tmp.dlid_path_bits = wc->dlid_path_bits;
+ tmp.port_num = wc->port_num;
+
+ if (copy_to_user(dest, &tmp, sizeof(tmp)))
+ return -EFAULT;
+ return 0;
+}
+
ssize_t ib_uverbs_poll_cq(struct ib_uverbs_file *file,
const char __user *buf, int in_len,
int out_len)
{
struct ib_uverbs_poll_cq cmd;
- struct ib_uverbs_poll_cq_resp *resp;
+ u8 __user *header_ptr;
+ u8 __user *data_ptr;
struct ib_cq *cq;
- struct ib_wc *wc;
- int ret = 0;
+ struct ib_wc wc;
+ u32 count = 0;
+ int ret;
int i;
- int rsize;
if (copy_from_user(&cmd, buf, sizeof cmd))
return -EFAULT;
- wc = kmalloc(cmd.ne * sizeof *wc, GFP_KERNEL);
- if (!wc)
- return -ENOMEM;
-
- rsize = sizeof *resp + cmd.ne * sizeof(struct ib_uverbs_wc);
- resp = kmalloc(rsize, GFP_KERNEL);
- if (!resp) {
- ret = -ENOMEM;
- goto out_wc;
- }
-
cq = idr_read_cq(cmd.cq_handle, file->ucontext, 0);
- if (!cq) {
- ret = -EINVAL;
- goto out;
- }
+ if (!cq)
+ return -EINVAL;
- resp->count = ib_poll_cq(cq, cmd.ne, wc);
+ /* we copy a struct ib_uverbs_poll_cq_resp to user space */
+ header_ptr = (void __user *)(unsigned long)cmd.response;
+ data_ptr = header_ptr + sizeof(u32) * 2;
- put_cq_read(cq);
+ for (i = 0; i < cmd.ne; i++) {
+ ret = ib_poll_cq(cq, 1, &wc);
+ if (ret < 0)
+ goto out_put;
+ if (!ret)
+ break;
- for (i = 0; i < resp->count; i++) {
- resp->wc[i].wr_id = wc[i].wr_id;
- resp->wc[i].status = wc[i].status;
- resp->wc[i].opcode = wc[i].opcode;
- resp->wc[i].vendor_err = wc[i].vendor_err;
- resp->wc[i].byte_len = wc[i].byte_len;
- resp->wc[i].ex.imm_data = (__u32 __force) wc[i].ex.imm_data;
- resp->wc[i].qp_num = wc[i].qp->qp_num;
- resp->wc[i].src_qp = wc[i].src_qp;
- resp->wc[i].wc_flags = wc[i].wc_flags;
- resp->wc[i].pkey_index = wc[i].pkey_index;
- resp->wc[i].slid = wc[i].slid;
- resp->wc[i].sl = wc[i].sl;
- resp->wc[i].dlid_path_bits = wc[i].dlid_path_bits;
- resp->wc[i].port_num = wc[i].port_num;
+ ret = copy_wc_to_user(data_ptr, &wc);
+ if (ret)
+ goto out_put;
+ data_ptr += sizeof(struct ib_uverbs_wc);
+ count++;
}
- if (copy_to_user((void __user *) (unsigned long) cmd.response, resp, rsize))
- ret = -EFAULT;
+ ret = copy_header_to_user(header_ptr, count);
+ if (ret)
+ goto out_put;
-out:
- kfree(resp);
+ ret = in_len;
-out_wc:
- kfree(wc);
- return ret ? ret : in_len;
+out_put:
+ put_cq_read(cq);
+ return ret;
}
ssize_t ib_uverbs_req_notify_cq(struct ib_uverbs_file *file,
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
