| Message ID | 20190725121508.16352-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | infiniband: hw: cxgb3: Fix a possible null-pointer dereference in connect_reply_upcall() | expand |
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; > }
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; }
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(-)