[PATCH 0/3] Address three Coverity complaints about RDMA code

[PATCH 0/3] Address three Coverity complaints about RDMA code

[Bart Van Assche]

Hello Doug,

The three patches in this series address warnings reported by Coverity 
on the RDMA code. Please consider these patches for kernel version v4.10.

Thanks,

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

[PATCH 1/3] IB/mad: Fix an array index check

[Bart Van Assche]

The array ib_mad_mgmt_class_table.method_table has MAX_MGMT_CLASS
(80) elements. Hence compare the array index with that value instead
of with IB_MGMT_MAX_METHODS (128). This patch avoids that Coverity
reports the following:

Overrunning array class->method_table of 80 8-byte elements at element index 127 (byte offset 1016) using index convert_mgmt_class(mad_hdr->mgmt_class) (which evaluates to 127).

Fixes: commit b7ab0b19a85f ("IB/mad: Verify mgmt class in received MADs")
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/core/mad.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 40cbd6b..2395fe2 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -1746,7 +1746,7 @@ find_mad_agent(struct ib_mad_port_private *port_priv,
 			if (!class)
 				goto out;
 			if (convert_mgmt_class(mad_hdr->mgmt_class) >=
-			    IB_MGMT_MAX_METHODS)
+			    ARRAY_SIZE(class->method_table))
 				goto out;
 			method = class->method_table[convert_mgmt_class(
 							mad_hdr->mgmt_class)];
-- 
2.10.2


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

Re: [PATCH 1/3] IB/mad: Fix an array index check

[Hal Rosenstock]

On 11/21/2016 1:21 PM, Bart Van Assche wrote:
> The array ib_mad_mgmt_class_table.method_table has MAX_MGMT_CLASS
> (80) elements. Hence compare the array index with that value instead
> of with IB_MGMT_MAX_METHODS (128). This patch avoids that Coverity
> reports the following:
> 
> Overrunning array class->method_table of 80 8-byte elements at element index 127 (byte offset 1016) using index convert_mgmt_class(mad_hdr->mgmt_class) (which evaluates to 127).
> 
> Fixes: commit b7ab0b19a85f ("IB/mad: Verify mgmt class in received MADs")
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>

Reviewed-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

> Cc: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
>  drivers/infiniband/core/mad.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
> index 40cbd6b..2395fe2 100644
> --- a/drivers/infiniband/core/mad.c
> +++ b/drivers/infiniband/core/mad.c
> @@ -1746,7 +1746,7 @@ find_mad_agent(struct ib_mad_port_private *port_priv,
>  			if (!class)
>  				goto out;
>  			if (convert_mgmt_class(mad_hdr->mgmt_class) >=
> -			    IB_MGMT_MAX_METHODS)
> +			    ARRAY_SIZE(class->method_table))
>  				goto out;
>  			method = class->method_table[convert_mgmt_class(
>  							mad_hdr->mgmt_class)];
> 

