Skip to contentSkip to conversation

IB/uverbs: check request parameters

PATCH In review Yann Droneaud 2 participants 10 messages May 4, 2015 – May 5, 2015 3 messages not in the archive

Subsystems (2)
InfiniBand Mellanox MLX5 IB driver
lore
Share & tools
lore

Yann Droneaud

author
#0 lore raw

cover IB/uverbs: check request parameters

Hi,

Please find a patchset against uverbs to improve the checks done on uverbs request parameters. This patchset in an extract of a previous patchset sent some times ago[1].

I've provided some explanation of the issues partialy addressed by this patchset in a previous message[2].

As we're now addressing overflows, I think it's time to apply this patchset.

Changes since v0 [3]
- updated against v4.1-rc2
- incorporated patches to add check on response buffer using access_ok()

[1] "[PATCH 00/22] infiniband: improve userspace input check"

http://marc.info/?i=cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

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

http://marc.info/?i=1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
http://mid.gmane.org/1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]

[3] "[PATCH 0/4] IB/uverbs: check request parameters"

http://marc.info/?i=cover.1405884453.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1405884453.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Yann Droneaud (6):
IB/uverbs: check userspace input buffer size
IB/uverbs: check userspace output buffer size
IB/uverbs: check userspace output buffer size in ib_uverbs_poll_cq() IB/uverbs: subtract command header from input size IB/uverbs: move cast from u64 to void __user pointer to its own
variable
IB/uverbs: check access to userspace response buffer

 drivers/infiniband/core/uverbs_cmd.c         | 449 +++++++++++++++++++++------
 drivers/infiniband/core/uverbs_main.c        |  29 +-
 drivers/infiniband/hw/mlx5/cq.c              |   6 +-
 drivers/infiniband/hw/mlx5/main.c            |   2 +-
 drivers/infiniband/hw/mlx5/srq.c             |   6 +-
 drivers/infiniband/hw/mthca/mthca_provider.c |   2 +-
 6 files changed, 382 insertions(+), 112 deletions(-)
Signature · 6 lines
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): cover.1430743694.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

author
#2 lore raw

PATCH 1/6 IB/uverbs: check userspace input buffer size

This patch makes uverbs functions check the length of the input buffer before reading the command content.

This will detect truncated command and will prevent uverbs from reading past userspace provided buffer.

Additionally, it will also prevent underflow while computing remaining input space. Such underflow would set inlen field in struct ib_udata in call to INIT_UDATA() to a meaningless value.

For example:

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

If in_len is less than sizeof(cmd), the result of subtraction is a negative value since in_len is an int per function prototype. But inlen field in struct ib_udata is unsigned, so the result is an overly large value in struct ib_udata. Such value will make further size checking useless, allowing more out of bound read in providers (eg. hw/) code.

Note that checking the input size and preventing the underflow and/or overflow might break existing userspace program that incorrectly set the buffer length in a uverbs request.

It's theoretically possible for a userspace driver to send a request with a wrong size that can still be processed correctly with the kernel:

Let's build a request through fake 'libibverbs' and 'libvendor-rdma' userspace driver.

It's a request which might be valid with current kernel but will set inlen to (size_t) -1 while it will be possible for vendor driver (eg. provider, under hw/) to read vendor fields:

    #include <infiniband/kern-abi.h>

    /* aka. struct ib_uverbs_cmd_hdr */
    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 */
    };

    struct ibv_cq *vendor_create_cq_broken_inlen(...)
    {
        struct vendor_create_cq      cmd;
        struct vendor_create_cq_resp resp;
        size_t cmd_size;

        /* the command header size must be subtracted here
           to ensure (inlen - sizeof(struct ib_uverbs_create_cq)
           is equal to 0 in ib_uverbs_create_cq(), as the header
           size is not subtracted in ib_uverbs_write() before
           invoking ib_uverbs_create_cq() */

        assert(sizeof(struct ibv_create_cq) >= sizeof(struct ibv_cmd_hdr));
        cmd_size = sizeof(struct ibv_create_cq) -
                   sizeof(struct ibv_cmd_hdr);

        /* instead of 0, make (inlen - sizeof(cmd)) underflow */
        cmd_size--;

        /* For the command to be valid in ib_uverbs_write(),
           it size must be at least equal to sizeof(struct
           ib_uverbs_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);

        ...

    }

This example shows how a request can be created to trigger un-handled underflow while allowing provider (eg. hw/) to process the request.

The provided patch will make it impossible to do it on patched kernels.

Applications using libibverbs are not going to be affected by this patch, but unknown broken applications using directly kernel uverbs API may fail.

Link http://marc.info/?i=1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Link http://marc.info/?i=cover.1430743694.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by Yann Droneaud <[email protected]>
---
 drivers/infiniband/core/uverbs_cmd.c | 105 +++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
drivers/infiniband/core/uverbs_cmd.c +105 -0
index a9f048990dfc..6d26bfab1bc6 100644
@@ -296,6 +296,9 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 	struct file			 *filp;
 	int ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -455,6 +458,9 @@ ssize_t ib_uverbs_query_device(struct ib_uverbs_file *file,
 	struct ib_device_attr              attr;
 	int                                ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -484,6 +490,9 @@ ssize_t ib_uverbs_query_port(struct ib_uverbs_file *file,
 	struct ib_port_attr              attr;
 	int                              ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -536,6 +545,9 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,
 	struct ib_pd                  *pd;
 	int                            ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -607,6 +619,9 @@ ssize_t ib_uverbs_dealloc_pd(struct ib_uverbs_file *file,
 	struct ib_uobject          *uobj;
 	int                         ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -733,6 +748,9 @@ ssize_t ib_uverbs_open_xrcd(struct ib_uverbs_file *file,
 	int				ret = 0;
 	int				new_xrcd = 0;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -868,6 +886,9 @@ ssize_t ib_uverbs_close_xrcd(struct ib_uverbs_file *file,
 	int                         live;
 	int                         ret = 0;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -945,6 +966,9 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file,
 	struct ib_mr                *mr;
 	int                          ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -1055,6 +1079,9 @@ ssize_t ib_uverbs_rereg_mr(struct ib_uverbs_file *file,
 	int                          ret;
 	struct ib_uobject	    *uobj;
 
+	if (in_len < sizeof(cmd))
+		return -EINVAL;
+
 	if (out_len < sizeof(resp))
 		return -ENOSPC;
 

@@ -1144,6 +1171,9 @@ ssize_t ib_uverbs_dereg_mr(struct ib_uverbs_file *file,
 	struct ib_uobject	 *uobj;
 	int                       ret = -EINVAL;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -1184,6 +1214,9 @@ ssize_t ib_uverbs_alloc_mw(struct ib_uverbs_file *file,
 	struct ib_mw                  *mw;
 	int                            ret;
 
+	if (in_len < sizeof(cmd))
+		return -EINVAL;
+
 	if (out_len < sizeof(resp))
 		return -ENOSPC;
 

@@ -1264,6 +1297,9 @@ ssize_t ib_uverbs_dealloc_mw(struct ib_uverbs_file *file,
 	struct ib_uobject	   *uobj;
 	int                         ret = -EINVAL;
 
+	if (in_len < sizeof(cmd))
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof(cmd)))
 		return -EFAULT;
 

@@ -1302,6 +1338,9 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file,
 	struct file				  *filp;
 	int ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -1342,6 +1381,9 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,
 	struct ib_cq                   *cq;
 	int                             ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -1441,6 +1483,9 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file,
 	struct ib_cq			*cq;
 	int				ret = -EINVAL;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -1506,6 +1551,9 @@ ssize_t ib_uverbs_poll_cq(struct ib_uverbs_file *file,
 	struct ib_wc                   wc;
 	int                            ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -1552,6 +1600,9 @@ ssize_t ib_uverbs_req_notify_cq(struct ib_uverbs_file *file,
 	struct ib_uverbs_req_notify_cq cmd;
 	struct ib_cq                  *cq;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -1579,6 +1630,9 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
 	struct ib_uverbs_event_file	*ev_file;
 	int                        	 ret = -EINVAL;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -1637,6 +1691,9 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 	struct ib_qp_init_attr          attr;
 	int ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -1827,6 +1884,9 @@ ssize_t ib_uverbs_open_qp(struct ib_uverbs_file *file,
 	struct ib_qp_open_attr          attr;
 	int ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -1919,6 +1979,9 @@ ssize_t ib_uverbs_query_qp(struct ib_uverbs_file *file,
 	struct ib_qp_init_attr         *init_attr;
 	int                            ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2032,6 +2095,9 @@ ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file,
 	struct ib_qp_attr         *attr;
 	int                        ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2129,6 +2195,9 @@ ssize_t ib_uverbs_destroy_qp(struct ib_uverbs_file *file,
 	struct ib_uqp_object        	*obj;
 	int                        	 ret = -EINVAL;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2189,6 +2258,9 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 	int				is_ud;
 	ssize_t                         ret = -EINVAL;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2430,6 +2502,9 @@ ssize_t ib_uverbs_post_recv(struct ib_uverbs_file *file,
 	struct ib_qp                   *qp;
 	ssize_t                         ret = -EINVAL;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2479,6 +2554,9 @@ ssize_t ib_uverbs_post_srq_recv(struct ib_uverbs_file *file,
 	struct ib_srq                      *srq;
 	ssize_t                             ret = -EINVAL;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2530,6 +2608,9 @@ ssize_t ib_uverbs_create_ah(struct ib_uverbs_file *file,
 	struct ib_ah_attr		attr;
 	int ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -2618,6 +2699,9 @@ ssize_t ib_uverbs_destroy_ah(struct ib_uverbs_file *file,
 	struct ib_uobject	   *uobj;
 	int			    ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2656,6 +2740,9 @@ ssize_t ib_uverbs_attach_mcast(struct ib_uverbs_file *file,
 	struct ib_uverbs_mcast_entry *mcast;
 	int                           ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2703,6 +2790,9 @@ ssize_t ib_uverbs_detach_mcast(struct ib_uverbs_file *file,
 	struct ib_uverbs_mcast_entry *mcast;
 	int                           ret = -EINVAL;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -3118,6 +3208,9 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file,
 	struct ib_udata                  udata;
 	int ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -3151,6 +3244,9 @@ ssize_t ib_uverbs_create_xsrq(struct ib_uverbs_file *file,
 	struct ib_udata                  udata;
 	int ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -3178,6 +3274,9 @@ ssize_t ib_uverbs_modify_srq(struct ib_uverbs_file *file,
 	struct ib_srq_attr          attr;
 	int                         ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -3208,6 +3307,9 @@ ssize_t ib_uverbs_query_srq(struct ib_uverbs_file *file,
 	struct ib_srq                   *srq;
 	int                             ret;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (out_len < sizeof resp)
 		return -ENOSPC;
 

@@ -3251,6 +3353,9 @@ ssize_t ib_uverbs_destroy_srq(struct ib_uverbs_file *file,
 	struct ib_usrq_object		 *us;
 	enum ib_srq_type		  srq_type;
 
+	if (in_len < sizeof cmd)
+		return -EINVAL;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
Signature · 6 lines
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#3 lore raw

PATCH 2/6 IB/uverbs: check userspace output buffer size

In reply to a message not in the archive

This patch makes uverbs functions check the length of the output buffer.

This will prevent uverbs from writing past userspace provided buffer.

Additionally, it will prevent an underflow while computing remaining output space. Such underflow would set outlen field in struct ib_udata in call to INIT_UDATA() to a meaningless value.

For example:

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

If out_len is less than sizeof(resp), the result of subtraction is a negative value since out_len is an int per function prototype. But outlen field in struct ib_udata is unsigned, so the result is an overly large value in struct ib_udata. Such value will make further size checking useless, allowing more out of bound write in providers (eg. hw/) code.

Note that checking the output size and preventing the underflow and/or overflow might break existing userspace program that incorrectly set the buffer length in a uverbs request.

It's theoretically possible for a userspace driver to send a request with a wrong size that can still be processed correctly with the kernel:

Let's build a request through fake 'libibverbs' and 'libvendor-rdma' userspace driver.

It's a request which might be valid with current kernel but will set outlen to (size_t) -1 while it will be possible for vendor driver (eg. provider, under hw/) to write vendor fields:

    #include <infiniband/kern-abi.h>

    /* aka. struct ib_uverbs_cmd_hdr */
    struct ibv_cmd_hdr {
        __u32 command;
        __u16 in_words;
        __u16 out_words;
    };

    struct vendor_resize_cq {
        struct ibv_resize_cq ibv_cmd;
        /* vendor defined fields */
    };

    struct vendor_resize_cq_resp resp {
        struct ibv_resize_cq_resp ibv_resp;
        /* vendor defined fields */
    };

    struct ibv_cq *vendor_resize_cq_broken_outlen(...)
    {
        struct vendor_resize_cq      cmd;
        struct vendor_resize_cq_resp resp;
        size_t                       resp_size;

        ...

        /* slightly less than struct ib_uverbs_resize_cq_resp */
        resp_size = sizeof(struct ibv_create_cq_resp);
        resp_size--;

        ...

        IBV_INIT_CMD_RESP(&cmd,
                          sizeof(cmd),
                          RESIZE_CQ,
                          &resp,
                          resp_size);
        ...
        write(context->cmd_fd,
              &cmd,
              sizeof(cmd));
        ...
    }

This example shows how a request can be created to trigger un-handled underflow while allowing provider (eg. hw/) to process the request.

The provided patch will make it impossible to do it on patched kernels.

Applications using libibverbs are not going to be affected by this patch, but unknown broken applications using directly kernel uverbs API may fail.

Link http://marc.info/?i=1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Link http://marc.info/?i=cover.1430743694.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by Yann Droneaud <[email protected]>
---
 drivers/infiniband/core/uverbs_cmd.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
drivers/infiniband/core/uverbs_cmd.c +27 -0
index 6d26bfab1bc6..a3a3b6eafd1d 100644
@@ -1486,6 +1486,9 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -1554,6 +1557,9 @@ ssize_t ib_uverbs_poll_cq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -1633,6 +1639,9 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -1982,6 +1991,9 @@ ssize_t ib_uverbs_query_qp(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2198,6 +2210,9 @@ ssize_t ib_uverbs_destroy_qp(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2261,6 +2276,9 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2505,6 +2523,9 @@ ssize_t ib_uverbs_post_recv(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -2557,6 +2578,9 @@ ssize_t ib_uverbs_post_srq_recv(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 

@@ -3356,6 +3380,9 @@ ssize_t ib_uverbs_destroy_srq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
Signature · 6 lines
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#4 lore raw

PATCH 3/6 IB/uverbs: check userspace output buffer size in ib_uverbs_poll_cq()

In reply to a message not in the archive

Makes ib_uverbs_poll_cq() check the length of the output buffer against number of wc: this will prevent the function from writing past userspace provided buffer.

Note that it might affect existing userspace programs that were setting 'struct ibv_poll_cq' field 'ne' to a value too large compared to the size of the buffer allocated to hold the work completion items.

Perhaps such userspace programs were hoping to have allocated enough memory to hold the maximum number of work completions that can be returned on this particular completion queue or it would have been broken with random memory corruption due to the kernel writing past the response buffer.

Example:

    int vendor_poll_cq_broken(struct ibv_cq *cq, int ne, struct ibv_wc *wc)
    {
        struct ibv_poll_cq       cmd;
        struct ibv_poll_cq_resp *resp;
        int                      resp_size;
        ssize_t                  sret;
        int                      ret = -1;

        cmd.cq_handle = cq->handle;
        cmd.ne        = ne;

         /* forget about ne * sizeof(struct ibv_kern_wc) */
        resp_size = sizeof(*resp);

        resp = malloc(resp_size);
        assert(resp != NULL);

        IBV_INIT_CMD_RESP(&cmd, sizeof(cmd),
                          POLL_CQ,
                          resp, resp_size);

        sret = write(cq->context->cmd_fd,
                     &cmd, sizeof(cmd));

        if (sret != sizeof(cmd))
            goto leave;

        ret = resp->count;

        /* ignore the response */

    leave:

        free(resp);

        return ret;
    }

The provided patch will make it impossible to do it on patched kernels.

Applications using libibverbs are not going to be affected by this patch, but unknown broken applications using directly kernel uverbs API may fail.

Link http://marc.info/?i=cover.1430743694.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by Yann Droneaud <[email protected]>
---
 drivers/infiniband/core/uverbs_cmd.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/infiniband/core/uverbs_cmd.c +3 -0
index a3a3b6eafd1d..147f61c8692b 100644
@@ -1563,6 +1563,9 @@ ssize_t ib_uverbs_poll_cq(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	if ((out_len - sizeof resp)/(sizeof(struct ib_uverbs_wc)) < cmd.ne)
+		return -ENOSPC;
+
 	cq = idr_read_cq(cmd.cq_handle, file->ucontext, 0);
 	if (!cq)
 		return -EINVAL;
Signature · 6 lines
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#5 lore raw

PATCH 4/6 IB/uverbs: subtract command header from input size

In reply to a message not in the archive

The uverbs handler functions are given a pointer to the command buffer, which point right after the command header.

But the size given to the function is the total size, including the command header size.

The size of the command header must be subtracted from the input length before calling the uverbs handler function so that the parameters are in par.

This patch makes ib_uverbs_write() subtract the header size from the total size when calling uverbs function.

As the return value from the uverbs function will be the size without the header (or a negative value), it must not be used as-is. Instead the function use the total size as return value when no error occurred.

Additionally, drivers (mthca, mlx5) taking the size of command header in account in their size checks are modified accordingly.

This patch is a pre-requisite to make bound checking effective.

Link http://marc.info/?i=cover.1430743694.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by Yann Droneaud <[email protected]>
---
 drivers/infiniband/core/uverbs_main.c        | 18 +++++++++++++-----
 drivers/infiniband/hw/mlx5/cq.c              |  6 ++----
 drivers/infiniband/hw/mlx5/main.c            |  2 +-
 drivers/infiniband/hw/mlx5/srq.c             |  6 ++----
 drivers/infiniband/hw/mthca/mthca_provider.c |  2 +-
 5 files changed, 19 insertions(+), 15 deletions(-)
drivers/infiniband/core/uverbs_main.c +13 -5
index 88cce9bb72fe..6ab287c6693c 100644
@@ -601,6 +601,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 {
 	struct ib_uverbs_file *file = filp->private_data;
 	struct ib_uverbs_cmd_hdr hdr;
+	const size_t written_count = count;
 	__u32 flags;
 
 	if (count < sizeof hdr)

@@ -614,6 +615,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 
 	if (!flags) {
 		__u32 command;
+		ssize_t err;
 
 		if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
 					   IB_USER_VERBS_CMD_COMMAND_MASK))

@@ -635,10 +637,17 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		if (hdr.in_words * 4 != count)
 			return -EINVAL;
 
-		return uverbs_cmd_table[command](file,
-						 buf + sizeof(hdr),
-						 hdr.in_words * 4,
-						 hdr.out_words * 4);
+		count -= sizeof(hdr);
+		buf += sizeof(hdr);
+
+		err = uverbs_cmd_table[command](file,
+						buf,
+						count,
+						hdr.out_words * 4);
+		if (err < 0)
+			return err;
+
+		return written_count;
 
 	} else if (flags == IB_USER_VERBS_CMD_FLAG_EXTENDED) {
 		__u32 command;

@@ -647,7 +656,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		struct ib_udata ucore;
 		struct ib_udata uhw;
 		int err;
-		size_t written_count = count;
 
 		if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
 					   IB_USER_VERBS_CMD_COMMAND_MASK))
drivers/infiniband/hw/mlx5/cq.c +2 -4
index 2ee6b1051975..3b38febf685e 100644
@@ -613,10 +613,8 @@ static int create_cq_user(struct mlx5_ib_dev *dev, struct ib_udata *udata,
 	int ncont;
 	int err;
 
-	ucmdlen =
-		(udata->inlen - sizeof(struct ib_uverbs_cmd_hdr) <
-		 sizeof(ucmd)) ? (sizeof(ucmd) -
-				  sizeof(ucmd.reserved)) : sizeof(ucmd);
+	ucmdlen = (udata->inlen < sizeof(ucmd)) ?
+	    (sizeof(ucmd) - sizeof(ucmd.reserved)) : sizeof(ucmd);
 
 	if (ib_copy_from_udata(&ucmd, udata, ucmdlen))
 		return -EFAULT;
drivers/infiniband/hw/mlx5/main.c +1 -1
index 57c9809e8b87..d951bb9a89a3 100644
@@ -408,7 +408,7 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
 		return ERR_PTR(-EAGAIN);
 
 	memset(&req, 0, sizeof(req));
-	reqlen = udata->inlen - sizeof(struct ib_uverbs_cmd_hdr);
+	reqlen = udata->inlen;
 	if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req))
 		ver = 0;
 	else if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req_v2))
drivers/infiniband/hw/mlx5/srq.c +2 -4
index 02d77a29764d..c342bd47e168 100644
@@ -86,10 +86,8 @@ static int create_srq_user(struct ib_pd *pd, struct mlx5_ib_srq *srq,
 	int ncont;
 	u32 offset;
 
-	ucmdlen =
-		(udata->inlen - sizeof(struct ib_uverbs_cmd_hdr) <
-		 sizeof(ucmd)) ? (sizeof(ucmd) -
-				  sizeof(ucmd.reserved)) : sizeof(ucmd);
+	ucmdlen = (udata->inlen < sizeof(ucmd)) ?
+	    (sizeof(ucmd) - sizeof(ucmd.reserved)) : sizeof(ucmd);
 
 	if (ib_copy_from_udata(&ucmd, udata, ucmdlen)) {
 		mlx5_ib_dbg(dev, "failed copy udata\n");
drivers/infiniband/hw/mthca/mthca_provider.c +1 -1
index 415f8e1a54db..e5933b032fc3 100644
@@ -986,7 +986,7 @@ static struct ib_mr *mthca_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 	int err = 0;
 	int write_mtt_size;
 
-	if (udata->inlen - sizeof (struct ib_uverbs_cmd_hdr) < sizeof ucmd) {
+	if (udata->inlen < sizeof ucmd) {
 		if (!to_mucontext(pd->uobject->context)->reg_mr_warned) {
 			mthca_warn(dev, "Process '%s' did not pass in MR attrs.\n",
 				   current->comm);
Signature · 6 lines
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#6 lore raw

PATCH 5/6 IB/uverbs: move cast from u64 to void __user pointer to its own variable

In reply to a message not in the archive

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

This variable will be used for multiple purposes:
- check pointer / size validity with access_ok()
- setup ib_udata through INIT_UDATA()
- in copy_to_user()

When implicit cast will be removed from INIT_UDATA() macro, this variable will be used so that the address will be stored in pointer and will not trigger warnings from compiler.

Link http://marc.info/?i=cover.1430743694.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by Yann Droneaud <[email protected]>
---
 drivers/infiniband/core/uverbs_cmd.c  | 175 ++++++++++++++++++++++------------
 drivers/infiniband/core/uverbs_main.c |  11 ++-
 2 files changed, 121 insertions(+), 65 deletions(-)
drivers/infiniband/core/uverbs_cmd.c +114 -61
index 147f61c8692b..046ca87480e9 100644
@@ -287,6 +287,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;
 #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING

@@ -305,6 +306,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) {

@@ -313,7 +316,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);

@@ -364,8 +367,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;
 	}

@@ -455,6 +457,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;
 

@@ -467,6 +470,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;

@@ -474,8 +479,7 @@ ssize_t ib_uverbs_query_device(struct ib_uverbs_file *file,
 	memset(&resp, 0, sizeof resp);
 	copy_query_dev_fields(file, &resp, &attr);
 
-	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;

@@ -487,6 +491,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;
 

@@ -499,6 +504,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;

@@ -527,8 +534,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;

@@ -540,6 +546,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;

@@ -554,8 +561,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);

@@ -584,8 +593,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;
 	}

@@ -740,6 +748,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;

@@ -757,8 +766,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);

@@ -830,8 +841,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;
 	}

@@ -960,6 +970,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;

@@ -975,8 +986,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))

