[PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Gustavo A. R. Silva]

In case the message header and payload cannot be stored, function
nlmsg_put returns null.

Fix this by adding multiple sanity checks and avoid a potential
null dereference on _nlh_ when calling nlmsg_end.

Addresses-Coverity-ID: 1454215 ("Dereference null return value")
Addresses-Coverity-ID: 1454223 ("Dereference null return value")
Addresses-Coverity-ID: 1454224 ("Dereference null return value")
Addresses-Coverity-ID: 1464669 ("Dereference null return value")
Addresses-Coverity-ID: 1464670 ("Dereference null return value")
Addresses-Coverity-ID: 1464672 ("Dereference null return value")
Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit implementation")
Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit callback")
Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit implementation")
Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP information")
Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource utilization")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
index 5326a68..dc8f6eb 100644
--- a/drivers/infiniband/core/nldev.c
+++ b/drivers/infiniband/core/nldev.c
@@ -313,6 +313,11 @@ static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
 			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET),
 			0, 0);
+	if (!nlh) {
+		err = -EMSGSIZE;
+		goto err_free;
+
+	}
 
 	err = fill_dev_info(msg, device);
 	if (err)
@@ -344,6 +349,8 @@ static int _nldev_get_dumpit(struct ib_device *device,
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET),
 			0, NLM_F_MULTI);
+	if (!nlh)
+		goto out;
 
 	if (fill_dev_info(skb, device)) {
 		nlmsg_cancel(skb, nlh);
@@ -354,7 +361,8 @@ static int _nldev_get_dumpit(struct ib_device *device,
 
 	idx++;
 
-out:	cb->args[0] = idx;
+out:
+	cb->args[0] = idx;
 	return skb->len;
 }
 
@@ -404,6 +412,10 @@ static int nldev_port_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
 			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET),
 			0, 0);
+	if (!nlh) {
+		err = -EMSGSIZE;
+		goto err_free;
+	}
 
 	err = fill_port_info(msg, device, port);
 	if (err)
@@ -464,6 +476,8 @@ static int nldev_port_get_dumpit(struct sk_buff *skb,
 				RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
 						 RDMA_NLDEV_CMD_PORT_GET),
 				0, NLM_F_MULTI);
+		if (!nlh)
+			goto out;
 
 		if (fill_port_info(skb, device, p)) {
 			nlmsg_cancel(skb, nlh);
@@ -507,6 +521,10 @@ static int nldev_res_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
 			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_GET),
 			0, 0);
+	if (!nlh) {
+		ret = -EMSGSIZE;
+		goto err_free;
+	}
 
 	ret = fill_res_info(msg, device);
 	if (ret)
@@ -537,6 +555,8 @@ static int _nldev_res_get_dumpit(struct ib_device *device,
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_GET),
 			0, NLM_F_MULTI);
+	if (!nlh)
+		goto out;
 
 	if (fill_res_info(skb, device)) {
 		nlmsg_cancel(skb, nlh);
@@ -603,6 +623,10 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb,
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_QP_GET),
 			0, NLM_F_MULTI);
