linux-kernel - Re: [PATCH] infiniband: hw: cxgb3: Fix a possible null-pointer dereference in connect_reply

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ee858ba02e8e560672aff68aead2323bc2136b6c.camel@redhat.com>
Date:   Mon, 29 Jul 2019 12:28:09 -0400
From:   Doug Ledford <dledford@...hat.com>
To:     Jia-Ju Bai <baijiaju1990@...il.com>, bharat@...lsio.com,
        jgg@...pe.ca
Cc:     linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] infiniband: hw: cxgb3: Fix a possible null-pointer
 dereference in connect_reply_upcall()

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@...il.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@...hat.com>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)