I think there is also similar thing in missing check in ib_register_mad_agent where:

        /*
         * Make sure MAD registration (if supplied)
         * is non overlapping with any existing ones
         */
        if (mad_reg_req) {
                mgmt_class = convert_mgmt_class(mad_reg_req->mgmt_class);
                if (!is_vendor_class(mgmt_class)) {
                        class = port_priv->version[mad_reg_req->
                                                   mgmt_class_version].class;
                        if (class) {
                                method = class->method_table[mgmt_class];

so here the class' method_table is also accessed without checking mgmt_class for range violation, right ?

-- Hal

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

Re: [PATCH 1/3] IB/mad: Fix an array index check

[Bart Van Assche]

On 11/21/2016 12:17 PM, Hal Rosenstock wrote:
> I think there is also similar thing in missing check in ib_register_mad_agent where:
> 
>         /*
>          * Make sure MAD registration (if supplied)
>          * is non overlapping with any existing ones
>          */
>         if (mad_reg_req) {
>                 mgmt_class = convert_mgmt_class(mad_reg_req->mgmt_class);
>                 if (!is_vendor_class(mgmt_class)) {
>                         class = port_priv->version[mad_reg_req->
>                                                    mgmt_class_version].class;
>                         if (class) {
>                                 method = class->method_table[mgmt_class];
> 
> so here the class' method_table is also accessed without checking mgmt_class for range violation, right ?

Hello Hal,

I think such a check is already present in ib_register_mad_agent():

		if (mad_reg_req->mgmt_class >= MAX_MGMT_CLASS) {
			/*
			 * IB_MGMT_CLASS_SUBN_DIRECTED_ROUTE is the only
			 * one in this range currently allowed
			 */
			if (mad_reg_req->mgmt_class !=
			    IB_MGMT_CLASS_SUBN_DIRECTED_ROUTE) {
				dev_notice(&device->dev,
					   "%s: Invalid Mgmt Class 0x%x\n",
					   __func__, mad_reg_req->mgmt_class);
				goto error1;
			}
		} [ ... ]

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

Re: [PATCH 1/3] IB/mad: Fix an array index check

[Hal Rosenstock]

Hi Bart,

On 11/21/2016 3:52 PM, Bart Van Assche wrote:
> Hello Hal,
> 
> I think such a check is already present in ib_register_mad_agent():
> 
> 		if (mad_reg_req->mgmt_class >= MAX_MGMT_CLASS) {

You're right.

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

RE: [PATCH 1/3] IB/mad: Fix an array index check

[Weiny, Ira]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1726 bytes --]

> 
> The array ib_mad_mgmt_class_table.method_table has MAX_MGMT_CLASS
> (80) elements. Hence compare the array index with that value instead of with
> IB_MGMT_MAX_METHODS (128). This patch avoids that Coverity reports the
> following:
> 
> Overrunning array class->method_table of 80 8-byte elements at element index
> 127 (byte offset 1016) using index convert_mgmt_class(mad_hdr-
> >mgmt_class) (which evaluates to 127).
> 
> Fixes: commit b7ab0b19a85f ("IB/mad: Verify mgmt class in received MADs")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Sean Hefty <sean.hefty@intel.com>
> Cc: <stable@vger.kernel.org>

Thanks!

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/infiniband/core/mad.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c index
> 40cbd6b..2395fe2 100644
> --- a/drivers/infiniband/core/mad.c
> +++ b/drivers/infiniband/core/mad.c
> @@ -1746,7 +1746,7 @@ find_mad_agent(struct ib_mad_port_private
> *port_priv,
>  			if (!class)
>  				goto out;
>  			if (convert_mgmt_class(mad_hdr->mgmt_class) >=
> -			    IB_MGMT_MAX_METHODS)
> +			    ARRAY_SIZE(class->method_table))
>  				goto out;
>  			method = class->method_table[convert_mgmt_class(
>  							mad_hdr-
> >mgmt_class)];
> --
> 2.10.2
> 
> 
> --
> 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
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

[PATCH 2/3] IPoIB: Avoid reading an uninitialized member variable

[Bart Van Assche]

This patch avoids that Coverity reports the following:

    Using uninitialized value port_attr.state when calling printk

Fixes: commit 94232d9ce817 ("IPoIB: Start multicast join process only on active ports")
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 1909dd2..fddff40 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -575,8 +575,11 @@ void ipoib_mcast_join_task(struct work_struct *work)
 	if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
 		return;
 
-	if (ib_query_port(priv->ca, priv->port, &port_attr) ||
-	    port_attr.state != IB_PORT_ACTIVE) {
+	if (ib_query_port(priv->ca, priv->port, &port_attr)) {
+		ipoib_dbg(priv, "ib_query_port() failed\n");
+		return;
+	}
+	if (port_attr.state != IB_PORT_ACTIVE) {
 		ipoib_dbg(priv, "port state is not ACTIVE (state = %d) suspending join task\n",
 			  port_attr.state);
 		return;
-- 
2.10.2

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

Re: [PATCH 2/3] IPoIB: Avoid reading an uninitialized member variable

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]

On Mon, Nov 21, 2016 at 10:21:41AM -0800, Bart Van Assche wrote:
> This patch avoids that Coverity reports the following:
>
>     Using uninitialized value port_attr.state when calling printk
>
> Fixes: commit 94232d9ce817 ("IPoIB: Start multicast join process only on active ports")
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

Except that it doesn't print the reason why ib_query_port failed,
it look good.
Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Thanks

> ---
>  drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> index 1909dd2..fddff40 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> @@ -575,8 +575,11 @@ void ipoib_mcast_join_task(struct work_struct *work)
>  	if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
>  		return;
>
> -	if (ib_query_port(priv->ca, priv->port, &port_attr) ||
> -	    port_attr.state != IB_PORT_ACTIVE) {
> +	if (ib_query_port(priv->ca, priv->port, &port_attr)) {
> +		ipoib_dbg(priv, "ib_query_port() failed\n");
> +		return;
> +	}
> +	if (port_attr.state != IB_PORT_ACTIVE) {
>  		ipoib_dbg(priv, "port state is not ACTIVE (state = %d) suspending join task\n",
>  			  port_attr.state);
>  		return;
> --
> 2.10.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/3] IB/multicast: Check ib_find_pkey() return value

[Bart Van Assche]

This patch avoids that Coverity complains about not checking the
ib_find_pkey() return value.

Fixes: commit 547af76521b3 ("IB/multicast: Report errors on multicast groups if P_key changes")
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/core/multicast.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c
index e51b739..322cb67 100644
--- a/drivers/infiniband/core/multicast.c
+++ b/drivers/infiniband/core/multicast.c
@@ -518,8 +518,11 @@ static void join_handler(int status, struct ib_sa_mcmember_rec *rec,
 		process_join_error(group, status);
 	else {
 		int mgids_changed, is_mgid0;
-		ib_find_pkey(group->port->dev->device, group->port->port_num,
-			     be16_to_cpu(rec->pkey), &pkey_index);
+
+		if (ib_find_pkey(group->port->dev->device,
+				 group->port->port_num, be16_to_cpu(rec->pkey),
+				 &pkey_index))
+			pkey_index = MCAST_INVALID_PKEY_INDEX;
 
 		spin_lock_irq(&group->port->lock);
 		if (group->state == MCAST_BUSY &&
-- 
2.10.2

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

Re: [PATCH 3/3] IB/multicast: Check ib_find_pkey() return value

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

On Mon, Nov 21, 2016 at 10:22:17AM -0800, Bart Van Assche wrote:
> This patch avoids that Coverity complains about not checking the
> ib_find_pkey() return value.
>
> Fixes: commit 547af76521b3 ("IB/multicast: Report errors on multicast groups if P_key changes")
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
>  drivers/infiniband/core/multicast.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c
> index e51b739..322cb67 100644
> --- a/drivers/infiniband/core/multicast.c
> +++ b/drivers/infiniband/core/multicast.c
> @@ -518,8 +518,11 @@ static void join_handler(int status, struct ib_sa_mcmember_rec *rec,
>  		process_join_error(group, status);
>  	else {
>  		int mgids_changed, is_mgid0;
> -		ib_find_pkey(group->port->dev->device, group->port->port_num,
> -			     be16_to_cpu(rec->pkey), &pkey_index);
> +
> +		if (ib_find_pkey(group->port->dev->device,
> +				 group->port->port_num, be16_to_cpu(rec->pkey),
> +				 &pkey_index))
> +			pkey_index = MCAST_INVALID_PKEY_INDEX;

The coverity warning is valid, ib_find_pkey() can return ENOMEM
(ib_find_pkey()->ib_query_pkey()->(device->query_pkey())->mlx5_ib_query_pkey()->mlx5_query_mad_ifc_pkey()/mlx5_query_hca_vport_pkey())

I'm not sure that the error handling proposed is enough, pkey_index is initialized to
MCAST_INVALID_PKEY_INDEX at the beginning of function and in case of error it preserves
its value.

Don't you need to update process_group_error() function? It has code
flows with ib_find_pkey() without checking return value, which can be
anything too.

Thanks

>
>  		spin_lock_irq(&group->port->lock);
>  		if (group->state == MCAST_BUSY &&
> --
> 2.10.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] IB/multicast: Check ib_find_pkey() return value

[Bart Van Assche]

On 11/21/2016 11:11 PM, Leon Romanovsky wrote:
> Don't you need to update process_group_error() function? It has code
> flows with ib_find_pkey() without checking return value, which can be
> anything too.

Hello Leon,

I'll leave that to someone who is familiar with the multicast code.

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

Re: [PATCH 0/3] Address three Coverity complaints about RDMA code

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 365 bytes --]

On 11/21/2016 1:20 PM, Bart Van Assche wrote:
> Hello Doug,
> 
> The three patches in this series address warnings reported by Coverity
> on the RDMA code. Please consider these patches for kernel version v4.10.
> 
> Thanks,
> 
> Bart.

Thanks, applied.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]