+	if (!nlh) {
+		ret = -EMSGSIZE;
+		goto err_index;
+	}
 
 	if (fill_nldev_handle(skb, device)) {
 		ret = -EMSGSIZE;
-- 
2.7.4

Re: [PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Leon Romanovsky]

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

On Fri, Feb 09, 2018 at 12:37:02AM -0600, Gustavo A. R. Silva wrote:
> In case the message header and payload cannot be stored, function
> nlmsg_put returns null.
>
> Fix this by adding multiple sanity checks and avoid a potential
> null dereference on _nlh_ when calling nlmsg_end.
>
> Addresses-Coverity-ID: 1454215 ("Dereference null return value")
> Addresses-Coverity-ID: 1454223 ("Dereference null return value")
> Addresses-Coverity-ID: 1454224 ("Dereference null return value")
> Addresses-Coverity-ID: 1464669 ("Dereference null return value")
> Addresses-Coverity-ID: 1464670 ("Dereference null return value")
> Addresses-Coverity-ID: 1464672 ("Dereference null return value")
> Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit implementation")
> Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit callback")
> Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit implementation")
> Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP information")
> Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource utilization")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>

It will be much better to fix the tool instead of fixing ghost case.
This scenario is impossible for all those flows.
We can receive the skv/msg in two ways:
 * First by allocating new message with NLMSG_DEFAULT_SIZE, which has more room
   than nlmsg_total_size(payload), payload is 0.
 * Second by getting from netlink.c and it will be at least "struct nlmsghdr" too.

Can you please add this info to the commit message?

Thanks

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

Re: [PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Gustavo A. R. Silva]

Hi Leon,

Quoting Leon Romanovsky <leon@kernel.org>:

> On Fri, Feb 09, 2018 at 12:37:02AM -0600, Gustavo A. R. Silva wrote:
>> In case the message header and payload cannot be stored, function
>> nlmsg_put returns null.
>>
>> Fix this by adding multiple sanity checks and avoid a potential
>> null dereference on _nlh_ when calling nlmsg_end.
>>
>> Addresses-Coverity-ID: 1454215 ("Dereference null return value")
>> Addresses-Coverity-ID: 1454223 ("Dereference null return value")
>> Addresses-Coverity-ID: 1454224 ("Dereference null return value")
>> Addresses-Coverity-ID: 1464669 ("Dereference null return value")
>> Addresses-Coverity-ID: 1464670 ("Dereference null return value")
>> Addresses-Coverity-ID: 1464672 ("Dereference null return value")
>> Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit implementation")
>> Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit callback")
>> Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit implementation")
>> Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP information")
>> Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource utilization")
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>  drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>
>
> It will be much better to fix the tool instead of fixing ghost case.
> This scenario is impossible for all those flows.
> We can receive the skv/msg in two ways:
>  * First by allocating new message with NLMSG_DEFAULT_SIZE, which  
> has more room
>    than nlmsg_total_size(payload), payload is 0.
>  * Second by getting from netlink.c and it will be at least "struct  
> nlmsghdr" too.
>
> Can you please add this info to the commit message?
>

Actually, I was planing to send a new version of this patch. This time  
using the unlikely macro for all the null checks on nlh.

What do you think?

Thanks
--
Gustavo

Re: [PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Leon Romanovsky]

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

On Fri, Feb 09, 2018 at 07:36:49AM -0600, Gustavo A. R. Silva wrote:
> Hi Leon,
>
> Quoting Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>
> > On Fri, Feb 09, 2018 at 12:37:02AM -0600, Gustavo A. R. Silva wrote:
> > > In case the message header and payload cannot be stored, function
> > > nlmsg_put returns null.
> > >
> > > Fix this by adding multiple sanity checks and avoid a potential
> > > null dereference on _nlh_ when calling nlmsg_end.
> > >
> > > Addresses-Coverity-ID: 1454215 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1454223 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1454224 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1464669 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1464670 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1464672 ("Dereference null return value")
> > > Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit implementation")
> > > Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit callback")
> > > Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit implementation")
> > > Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP information")
> > > Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource utilization")
> > > Signed-off-by: Gustavo A. R. Silva <gustavo-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
> > > ---
> > >  drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > >
> >
> > It will be much better to fix the tool instead of fixing ghost case.
> > This scenario is impossible for all those flows.
> > We can receive the skv/msg in two ways:
> >  * First by allocating new message with NLMSG_DEFAULT_SIZE, which has
> > more room
> >    than nlmsg_total_size(payload), payload is 0.
> >  * Second by getting from netlink.c and it will be at least "struct
> > nlmsghdr" too.
> >
> > Can you please add this info to the commit message?
> >
>
> Actually, I was planing to send a new version of this patch. This time using
> the unlikely macro for all the null checks on nlh.
>
> What do you think?

It is not datapath, so "unlikely" is not needed. Let's assume that smart enough
compiler will optimize such flow anyway, because nlmsg_put returns NULL
in unlikely scenario, so this check will be unlikely automatically too.

Thanks

>
> Thanks
> --
> Gustavo
>
>
>
>
>
>

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

Re: [PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Leon Romanovsky]

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

On Fri, Feb 09, 2018 at 07:36:49AM -0600, Gustavo A. R. Silva wrote:
> Hi Leon,
>
> Quoting Leon Romanovsky <leon@kernel.org>:
>
> > On Fri, Feb 09, 2018 at 12:37:02AM -0600, Gustavo A. R. Silva wrote:
> > > In case the message header and payload cannot be stored, function
> > > nlmsg_put returns null.
> > >
> > > Fix this by adding multiple sanity checks and avoid a potential
> > > null dereference on _nlh_ when calling nlmsg_end.
> > >
> > > Addresses-Coverity-ID: 1454215 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1454223 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1454224 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1464669 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1464670 ("Dereference null return value")
> > > Addresses-Coverity-ID: 1464672 ("Dereference null return value")
> > > Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit implementation")
> > > Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit callback")
> > > Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit implementation")
> > > Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP information")
> > > Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource utilization")
> > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > > ---
> > >  drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > >
> >
> > It will be much better to fix the tool instead of fixing ghost case.
> > This scenario is impossible for all those flows.
> > We can receive the skv/msg in two ways:
> >  * First by allocating new message with NLMSG_DEFAULT_SIZE, which has
> > more room
> >    than nlmsg_total_size(payload), payload is 0.
> >  * Second by getting from netlink.c and it will be at least "struct
> > nlmsghdr" too.
> >
> > Can you please add this info to the commit message?
> >
>
> Actually, I was planing to send a new version of this patch. This time using
> the unlikely macro for all the null checks on nlh.
>
> What do you think?

It is not datapath, so "unlikely" is not needed. Let's assume that smart enough
compiler will optimize such flow anyway, because nlmsg_put returns NULL
in unlikely scenario, so this check will be unlikely automatically too.

Thanks

>
> Thanks
> --
> Gustavo
>
>
>
>
>
>

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

Re: [PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Gustavo A. R. Silva]

Quoting Leon Romanovsky <leon@kernel.org>:

> On Fri, Feb 09, 2018 at 07:36:49AM -0600, Gustavo A. R. Silva wrote:
>> Hi Leon,
>>
>> Quoting Leon Romanovsky <leon@kernel.org>:
>>
>> > On Fri, Feb 09, 2018 at 12:37:02AM -0600, Gustavo A. R. Silva wrote:
>> > > In case the message header and payload cannot be stored, function
>> > > nlmsg_put returns null.
>> > >
>> > > Fix this by adding multiple sanity checks and avoid a potential
>> > > null dereference on _nlh_ when calling nlmsg_end.
>> > >
>> > > Addresses-Coverity-ID: 1454215 ("Dereference null return value")
>> > > Addresses-Coverity-ID: 1454223 ("Dereference null return value")
>> > > Addresses-Coverity-ID: 1454224 ("Dereference null return value")
>> > > Addresses-Coverity-ID: 1464669 ("Dereference null return value")
>> > > Addresses-Coverity-ID: 1464670 ("Dereference null return value")
>> > > Addresses-Coverity-ID: 1464672 ("Dereference null return value")
>> > > Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit  
>> implementation")
>> > > Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit callback")
>> > > Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit  
>> implementation")
>> > > Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP information")
>> > > Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource utilization")
>> > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> > > ---
>> > >  drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
>> > >  1 file changed, 25 insertions(+), 1 deletion(-)
>> > >
>> >
>> > It will be much better to fix the tool instead of fixing ghost case.
>> > This scenario is impossible for all those flows.
>> > We can receive the skv/msg in two ways:
>> >  * First by allocating new message with NLMSG_DEFAULT_SIZE, which has
>> > more room
>> >    than nlmsg_total_size(payload), payload is 0.
>> >  * Second by getting from netlink.c and it will be at least "struct
>> > nlmsghdr" too.
>> >
>> > Can you please add this info to the commit message?
>> >
>>
>> Actually, I was planing to send a new version of this patch. This time using
>> the unlikely macro for all the null checks on nlh.
>>
>> What do you think?
>
> It is not datapath, so "unlikely" is not needed. Let's assume that  
> smart enough
> compiler will optimize such flow anyway, because nlmsg_put returns NULL
> in unlikely scenario, so this check will be unlikely automatically too.
>

I'm curious about why the return value of nlmsg_put is null checked  
118 out of 129 times (based on Coverity reports) in the last  
linux-next tree.

So based on what you mention, do you think all those checks are  
actually unnecessary and, maybe they should be removed?

Thanks
--
Gustavo

Re: [PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Leon Romanovsky]

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

On Fri, Feb 09, 2018 at 09:56:00AM -0600, Gustavo A. R. Silva wrote:
>
> Quoting Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>
> > On Fri, Feb 09, 2018 at 07:36:49AM -0600, Gustavo A. R. Silva wrote:
> > > Hi Leon,
> > >
> > > Quoting Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> > >
> > > > On Fri, Feb 09, 2018 at 12:37:02AM -0600, Gustavo A. R. Silva wrote:
> > > > > In case the message header and payload cannot be stored, function
> > > > > nlmsg_put returns null.
> > > > >
> > > > > Fix this by adding multiple sanity checks and avoid a potential
> > > > > null dereference on _nlh_ when calling nlmsg_end.
> > > > >
> > > > > Addresses-Coverity-ID: 1454215 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1454223 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1454224 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1464669 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1464670 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1464672 ("Dereference null return value")
> > > > > Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit
> > > implementation")
> > > > > Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit callback")
> > > > > Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit
> > > implementation")
> > > > > Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP information")
> > > > > Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource utilization")
> > > > > Signed-off-by: Gustavo A. R. Silva <gustavo-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
> > > > > ---
> > > > >  drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
> > > > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > > > >
> > > >
> > > > It will be much better to fix the tool instead of fixing ghost case.
> > > > This scenario is impossible for all those flows.
> > > > We can receive the skv/msg in two ways:
> > > >  * First by allocating new message with NLMSG_DEFAULT_SIZE, which has
> > > > more room
> > > >    than nlmsg_total_size(payload), payload is 0.
> > > >  * Second by getting from netlink.c and it will be at least "struct
> > > > nlmsghdr" too.
> > > >
> > > > Can you please add this info to the commit message?
> > > >
> > >
> > > Actually, I was planing to send a new version of this patch. This time using
> > > the unlikely macro for all the null checks on nlh.
> > >
> > > What do you think?
> >
> > It is not datapath, so "unlikely" is not needed. Let's assume that smart
> > enough
> > compiler will optimize such flow anyway, because nlmsg_put returns NULL
> > in unlikely scenario, so this check will be unlikely automatically too.
> >
>
> I'm curious about why the return value of nlmsg_put is null checked 118 out
> of 129 times (based on Coverity reports) in the last linux-next tree.
>
> So based on what you mention, do you think all those checks are actually
> unnecessary and, maybe they should be removed?

I honestly don't know about all cases, but if message is allocated with
NLMSG_DEFAULT_SIZE and payload is 0, this check won't be needed.

So go ahead, add check if (!...) in all places, but be cautious with
"potential null dereference" claims, it is not always true.

Thanks

>
> Thanks
> --
> Gustavo
>
>
>
>
>
>

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

Re: [PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Leon Romanovsky]

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

On Fri, Feb 09, 2018 at 09:56:00AM -0600, Gustavo A. R. Silva wrote:
>
> Quoting Leon Romanovsky <leon@kernel.org>:
>
> > On Fri, Feb 09, 2018 at 07:36:49AM -0600, Gustavo A. R. Silva wrote:
> > > Hi Leon,
> > >
> > > Quoting Leon Romanovsky <leon@kernel.org>:
> > >
> > > > On Fri, Feb 09, 2018 at 12:37:02AM -0600, Gustavo A. R. Silva wrote:
> > > > > In case the message header and payload cannot be stored, function
> > > > > nlmsg_put returns null.
> > > > >
> > > > > Fix this by adding multiple sanity checks and avoid a potential
> > > > > null dereference on _nlh_ when calling nlmsg_end.
> > > > >
> > > > > Addresses-Coverity-ID: 1454215 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1454223 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1454224 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1464669 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1464670 ("Dereference null return value")
> > > > > Addresses-Coverity-ID: 1464672 ("Dereference null return value")
> > > > > Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit
> > > implementation")
> > > > > Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit callback")
> > > > > Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit
> > > implementation")
> > > > > Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP information")
> > > > > Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource utilization")
> > > > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > > > > ---
> > > > >  drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
> > > > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > > > >
> > > >
> > > > It will be much better to fix the tool instead of fixing ghost case.
> > > > This scenario is impossible for all those flows.
> > > > We can receive the skv/msg in two ways:
> > > >  * First by allocating new message with NLMSG_DEFAULT_SIZE, which has
> > > > more room
> > > >    than nlmsg_total_size(payload), payload is 0.
> > > >  * Second by getting from netlink.c and it will be at least "struct
> > > > nlmsghdr" too.
> > > >
> > > > Can you please add this info to the commit message?
> > > >
> > >
> > > Actually, I was planing to send a new version of this patch. This time using
> > > the unlikely macro for all the null checks on nlh.
> > >
> > > What do you think?
> >
> > It is not datapath, so "unlikely" is not needed. Let's assume that smart
> > enough
> > compiler will optimize such flow anyway, because nlmsg_put returns NULL
> > in unlikely scenario, so this check will be unlikely automatically too.
> >
>
> I'm curious about why the return value of nlmsg_put is null checked 118 out
> of 129 times (based on Coverity reports) in the last linux-next tree.
>
> So based on what you mention, do you think all those checks are actually
> unnecessary and, maybe they should be removed?

I honestly don't know about all cases, but if message is allocated with
NLMSG_DEFAULT_SIZE and payload is 0, this check won't be needed.

So go ahead, add check if (!...) in all places, but be cautious with
"potential null dereference" claims, it is not always true.

Thanks

>
> Thanks
> --
> Gustavo
>
>
>
>
>
>

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

Re: [PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Gustavo A. R. Silva]

Quoting Leon Romanovsky <leon@kernel.org>:

> On Fri, Feb 09, 2018 at 09:56:00AM -0600, Gustavo A. R. Silva wrote:
>>
>> Quoting Leon Romanovsky <leon@kernel.org>:
>>
>> > On Fri, Feb 09, 2018 at 07:36:49AM -0600, Gustavo A. R. Silva wrote:
>> > > Hi Leon,
>> > >
>> > > Quoting Leon Romanovsky <leon@kernel.org>:
>> > >
>> > > > On Fri, Feb 09, 2018 at 12:37:02AM -0600, Gustavo A. R. Silva wrote:
>> > > > > In case the message header and payload cannot be stored, function
>> > > > > nlmsg_put returns null.
>> > > > >
>> > > > > Fix this by adding multiple sanity checks and avoid a potential
>> > > > > null dereference on _nlh_ when calling nlmsg_end.
>> > > > >
>> > > > > Addresses-Coverity-ID: 1454215 ("Dereference null return value")
>> > > > > Addresses-Coverity-ID: 1454223 ("Dereference null return value")
>> > > > > Addresses-Coverity-ID: 1454224 ("Dereference null return value")
>> > > > > Addresses-Coverity-ID: 1464669 ("Dereference null return value")
>> > > > > Addresses-Coverity-ID: 1464670 ("Dereference null return value")
>> > > > > Addresses-Coverity-ID: 1464672 ("Dereference null return value")
>> > > > > Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit
>> > > implementation")
>> > > > > Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port  
>> doit callback")
>> > > > > Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit
>> > > implementation")
>> > > > > Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP information")
>> > > > > Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource  
>> utilization")
>> > > > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> > > > > ---
>> > > > >  drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
>> > > > >  1 file changed, 25 insertions(+), 1 deletion(-)
>> > > > >
>> > > >
>> > > > It will be much better to fix the tool instead of fixing ghost case.
>> > > > This scenario is impossible for all those flows.
>> > > > We can receive the skv/msg in two ways:
>> > > >  * First by allocating new message with NLMSG_DEFAULT_SIZE, which has
>> > > > more room
>> > > >    than nlmsg_total_size(payload), payload is 0.
>> > > >  * Second by getting from netlink.c and it will be at least "struct
>> > > > nlmsghdr" too.
>> > > >
>> > > > Can you please add this info to the commit message?
>> > > >
>> > >
>> > > Actually, I was planing to send a new version of this patch.  
>> This time using
>> > > the unlikely macro for all the null checks on nlh.
>> > >
>> > > What do you think?
>> >
>> > It is not datapath, so "unlikely" is not needed. Let's assume that smart
>> > enough
>> > compiler will optimize such flow anyway, because nlmsg_put returns NULL
>> > in unlikely scenario, so this check will be unlikely automatically too.
>> >
>>
>> I'm curious about why the return value of nlmsg_put is null checked 118 out
>> of 129 times (based on Coverity reports) in the last linux-next tree.
>>
>> So based on what you mention, do you think all those checks are actually
>> unnecessary and, maybe they should be removed?
>
> I honestly don't know about all cases, but if message is allocated with
> NLMSG_DEFAULT_SIZE and payload is 0, this check won't be needed.
>

I got it.

> So go ahead, add check if (!...) in all places, but be cautious with
> "potential null dereference" claims, it is not always true.
>

You are right. I will update the subject and commit message.

Thanks for the feedback, Leon.
I appreciate it.
--
Gustavo

Re: [PATCH] RDMA/nldev: Fix multiple potential NULL pointer dereferences

[Gustavo A. R. Silva]

Hi Leon,

On 02/09/2018 11:36 AM, Gustavo A. R. Silva wrote:
> 
> Quoting Leon Romanovsky <leon@kernel.org>:
> 
>> On Fri, Feb 09, 2018 at 09:56:00AM -0600, Gustavo A. R. Silva wrote:
>>>
>>> Quoting Leon Romanovsky <leon@kernel.org>:
>>>
>>> > On Fri, Feb 09, 2018 at 07:36:49AM -0600, Gustavo A. R. Silva wrote:
>>> > > Hi Leon,
>>> > >
>>> > > Quoting Leon Romanovsky <leon@kernel.org>:
>>> > >
>>> > > > On Fri, Feb 09, 2018 at 12:37:02AM -0600, Gustavo A. R. Silva 
>>> wrote:
>>> > > > > In case the message header and payload cannot be stored, 
>>> function
>>> > > > > nlmsg_put returns null.
>>> > > > >
>>> > > > > Fix this by adding multiple sanity checks and avoid a potential
>>> > > > > null dereference on _nlh_ when calling nlmsg_end.
>>> > > > >
>>> > > > > Addresses-Coverity-ID: 1454215 ("Dereference null return value")
>>> > > > > Addresses-Coverity-ID: 1454223 ("Dereference null return value")
>>> > > > > Addresses-Coverity-ID: 1454224 ("Dereference null return value")
>>> > > > > Addresses-Coverity-ID: 1464669 ("Dereference null return value")
>>> > > > > Addresses-Coverity-ID: 1464670 ("Dereference null return value")
>>> > > > > Addresses-Coverity-ID: 1464672 ("Dereference null return value")
>>> > > > > Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit
>>> > > implementation")
>>> > > > > Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit 
>>> callback")
>>> > > > > Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit
>>> > > implementation")
>>> > > > > Fixes: b5fa635aab8f ("RDMA/nldev: Provide detailed QP 
>>> information")
>>> > > > > Fixes: bf3c5a93c523 ("RDMA/nldev: Provide global resource 
>>> utilization")
>>> > > > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>> > > > > ---
>>> > > > >  drivers/infiniband/core/nldev.c | 26 +++++++++++++++++++++++++-
>>> > > > >  1 file changed, 25 insertions(+), 1 deletion(-)
>>> > > > >
>>> > > >
>>> > > > It will be much better to fix the tool instead of fixing ghost 
>>> case.
>>> > > > This scenario is impossible for all those flows.
>>> > > > We can receive the skv/msg in two ways:
>>> > > >  * First by allocating new message with NLMSG_DEFAULT_SIZE, 
>>> which has
>>> > > > more room
>>> > > >    than nlmsg_total_size(payload), payload is 0.
>>> > > >  * Second by getting from netlink.c and it will be at least 
>>> "struct
>>> > > > nlmsghdr" too.
>>> > > >
>>> > > > Can you please add this info to the commit message?
>>> > > >
>>> > >
>>> > > Actually, I was planing to send a new version of this patch. This 
>>> time using
>>> > > the unlikely macro for all the null checks on nlh.
>>> > >
>>> > > What do you think?
>>> >
>>> > It is not datapath, so "unlikely" is not needed. Let's assume that 
>>> smart
>>> > enough
>>> > compiler will optimize such flow anyway, because nlmsg_put returns 
>>> NULL
>>> > in unlikely scenario, so this check will be unlikely automatically 
>>> too.
>>> >
>>>
>>> I'm curious about why the return value of nlmsg_put is null checked 
>>> 118 out
>>> of 129 times (based on Coverity reports) in the last linux-next tree.
>>>
>>> So based on what you mention, do you think all those checks are actually
>>> unnecessary and, maybe they should be removed?
>>
>> I honestly don't know about all cases, but if message is allocated with
>> NLMSG_DEFAULT_SIZE and payload is 0, this check won't be needed.
>>
> 
> I got it.
> 
>> So go ahead, add check if (!...) in all places, but be cautious with
>> "potential null dereference" claims, it is not always true.
>>
> 

I've finally decided to document all these cases as False Positives in 
the Coverity platform.

I think it is better to do that than adding unnecessary code. I will 
also add a link to this conversation to the Coverity database.

Thanks a lot for your feedback.
--
Gustavo