@@ -1034,8 +1047,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;
 	}

@@ -1072,6 +1084,7 @@ ssize_t ib_uverbs_rereg_mr(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_rereg_mr      cmd;
 	struct ib_uverbs_rereg_mr_resp resp;
+	char __user                   *response;
 	struct ib_udata              udata;
 	struct ib_pd                *pd = NULL;
 	struct ib_mr                *mr;

@@ -1088,8 +1101,10 @@ ssize_t ib_uverbs_rereg_mr(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof(cmd)))
 		return -EFAULT;
 
+	response = (char __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.flags & ~IB_MR_REREG_SUPPORTED || !cmd.flags)

@@ -1145,8 +1160,7 @@ ssize_t ib_uverbs_rereg_mr(struct ib_uverbs_file *file,
 	resp.lkey      = mr->lkey;
 	resp.rkey      = mr->rkey;
 
-	if (copy_to_user((void __user *)(unsigned long)cmd.response,
-			 &resp, sizeof(resp)))
+	if (copy_to_user(response, &resp, sizeof(resp)))
 		ret = -EFAULT;
 	else
 		ret = in_len;

@@ -1209,6 +1223,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;

@@ -1223,6 +1238,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;

@@ -1256,8 +1273,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;
 	}

@@ -1335,6 +1351,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;
 

