diff mbox

Bug[RDMA/core]:Null pointer check is missing in addr_resolve

Message ID VI1PR0502MB3008430724D241AFE657BE8BD1CD0@VI1PR0502MB3008.eurprd05.prod.outlook.com (mailing list archive)
State Superseded
Headers show

Commit Message

Parav Pandit Feb. 22, 2018, 7:35 p.m. UTC 
Hi Muneendra,

> -----Original Message-----

> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-

> owner@vger.kernel.org] On Behalf Of Muneendra Kumar M

> Sent: Wednesday, February 21, 2018 4:21 AM

> To: linux-rdma@vger.kernel.org

> Subject: [PATCH] Bug[RDMA/core]:Null pointer check is missing in addr_resolve

> 

> Null pointer check is missing in addr_resolve as dev_get_by_index may return a

> NULL pointer.

> And this patch will check whether ndev is NULL and further access the same .

> We observed the issue where the sytem crashed in the below code

> 

> if (ndev->flags & IFF_LOOPBACK)

>   {

>     ret = rdma_translate_ip (dst_in, addr, NULL);

>     /*

>      * Put the loopback device and get the translated

>      * device instead.

>      */

>     dev_put (ndev);

>     ndev = dev_get_by_index (addr->net, addr->bound_dev_if);

Above ndev assignment is of no use. It just serves the purpose of keeping dev_put().
So a simpler patch would be possibly below.



>   /*Bug: dev_get_by_index returns null*/ } else

>   {

>     addr->bound_dev_if = ndev->ifindex;

>   }

> 

> dev_put (ndev) <== system crashed

> And below is the crash

> 

> [  146.173149] BUG: unable to handle kernel NULL pointer dereference at

> 00000000000004a0

> [  146.173198] IP: addr_resolve+0x9e/0x3e0 [ib_core] [  146.173221] PGD 0 P4D

> 0 [  146.173869] Oops: 0000 [#1] SMP PTI

> [  146.182859] CPU: 8 PID: 127 Comm: kworker/8:1 Tainted: G           O

> 4.15.0-rc6+ #18

> [  146.183758] Hardware name: LENOVO System x3650 M5: -[8871AC1]-

> /01KN179,

> BIOS-[TCE132H-2.50]- 10/11/2017

> [  146.184691] Workqueue: ib_cm cm_work_handler [ib_cm] [  146.185632] RIP:

> 0010:addr_resolve+0x9e/0x3e0 [ib_core] [  146.186584] RSP:

> 0018:ffffc9000362faa0 EFLAGS: 00010246 [  146.187521] RAX:

> 000000000000001b RBX: ffffc9000362fc08 RCX:

> 0000000000000006

> [  146.188472] RDX: 0000000000000000 RSI: 0000000000000096 RDI:

> ffff88087fc16990

> [  146.189427] RBP: ffffc9000362fb18 R08: 00000000ffffff9d R09:

> 00000000000004ac

> [  146.190392] R10: 00000000000001e7 R11: 0000000000000001 R12:

> ffff88086af2e090

> [  146.191361] R13: 0000000000000000 R14: 0000000000000001 R15:

> 00000000ffffff9d

> [  146.192327] FS:  0000000000000000(0000) GS:ffff88087fc00000(0000)

> knlGS:0000000000000000

> [  146.193301] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [

> 146.194274] CR2: 00000000000004a0 CR3: 000000000220a002 CR4:

> 00000000003606e0

> [  146.195258] DR0: 0000000000000000 DR1: 0000000000000000 DR2:

> 0000000000000000

> [  146.196256] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:

> 0000000000000400

> [  146.197231] Call Trace:

> [  146.198209]  ? rdma_addr_register_client+0x30/0x30 [ib_core] [  146.199199]

> rdma_resolve_ip+0x1af/0x280 [ib_core] [  146.200196]

> rdma_addr_find_l2_eth_by_grh+0x154/0x2b0 [ib_core]

> 

> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

> ---

>  drivers/infiniband/core/addr.c | 13 +++++++++----

>  1 file changed, 9 insertions(+), 4 deletions(-)

> 

> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c index

> a5b4cf0..d61ed45 100644

> --- a/drivers/infiniband/core/addr.c

> +++ b/drivers/infiniband/core/addr.c

> @@ -523,7 +523,8 @@ static int addr_resolve(struct sockaddr *src_in,

>  			ndev = dev_get_by_index(addr->net,

> addr->bound_dev_if);

>  		} else {

>  			ndev = rt->dst.dev;

> -			dev_hold(ndev);

> +			if (ndev)

> +				dev_hold(ndev);

Given that if addr4_resolve() or addr6_resolve() fails, rt and dst entries are not accessed,
this ndev should not be NULL, do you see this NULL too?

>  		}

> 

>  		ip_rt_put(rt);

> @@ -544,7 +545,8 @@ static int addr_resolve(struct sockaddr *src_in,

>  			ndev = dev_get_by_index(addr->net,

> addr->bound_dev_if);

>  		} else {

>  			ndev = dst->dev;

> -			dev_hold(ndev);

> +			if (ndev)

> +				dev_hold(ndev);

>  		}

> 

>  		dst_release(dst);

> @@ -556,12 +558,15 @@ static int addr_resolve(struct sockaddr *src_in,

>  		 * Put the loopback device and get the translated

>  		 * device instead.

>  		 */

> -		dev_put(ndev);

> +		if (ndev)

> +			dev_put(ndev);

>  		ndev = dev_get_by_index(addr->net, addr->bound_dev_if);

>  	} else {

>  		addr->bound_dev_if = ndev->ifindex;

>  	}

> -	dev_put(ndev);

> +	/*The ndev could be null */

> +	if (ndev)

> +		dev_put(ndev);

> 

>  	return ret;

>  }

> --

> 1.8.3.1

> --

> 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/addr.c b/drivers/infiniband/core/addr.c
index a039a49..4af043e 100644
--- a/drivers/infiniband/core/addr.c
+++ b/drivers/infiniband/core/addr.c
@@ -550,19 +550,13 @@  static int addr_resolve(struct sockaddr *src_in,
                dst_release(dst);
        }

-       if (ndev->flags & IFF_LOOPBACK) {
-               ret = rdma_translate_ip(dst_in, addr);
-               /*
-                * Put the loopback device and get the translated
-                * device instead.
-                */
+       if (ndev) {
+               if (ndev->flags & IFF_LOOPBACK)
+                       ret = rdma_translate_ip(dst_in, addr);
+               else
+                       addr->bound_dev_if = ndev->ifindex;
                dev_put(ndev);
-               ndev = dev_get_by_index(addr->net, addr->bound_dev_if);
-       } else {
-               addr->bound_dev_if = ndev->ifindex;
        }
-       dev_put(ndev);
-
        return ret;
 }