All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: 9p: Fix possible null-pointer dereferences in p9_cm_event_handler()
@ 2019-07-24 10:39 Jia-Ju Bai
  2019-07-24 12:55 ` Dominique Martinet
  0 siblings, 1 reply; 3+ messages in thread
From: Jia-Ju Bai @ 2019-07-24 10:39 UTC (permalink / raw)
  To: ericvh, lucho, asmadeus, davem
  Cc: v9fs-developer, netdev, linux-kernel, Jia-Ju Bai

In p9_cm_event_handler(), there is an if statement on 260 to check
whether rdma is NULL, which indicates that rdma can be NULL.
If so, using rdma->xxx may cause a possible null-pointer dereference.

To fix these bugs, rdma is checked before being used.

These bugs are found by a static analysis tool STCheck written by us.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 net/9p/trans_rdma.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index bac8dad5dd69..eba3c5fc2731 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -242,18 +242,24 @@ p9_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 	struct p9_trans_rdma *rdma = c->trans;
 	switch (event->event) {
 	case RDMA_CM_EVENT_ADDR_RESOLVED:
-		BUG_ON(rdma->state != P9_RDMA_INIT);
-		rdma->state = P9_RDMA_ADDR_RESOLVED;
+		if (rdma) {
+			BUG_ON(rdma->state != P9_RDMA_INIT);
+			rdma->state = P9_RDMA_ADDR_RESOLVED;
+		}
 		break;
 
 	case RDMA_CM_EVENT_ROUTE_RESOLVED:
-		BUG_ON(rdma->state != P9_RDMA_ADDR_RESOLVED);
-		rdma->state = P9_RDMA_ROUTE_RESOLVED;
+		if (rdma) {
+			BUG_ON(rdma->state != P9_RDMA_ADDR_RESOLVED);
+			rdma->state = P9_RDMA_ROUTE_RESOLVED;
+		}
 		break;
 
 	case RDMA_CM_EVENT_ESTABLISHED:
-		BUG_ON(rdma->state != P9_RDMA_ROUTE_RESOLVED);
-		rdma->state = P9_RDMA_CONNECTED;
+		if (rdma) {
+			BUG_ON(rdma->state != P9_RDMA_ROUTE_RESOLVED);
+			rdma->state = P9_RDMA_CONNECTED;
+		}
 		break;
 
 	case RDMA_CM_EVENT_DISCONNECTED:
@@ -277,12 +283,14 @@ p9_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 	case RDMA_CM_EVENT_ADDR_ERROR:
 	case RDMA_CM_EVENT_UNREACHABLE:
 		c->status = Disconnected;
-		rdma_disconnect(rdma->cm_id);
+		if (rdma)
+			rdma_disconnect(rdma->cm_id);
 		break;
 	default:
 		BUG();
 	}
-	complete(&rdma->cm_done);
+	if (rdma)
+		complete(&rdma->cm_done);
 	return 0;
 }
 
-- 
2.17.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] net: 9p: Fix possible null-pointer dereferences in p9_cm_event_handler()
  2019-07-24 10:39 [PATCH] net: 9p: Fix possible null-pointer dereferences in p9_cm_event_handler() Jia-Ju Bai
@ 2019-07-24 12:55 ` Dominique Martinet
  2019-09-03 10:55   ` Dominique Martinet
  0 siblings, 1 reply; 3+ messages in thread
From: Dominique Martinet @ 2019-07-24 12:55 UTC (permalink / raw)
  To: Jia-Ju Bai; +Cc: ericvh, lucho, davem, v9fs-developer, netdev, linux-kernel

Jia-Ju Bai wrote on Wed, Jul 24, 2019:
> In p9_cm_event_handler(), there is an if statement on 260 to check
> whether rdma is NULL, which indicates that rdma can be NULL.
> If so, using rdma->xxx may cause a possible null-pointer dereference.

The final dereference (complete(&rdma->cm_done) line 285) has been here
from the start, so we would have seen crashes by now if rdma could be
null at this point.

Let's do it the other way around and remove the useless "if (rdma)" that
has been here from day 1 instead ; I basically did the same with
c->status a few months ago (from a coverity report)...


That said, please note that 'rdma checked for null in this event->event
== RDMA_CM_EVENT_DISCONNECTED branch' does not mean rdma can be null in
other branches.
static analysis does not say anything more than the final complete()
should also have a check if the previous one has, but nothing about the
other cases in the switch.


Thanks,
-- 
Dominique

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] net: 9p: Fix possible null-pointer dereferences in p9_cm_event_handler()
  2019-07-24 12:55 ` Dominique Martinet
@ 2019-09-03 10:55   ` Dominique Martinet
  0 siblings, 0 replies; 3+ messages in thread
From: Dominique Martinet @ 2019-09-03 10:55 UTC (permalink / raw)
  To: Jia-Ju Bai; +Cc: ericvh, lucho, davem, v9fs-developer, netdev, linux-kernel

Jia-Ju,

Dominique Martinet wrote on Wed, Jul 24, 2019:
> Jia-Ju Bai wrote on Wed, Jul 24, 2019:
> > In p9_cm_event_handler(), there is an if statement on 260 to check
> > whether rdma is NULL, which indicates that rdma can be NULL.
> > If so, using rdma->xxx may cause a possible null-pointer dereference.
> 
> The final dereference (complete(&rdma->cm_done) line 285) has been here
> from the start, so we would have seen crashes by now if rdma could be
> null at this point.
> 
> Let's do it the other way around and remove the useless "if (rdma)" that
> has been here from day 1 instead ; I basically did the same with
> c->status a few months ago (from a coverity report)...

Did you get anywhere with this, or should I submit a new patch myself ?
In the later case I'll tag this as Reported-by you

Thanks,
-- 
Dominique

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-09-03 10:55 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-24 10:39 [PATCH] net: 9p: Fix possible null-pointer dereferences in p9_cm_event_handler() Jia-Ju Bai
2019-07-24 12:55 ` Dominique Martinet
2019-09-03 10:55   ` Dominique Martinet

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.