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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Parav,
Thanks for your feedback.
As you mentioned below since addr4_resolve() or addr6_resolve() doesn't fail
,ndev is not NULL in mycase.
And the below simpler patch which was provided by you  holds good.

I will update the patch and will resend the same.
Will it be ok?


Regards,
Muneendra.



-----Original Message-----
From: Parav Pandit [mailto:parav@xxxxxxxxxxxx]
Sent: Friday, February 23, 2018 1:06 AM
To: Muneendra Kumar M <muneendra.kumar@xxxxxxxxxxxx>;
linux-rdma@xxxxxxxxxxxxxxx
Subject: RE: [PATCH] Bug[RDMA/core]:Null pointer check is missing in
addr_resolve

Hi Muneendra,

> -----Original Message-----
> From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Muneendra Kumar M
> Sent: Wednesday, February 21, 2018 4:21 AM
> To: linux-rdma@xxxxxxxxxxxxxxx
> 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.

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;
 }



>   /*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@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux