diff mbox

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

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

Commit Message

Leon Romanovsky May 4, 2016, 5:36 a.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>
Signed-off-by: Leon Romanovsky <leon@kernel.org>
---
 drivers/infiniband/core/cma.c      | 2 +-
 drivers/infiniband/core/iwcm.c     | 2 +-
 drivers/infiniband/core/sa_query.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Comments

Steve Wise May 4, 2016, 3:39 p.m. UTC | #1 
> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Leon Romanovsky
> Sent: Wednesday, May 04, 2016 12:36 AM
> To: dledford@redhat.com
> Cc: linux-rdma@vger.kernel.org; markb@mellanox.com; majd@mellanox.com;
> matanb@mellanox.com; Leon Romanovsky
> Subject: [PATCH rdma-next 3/4] IB/core: Fix a potential array overrun in CMA
and
> SA agent
> 
> 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>
> Signed-off-by: Leon Romanovsky <leon@kernel.org>
> ---
>  drivers/infiniband/core/cma.c      | 2 +-
>  drivers/infiniband/core/iwcm.c     | 2 +-
>  drivers/infiniband/core/sa_query.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 93ab0ae..09a0243 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -4240,7 +4240,7 @@ out:
>  	return skb->len;
>  }
> 
> -static const struct ibnl_client_cbs cma_cb_table[] = {
> +static const struct ibnl_client_cbs
> cma_cb_table[RDMA_NL_RDMA_CM_NUM_OPS] = {
>  	[RDMA_NL_RDMA_CM_ID_STATS] = { .dump = cma_get_id_stats,
>  				       .module = THIS_MODULE },
>  };
> 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..24c2aac 100644
> --- a/drivers/infiniband/core/sa_query.c
> +++ b/drivers/infiniband/core/sa_query.c
> @@ -838,7 +838,7 @@ resp_out:
>  	return skb->len;
>  }
> 
> -static struct ibnl_client_cbs ib_sa_cb_table[] = {
> +static struct ibnl_client_cbs ib_sa_cb_table[RDMA_NL_LS_NUM_OPS] = {
>  	[RDMA_NL_LS_OP_RESOLVE] = {
>  		.dump = ib_nl_handle_resolve_resp,
>  		.module = THIS_MODULE },

Looks ok.

Reviewed-by: Steve Wise <swise@opengridcomputing.com>

--
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
Jason Gunthorpe May 4, 2016, 6:19 p.m. UTC | #2 
On Wed, May 04, 2016 at 08:36:04AM +0300, Leon Romanovsky wrote:

> -static const struct ibnl_client_cbs cma_cb_table[] = {
> +static const struct ibnl_client_cbs cma_cb_table[RDMA_NL_RDMA_CM_NUM_OPS] = {
>  	[RDMA_NL_RDMA_CM_ID_STATS] = { .dump = cma_get_id_stats,
>  				       .module = THIS_MODULE },

Isn't it much better to change this:

        if (ibnl_add_client(RDMA_NL_RDMA_CM, RDMA_NL_RDMA_CM_NUM_OPS, cma_cb_table))

to 
        if (ibnl_add_client(RDMA_NL_RDMA_CM, NELEMS(cma_cb_table), cma_cb_table))

And eliminate RDMA_NL_RDMA_CM_NUM_OPS entirely?

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
Ira Weiny May 4, 2016, 9:37 p.m. UTC | #3 
On Wed, May 04, 2016 at 12:19:22PM -0600, Jason Gunthorpe wrote:
> On Wed, May 04, 2016 at 08:36:04AM +0300, Leon Romanovsky wrote:
> 
> > -static const struct ibnl_client_cbs cma_cb_table[] = {
> > +static const struct ibnl_client_cbs cma_cb_table[RDMA_NL_RDMA_CM_NUM_OPS] = {
> >  	[RDMA_NL_RDMA_CM_ID_STATS] = { .dump = cma_get_id_stats,
> >  				       .module = THIS_MODULE },
> 
> Isn't it much better to change this:
> 
>         if (ibnl_add_client(RDMA_NL_RDMA_CM, RDMA_NL_RDMA_CM_NUM_OPS, cma_cb_table))
> 
> to 
>         if (ibnl_add_client(RDMA_NL_RDMA_CM, NELEMS(cma_cb_table), cma_cb_table))
> 
> And eliminate RDMA_NL_RDMA_CM_NUM_OPS entirely?

Well based on their other series I think we may need to revisit the design
pattern of netlink altogether.  But yea if we just want to fix this we can do
this.

Ira

> 
> 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
--
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
diff mbox

Patch

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 93ab0ae..09a0243 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -4240,7 +4240,7 @@  out:
 	return skb->len;
 }
 
-static const struct ibnl_client_cbs cma_cb_table[] = {
+static const struct ibnl_client_cbs cma_cb_table[RDMA_NL_RDMA_CM_NUM_OPS] = {
 	[RDMA_NL_RDMA_CM_ID_STATS] = { .dump = cma_get_id_stats,
 				       .module = THIS_MODULE },
 };
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..24c2aac 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -838,7 +838,7 @@  resp_out:
 	return skb->len;
 }
 
-static struct ibnl_client_cbs ib_sa_cb_table[] = {
+static struct ibnl_client_cbs ib_sa_cb_table[RDMA_NL_LS_NUM_OPS] = {
 	[RDMA_NL_LS_OP_RESOLVE] = {
 		.dump = ib_nl_handle_resolve_resp,
 		.module = THIS_MODULE },