[PATCH] infiniband: hw: cxgb3: Fix a possible null-pointer dereference in connect_reply_upcall()

[PATCH] infiniband: hw: cxgb3: Fix a possible null-pointer dereference in connect_reply_upcall()

[Jia-Ju Bai]

In connect_reply_upcall(), there is an if statement on line 730 to check
whether ep->com.cm_id is NULL:
    if (ep->com.cm_id)

When ep->com.cm_id is NULL, it is used on line 736:
    ep->com.cm_id->rem_ref(ep->com.cm_id);

Thus, a possible null-pointer dereference may occur.

To fix this bug, ep->com.cm_id is checked before being used.

This bug is found by a static analysis tool STCheck written by us.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/infiniband/hw/cxgb3/iwch_cm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c b/drivers/infiniband/hw/cxgb3/iwch_cm.c
index 0bca72cb4d9a..2b31c4726d3e 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
@@ -733,7 +733,8 @@ static void connect_reply_upcall(struct iwch_ep *ep, int status)
 		ep->com.cm_id->event_handler(ep->com.cm_id, &event);
 	}
 	if (status < 0) {
-		ep->com.cm_id->rem_ref(ep->com.cm_id);
+		if (ep->com.cm_id)
+			ep->com.cm_id->rem_ref(ep->com.cm_id);
 		ep->com.cm_id = NULL;
 		ep->com.qp = NULL;
 	}
-- 
2.17.0

Re: [PATCH] infiniband: hw: cxgb3: Fix a possible null-pointer dereference in connect_reply_upcall()

[Doug Ledford]

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

On Thu, 2019-07-25 at 20:15 +0800, Jia-Ju Bai wrote:
> In connect_reply_upcall(), there is an if statement on line 730 to
> check
> whether ep->com.cm_id is NULL:
>     if (ep->com.cm_id)
> 
> When ep->com.cm_id is NULL, it is used on line 736:
>     ep->com.cm_id->rem_ref(ep->com.cm_id);
> 
> Thus, a possible null-pointer dereference may occur.
> 
> To fix this bug, ep->com.cm_id is checked before being used.
> 
> This bug is found by a static analysis tool STCheck written by us.

I think this is one of those theoretical issues that is a non-issue in
practice.  The cxgb3 driver is older than dirt and only hangs around
because people out there are still using it in a few places.  We have no
reports of bugs in this function.  I looked through the code, but it
wasn't quickly obvious why this isn't an issue, but I suspect the real
answer is "we can never have a negative status and not have a cm_id" as
a result of the design of the code.  Verifying that with an audit is
more time than I want to spend though.  So, I'm going to drop this
patch.  If you can find a plausible path by which this is actually a
bug, then we can revisit it.  But I don't want to go around changing a
known working for years driver on the basis of "my new static code
checker found this thing" versus someone who reports an actual crash
(which is what this bug would cause if it exists).

> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  drivers/infiniband/hw/cxgb3/iwch_cm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c
> b/drivers/infiniband/hw/cxgb3/iwch_cm.c
> index 0bca72cb4d9a..2b31c4726d3e 100644
> --- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
> +++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
> @@ -733,7 +733,8 @@ static void connect_reply_upcall(struct iwch_ep
> *ep, int status)
>  		ep->com.cm_id->event_handler(ep->com.cm_id, &event);
>  	}
>  	if (status < 0) {
> -		ep->com.cm_id->rem_ref(ep->com.cm_id);
> +		if (ep->com.cm_id)
> +			ep->com.cm_id->rem_ref(ep->com.cm_id);
>  		ep->com.cm_id = NULL;
>  		ep->com.qp = NULL;
>  	}

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]