@@ -1347,6 +1364,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;

@@ -1358,8 +1377,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;

@@ -1375,6 +1393,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;

@@ -1390,8 +1409,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)

@@ -1442,8 +1463,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;
 	}

@@ -1479,6 +1499,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;

@@ -1492,8 +1513,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);

@@ -1506,8 +1529,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:

@@ -1548,7 +1570,7 @@ ssize_t ib_uverbs_poll_cq(struct ib_uverbs_file *file,
 {
 	struct ib_uverbs_poll_cq       cmd;
 	struct ib_uverbs_poll_cq_resp  resp;
-	u8 __user                     *header_ptr;
+	char __user		      *response;
 	u8 __user                     *data_ptr;
 	struct ib_cq                  *cq;
 	struct ib_wc                   wc;

@@ -1563,6 +1585,8 @@ ssize_t ib_uverbs_poll_cq(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	if ((out_len - sizeof resp)/(sizeof(struct ib_uverbs_wc)) < cmd.ne)
 		return -ENOSPC;
 

@@ -1571,8 +1595,7 @@ ssize_t ib_uverbs_poll_cq(struct ib_uverbs_file *file,
 		return -EINVAL;
 
 	/* 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 resp;
+	data_ptr = response + sizeof resp;
 
 	memset(&resp, 0, sizeof resp);
 	while (resp.count < cmd.ne) {

@@ -1590,7 +1613,7 @@ ssize_t ib_uverbs_poll_cq(struct ib_uverbs_file *file,
 		++resp.count;
 	}
 
-	if (copy_to_user(header_ptr, &resp, sizeof resp)) {
+	if (copy_to_user(response, &resp, sizeof resp)) {
 		ret = -EFAULT;
 		goto out_put;
 	}

@@ -1633,6 +1656,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;

@@ -1648,6 +1672,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;

@@ -1678,8 +1704,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;

@@ -1691,6 +1716,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;

@@ -1712,11 +1738,13 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	if (cmd.qp_type == IB_QPT_RAW_PACKET && !capable(CAP_NET_RAW))
 		return -EPERM;
 
 	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);

@@ -1829,8 +1857,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;
 	}

@@ -1888,6 +1915,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;

@@ -1905,8 +1933,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);

@@ -1948,8 +1978,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;
 	}

@@ -1986,6 +2015,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;

@@ -2000,6 +2030,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) {

@@ -2075,8 +2107,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:

@@ -2205,6 +2236,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;

@@ -2219,6 +2251,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);

@@ -2256,8 +2290,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;

@@ -2269,6 +2302,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;

@@ -2285,6 +2319,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
+	response = (void __user *)(unsigned long)cmd.response;
+
 	if (in_len < sizeof cmd + cmd.wqe_size * cmd.wr_count +
 	    cmd.sge_count * sizeof (struct ib_uverbs_sge))
 		return -EINVAL;

@@ -2407,8 +2443,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:

@@ -2519,6 +2554,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;

@@ -2532,6 +2568,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);

@@ -2554,8 +2592,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:

@@ -2574,6 +2611,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;

@@ -2587,6 +2625,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);

@@ -2609,8 +2649,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:

@@ -2629,6 +2668,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;

@@ -2644,6 +2684,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;

@@ -2686,8 +2728,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;
 	}

@@ -3097,6 +3138,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;

@@ -3104,6 +3146,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;

@@ -3179,8 +3223,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;
 	}

@@ -3232,6 +3275,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;
 

@@ -3244,6 +3288,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;

@@ -3253,7 +3299,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);

@@ -3268,6 +3314,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;
 

@@ -3280,8 +3327,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);

@@ -3330,6 +3379,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;

@@ -3343,6 +3393,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;

@@ -3360,8 +3412,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;

@@ -3373,6 +3424,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;

@@ -3389,6 +3441,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;

@@ -3423,8 +3477,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;
drivers/infiniband/core/uverbs_main.c +7 -4
index 6ab287c6693c..24225d32aa55 100644
@@ -653,6 +653,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;

@@ -688,12 +689,14 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		if (ex_hdr.cmd_hdr_reserved)
 			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;
 
 			if (!access_ok(VERIFY_WRITE,
-				       (void __user *) (unsigned long) ex_hdr.response,
+				       response,
 				       (hdr.out_words + ex_hdr.provider_out_words) * 8))
 				return -EFAULT;
 		} else {

@@ -701,12 +704,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 				return -EINVAL;
 		}
 
-		INIT_UDATA_BUF_OR_NULL(&ucore, buf, (unsigned long) ex_hdr.response,
+		INIT_UDATA_BUF_OR_NULL(&ucore, buf, response,
 				       hdr.in_words * 8, hdr.out_words * 8);
 
 		INIT_UDATA_BUF_OR_NULL(&uhw,
 				       buf + ucore.inlen,
-				       (unsigned long) ex_hdr.response + ucore.outlen,
+				       response + ucore.outlen,
 				       ex_hdr.provider_in_words * 8,
 				       ex_hdr.provider_out_words * 8);
Signature · 6 lines
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#7 lore raw

PATCH 6/6 IB/uverbs: check access to userspace response buffer

In reply to a message not in the archive

Add checks like the one added in commit 6cc3df840a8 ('IB/uverbs: Check access to userspace response buffer in extended command'), check output buffers with access_ok(VERIFY_WRITE,...) to ensure they are fully in userspace memory: if the buffer or a subset of the buffer is not valid, returns -EFAULT.

Each uverbs functions using a response buffer must have the same check. This patch adds such checks.

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.

In order to check the output buffer in its entirety with access_ok() to reject invalid request before any other checks, this patch move the output size check after copy_from_user(),

It's going to slightly change the behavior of uverbs: an invalid command partially not mapped, with a response size too short, would return -EFAULT while previously it would have returned -ENOSPC. Anyway, it makes more sense to return -EFAULT in such case.

Applications using libibverbs are not going to be affected by this patch, but unknown broken applications using directly kernel uverbs API may fail.

Link http://marc.info/?i=cover.1430743694.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by Yann Droneaud <[email protected]>
---
 drivers/infiniband/core/uverbs_cmd.c | 225 +++++++++++++++++++++++------------
 1 file changed, 150 insertions(+), 75 deletions(-)
drivers/infiniband/core/uverbs_cmd.c +150 -75
index 046ca87480e9..4fe77814c0e8 100644
@@ -300,14 +300,17 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	mutex_lock(&file->mutex);
 
 	if (file->ucontext) {

@@ -464,14 +467,17 @@ ssize_t ib_uverbs_query_device(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	ret = ib_query_device(file->device->ib_dev, &attr);
 	if (ret)
 		return ret;

@@ -498,14 +504,17 @@ ssize_t ib_uverbs_query_port(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	ret = ib_query_port(file->device->ib_dev, cmd.port_num, &attr);
 	if (ret)
 		return ret;

@@ -555,14 +564,17 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
 		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);

@@ -760,14 +772,17 @@ ssize_t ib_uverbs_open_xrcd(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
 		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof  resp);

@@ -980,14 +995,17 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
 		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);

@@ -1095,14 +1113,17 @@ ssize_t ib_uverbs_rereg_mr(struct ib_uverbs_file *file,
 	if (in_len < sizeof(cmd))
 		return -EINVAL;
 
-	if (out_len < sizeof(resp))
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof(cmd)))
 		return -EFAULT;
 
 	response = (char __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof(resp))
+		return -ENOSPC;
+
 	INIT_UDATA(&udata, buf + sizeof(cmd),
 		   response + sizeof(resp),
 		   in_len - sizeof(cmd), out_len - sizeof(resp));

@@ -1232,14 +1253,17 @@ ssize_t ib_uverbs_alloc_mw(struct ib_uverbs_file *file,
 	if (in_len < sizeof(cmd))
 		return -EINVAL;
 
-	if (out_len < sizeof(resp))
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof(cmd)))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof(resp))
+		return -ENOSPC;
+
 	uobj = kmalloc(sizeof(*uobj), GFP_KERNEL);
 	if (!uobj)
 		return -ENOMEM;

@@ -1358,14 +1382,17 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	ret = get_unused_fd_flags(O_CLOEXEC);
 	if (ret < 0)
 		return ret;

@@ -1403,14 +1430,17 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
 		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);

@@ -1507,14 +1537,17 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
 		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);

@@ -1579,14 +1612,17 @@ ssize_t ib_uverbs_poll_cq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if ((out_len - sizeof resp)/(sizeof(struct ib_uverbs_wc)) < cmd.ne)
 		return -ENOSPC;
 

@@ -1666,14 +1702,17 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	uobj = idr_write_uobj(&ib_uverbs_cq_idr, cmd.cq_handle, file->ucontext);
 	if (!uobj)
 		return -EINVAL;

@@ -1732,14 +1771,17 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (cmd.qp_type == IB_QPT_RAW_PACKET && !capable(CAP_NET_RAW))
 		return -EPERM;
 

@@ -1927,14 +1969,17 @@ ssize_t ib_uverbs_open_qp(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
 		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);

@@ -2024,14 +2069,17 @@ ssize_t ib_uverbs_query_qp(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	attr      = kmalloc(sizeof *attr, GFP_KERNEL);
 	init_attr = kmalloc(sizeof *init_attr, GFP_KERNEL);
 	if (!attr || !init_attr) {

@@ -2245,14 +2293,17 @@ ssize_t ib_uverbs_destroy_qp(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	memset(&resp, 0, sizeof resp);
 
 	uobj = idr_write_uobj(&ib_uverbs_qp_idr, cmd.qp_handle, file->ucontext);

@@ -2313,14 +2364,17 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	if (in_len < sizeof cmd + cmd.wqe_size * cmd.wr_count +
 	    cmd.sge_count * sizeof (struct ib_uverbs_sge))
 		return -EINVAL;

@@ -2562,14 +2616,17 @@ ssize_t ib_uverbs_post_recv(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	wr = ib_uverbs_unmarshall_recv(buf + sizeof cmd,
 				       in_len - sizeof cmd, cmd.wr_count,
 				       cmd.sge_count, cmd.wqe_size);

@@ -2619,14 +2676,17 @@ ssize_t ib_uverbs_post_srq_recv(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	wr = ib_uverbs_unmarshall_recv(buf + sizeof cmd,
 				       in_len - sizeof cmd, cmd.wr_count,
 				       cmd.sge_count, cmd.wqe_size);

@@ -2678,14 +2738,17 @@ ssize_t ib_uverbs_create_ah(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	uobj = kmalloc(sizeof *uobj, GFP_KERNEL);
 	if (!uobj)
 		return -ENOMEM;

@@ -3282,14 +3345,17 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	xcmd.response	 = cmd.response;
 	xcmd.user_handle = cmd.user_handle;
 	xcmd.srq_type	 = IB_SRQT_BASIC;

@@ -3321,14 +3387,17 @@ ssize_t ib_uverbs_create_xsrq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	INIT_UDATA(&udata, buf + sizeof cmd,
 		   response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);

@@ -3387,14 +3456,17 @@ ssize_t ib_uverbs_query_srq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	srq = idr_read_srq(cmd.srq_handle, file->ucontext);
 	if (!srq)
 		return -EINVAL;

@@ -3435,14 +3507,17 @@ ssize_t ib_uverbs_destroy_srq(struct ib_uverbs_file *file,
 	if (in_len < sizeof cmd)
 		return -EINVAL;
 
-	if (out_len < sizeof resp)
-		return -ENOSPC;
-
 	if (copy_from_user(&cmd, buf, sizeof cmd))
 		return -EFAULT;
 
 	response = (void __user *)(unsigned long)cmd.response;
 
+	if (!access_ok(VERIFY_WRITE, response, out_len))
+		return -EFAULT;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
 	uobj = idr_write_uobj(&ib_uverbs_srq_idr, cmd.srq_handle, file->ucontext);
 	if (!uobj)
 		return -EINVAL;
Signature · 6 lines
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#8 lore raw
In reply to a message not in the archive
Quoted 5 lines · Yann Droneaud

Hi,

Please find a patchset against uverbs to improve the checks done on uverbs request parameters. This patchset in an extract of a previous patchset sent some times ago[1].

I reviewed those patches last time they came around. In general, I'm on board with the idea of cleaning up option checking, but in your original patch submission you point out that the patches have a non-0 chance of breaking user applications that pass in parameters that wouldn't pass these checks but work anyway. Have you done any looking into that possibility?

Quoted 43 lines

I've provided some explanation of the issues partialy addressed by this patchset in a previous message[2].

As we're now addressing overflows, I think it's time to apply this patchset.

Changes since v0 [3]
- updated against v4.1-rc2
- incorporated patches to add check on response buffer using access_ok()

[1] "[PATCH 00/22] infiniband: improve userspace input check"

http://marc.info/?i=cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

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

http://marc.info/?i=1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
http://mid.gmane.org/1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]

[3] "[PATCH 0/4] IB/uverbs: check request parameters"

http://marc.info/?i=cover.1405884453.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1405884453.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Yann Droneaud (6):
IB/uverbs: check userspace input buffer size
IB/uverbs: check userspace output buffer size
IB/uverbs: check userspace output buffer size in ib_uverbs_poll_cq() IB/uverbs: subtract command header from input size IB/uverbs: move cast from u64 to void __user pointer to its own
variable
IB/uverbs: check access to userspace response buffer

 drivers/infiniband/core/uverbs_cmd.c         | 449 +++++++++++++++++++++------
 drivers/infiniband/core/uverbs_main.c        |  29 +-
 drivers/infiniband/hw/mlx5/cq.c              |   6 +-
 drivers/infiniband/hw/mlx5/main.c            |   2 +-
 drivers/infiniband/hw/mlx5/srq.c             |   6 +-
 drivers/infiniband/hw/mthca/mthca_provider.c |   2 +-
 6 files changed, 382 insertions(+), 112 deletions(-)
Signature · 2 lines
Doug Ledford <[email protected]>
              GPG KeyID: 0E572FDD

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#10 lore raw

Hi,

Quoted 6 lines · Doug Ledford
Quoted 4 lines · Yann Droneaud

Please find a patchset against uverbs to improve the checks done on uverbs request parameters. This patchset in an extract of a previous patchset sent some times ago[1].

Quoted 7 lines

I reviewed those patches last time they came around. In general, I'm on board with the idea of cleaning up option checking, but in your original patch submission you point out that the patches have a non-0 chance of breaking user applications that pass in parameters that wouldn't pass these checks but work anyway. Have you done any looking into that possibility?

I believe applications using libibverbs are immune of any kind of regression.

But I cannot tell for sure for applications that access directly uverbs:
those could be doing broken things.

Anyway, we should prevent such applications to broke into the kernel:
integer over/under flow or buffer overflow are not acceptable.

Quoted 35 lines
Quoted 35 lines

I've provided some explanation of the issues partialy addressed by this patchset in a previous message[2].

As we're now addressing overflows, I think it's time to apply this patchset.

Changes since v0 [3]
- updated against v4.1-rc2
- incorporated patches to add check on response buffer using access_ok()

[1] "[PATCH 00/22] infiniband: improve userspace input check"

http://marc.info/?i=cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

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

http://marc.info/?i=1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWdBPR1lH4CV8@public.gmane.orgain
http://mid.gmane.org/1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWbh0DlRQRlKF@public.gmane.orgin

[3] "[PATCH 0/4] IB/uverbs: check request parameters"

http://marc.info/?i=cover.1405884453.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1405884453.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Yann Droneaud (6):
IB/uverbs: check userspace input buffer size
IB/uverbs: check userspace output buffer size
IB/uverbs: check userspace output buffer size in ib_uverbs_poll_cq() IB/uverbs: subtract command header from input size IB/uverbs: move cast from u64 to void __user pointer to its own
variable
IB/uverbs: check access to userspace response buffer

Quoted 10 lines
Quoted 8 lines
 drivers/infiniband/core/uverbs_cmd.c         | 449 +++++++++++++++++++++------
 drivers/infiniband/core/uverbs_main.c        |  29 +-
 drivers/infiniband/hw/mlx5/cq.c              |   6 +-
 drivers/infiniband/hw/mlx5/main.c            |   2 +-
 drivers/infiniband/hw/mlx5/srq.c             |   6 +-
 drivers/infiniband/hw/mthca/mthca_provider.c |   2 +-
 6 files changed, 382 insertions(+), 112 deletions(-)

Regards.

Signature · 8 lines
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#12 lore raw

Hi,

Quoted 15 lines · Yann Droneaud
Quoted 6 lines · Doug Ledford
Quoted 4 lines · Yann Droneaud

Please find a patchset against uverbs to improve the checks done on uverbs request parameters. This patchset in an extract of a previous patchset sent some times ago[1].

Quoted 7 lines

I reviewed those patches last time they came around. In general, I'm on board with the idea of cleaning up option checking, but in your original patch submission you point out that the patches have a non-0 chance of breaking user applications that pass in parameters that wouldn't pass these checks but work anyway. Have you done any looking into that possibility?

Quoted 9 lines

I believe applications using libibverbs are immune of any kind of regression.

But I cannot tell for sure for applications that access directly uverbs:
those could be doing broken things.

Anyway, we should prevent such applications to broke into the kernel:
integer over/under flow or buffer overflow are not acceptable.

I would also add, that, being stricter doesn't introduce a new, diverging behavior that newer applications could rely on, so if one existing application is found to be broken by the updated checks, those patchs could simply be reverted without falling into a compatibility nightmare where we would have to support two different behaviors.

Quoted 35 lines
Quoted 35 lines
Quoted 35 lines

I've provided some explanation of the issues partialy addressed by this patchset in a previous message[2].

As we're now addressing overflows, I think it's time to apply this patchset.

Changes since v0 [3]
- updated against v4.1-rc2
- incorporated patches to add check on response buffer using access_ok()

[1] "[PATCH 00/22] infiniband: improve userspace input check"

http://marc.info/?i=cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

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

http://marc.info/?i=1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWQ@public.gmane.orgomain
http://mid.gmane.org/1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWWGXanvQGlWp@public.gmane.orgmain

[3] "[PATCH 0/4] IB/uverbs: check request parameters"

http://marc.info/?i=cover.1405884453.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1405884453.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Yann Droneaud (6):
IB/uverbs: check userspace input buffer size
IB/uverbs: check userspace output buffer size
IB/uverbs: check userspace output buffer size in ib_uverbs_poll_cq() IB/uverbs: subtract command header from input size IB/uverbs: move cast from u64 to void __user pointer to its own
variable
IB/uverbs: check access to userspace response buffer

Quoted 11 lines
Quoted 10 lines
Quoted 8 lines
 drivers/infiniband/core/uverbs_cmd.c         | 449 +++++++++++++++++++++------
 drivers/infiniband/core/uverbs_main.c        |  29 +-
 drivers/infiniband/hw/mlx5/cq.c              |   6 +-
 drivers/infiniband/hw/mlx5/main.c            |   2 +-
 drivers/infiniband/hw/mlx5/srq.c             |   6 +-
 drivers/infiniband/hw/mthca/mthca_provider.c |   2 +-
 6 files changed, 382 insertions(+), 112 deletions(-)

Regards.

Signature · 9 lines
Yann Droneaud
OPTEYA



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html