diff mbox

[rdma-next,V1,3/5] IB/core: Fix a potential array overrun in CMA and SA agent

Message ID 1462540026-12012-4-git-send-email-leon@kernel.org (mailing list archive)
State Superseded
Headers show

Commit Message

Leon Romanovsky May 6, 2016, 1:07 p.m. UTC 
From: Mark Bloch <markb@mellanox.com>

Fix array overrun when going over callback table.
In declaration of callback table, the max size isn't provided and
in registration phase, it is provided.

There is potential scenario where a new operation is added
and it is not supported by current client. The acceptance of
such operation by ib_netlink will cause to array overrun.

Fixes: 809d5fc9bf65 ("infiniband: pass rdma_cm module to netlink_dump_start")
Fixes: b493d91d333e ("iwcm: common code for port mapper")
Fixes: 2ca546b92a02 ("IB/sa: Route SA pathrecord query through netlink")
Signed-off-by: Mark Bloch <markb@mellanox.com>
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
Signed-off-by: Leon Romanovsky <leon@kernel.org>
---
 drivers/infiniband/core/cma.c      | 3 ++-
 drivers/infiniband/core/iwcm.c     | 2 +-
 drivers/infiniband/core/sa_query.c | 2 +-
 include/uapi/rdma/rdma_netlink.h   | 8 +++-----
 4 files changed, 7 insertions(+), 8 deletions(-)

Comments

Jason Gunthorpe May 6, 2016, 6:23 p.m. UTC | #1 
On Fri, May 06, 2016 at 04:07:04PM +0300, Leon Romanovsky wrote:
> -static struct ibnl_client_cbs iwcm_nl_cb_table[] = {
> +static struct ibnl_client_cbs iwcm_nl_cb_table[RDMA_NL_IWPM_NUM_OPS] = {

Why keep this hunk instead of using ARRAY_SIZE?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Leon Romanovsky May 6, 2016, 7:40 p.m. UTC | #2 
On Fri, May 06, 2016 at 12:23:21PM -0600, Jason Gunthorpe wrote:
> On Fri, May 06, 2016 at 04:07:04PM +0300, Leon Romanovsky wrote:
> > -static struct ibnl_client_cbs iwcm_nl_cb_table[] = {
> > +static struct ibnl_client_cbs iwcm_nl_cb_table[RDMA_NL_IWPM_NUM_OPS] = {
> 
> Why keep this hunk instead of using ARRAY_SIZE?

I tried to minimize my changes to original author patch. I'm sending
updated version.

Thanks for review.

> 
> Jason
diff mbox

Patch

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 93ab0ae..b575bd5 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -4294,7 +4294,8 @@  static int __init cma_init(void)
 	if (ret)
 		goto err;
 
-	if (ibnl_add_client(RDMA_NL_RDMA_CM, RDMA_NL_RDMA_CM_NUM_OPS, cma_cb_table))
+	if (ibnl_add_client(RDMA_NL_RDMA_CM, ARRAY_SIZE(cma_cb_table),
+			    cma_cb_table))
 		pr_warn("RDMA CMA: failed to add netlink callback\n");
 	cma_configfs_init();
 
diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index e28a160..5011ecf 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -59,7 +59,7 @@  MODULE_AUTHOR("Tom Tucker");
 MODULE_DESCRIPTION("iWARP CM");
 MODULE_LICENSE("Dual BSD/GPL");
 
-static struct ibnl_client_cbs iwcm_nl_cb_table[] = {
+static struct ibnl_client_cbs iwcm_nl_cb_table[RDMA_NL_IWPM_NUM_OPS] = {
 	[RDMA_NL_IWPM_REG_PID] = {.dump = iwpm_register_pid_cb},
 	[RDMA_NL_IWPM_ADD_MAPPING] = {.dump = iwpm_add_mapping_cb},
 	[RDMA_NL_IWPM_QUERY_MAPPING] = {.dump = iwpm_add_and_query_mapping_cb},
diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 8a09c0f..1e7c652 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -1820,7 +1820,7 @@  static int __init ib_sa_init(void)
 		goto err3;
 	}
 
-	if (ibnl_add_client(RDMA_NL_LS, RDMA_NL_LS_NUM_OPS,
+	if (ibnl_add_client(RDMA_NL_LS, ARRAY_SIZE(ib_sa_cb_table),
 			    ib_sa_cb_table)) {
 		pr_err("Failed to add netlink callback\n");
 		ret = -EINVAL;
diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
index 6e373d1..acd175d 100644
--- a/include/uapi/rdma/rdma_netlink.h
+++ b/include/uapi/rdma/rdma_netlink.h
@@ -24,8 +24,7 @@  enum {
 #define RDMA_NL_GET_TYPE(client, op) ((client << 10) + op)
 
 enum {
-	RDMA_NL_RDMA_CM_ID_STATS = 0,
-	RDMA_NL_RDMA_CM_NUM_OPS
+	RDMA_NL_RDMA_CM_ID_STATS
 };
 
 enum {
@@ -137,9 +136,8 @@  enum {
  *   SET_TIMEOUT - The local service requests the client to set the timeout.
  */
 enum {
-	RDMA_NL_LS_OP_RESOLVE = 0,
-	RDMA_NL_LS_OP_SET_TIMEOUT,
-	RDMA_NL_LS_NUM_OPS
+	RDMA_NL_LS_OP_RESOLVE,
+	RDMA_NL_LS_OP_SET_TIMEOUT
 };
 
 /* Local service netlink message flags */