net: 9p: Fix a possible null-pointer dereference in p9_cm_event_handler()

[PATCH] net: 9p: Fix a possible null-pointer dereference in p9_cm_event_handler()

Posted by Tuo Li 1 month, 3 weeks ago

In p9_cm_event_handler(), rdma is checked in the RDMA_CM_EVENT_DISCONNECTED
case, indicating that it may be NULL. If this happens, a null-pointer
dereference can occur when complete() is called:

  complete(&rdma->cm_done);

To prevent such a potential null-pointer dereference, add a defensive check
before invoking complete().

Signed-off-by: Tuo Li <islituo@gmail.com>
---
 net/9p/trans_rdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 4d406479f83b..b5ceae1cccbb 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -170,7 +170,8 @@ p9_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 	default:
 		BUG();
 	}
-	complete(&rdma->cm_done);
+	if (rdma)
+		complete(&rdma->cm_done);
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH] net: 9p: Fix a possible null-pointer dereference in p9_cm_event_handler()

Posted by asmadeus@codewreck.org 1 month, 3 weeks ago

Tuo Li wrote on Thu, Dec 18, 2025 at 03:10:12PM +0800:
> In p9_cm_event_handler(), rdma is checked in the RDMA_CM_EVENT_DISCONNECTED
> case, indicating that it may be NULL. If this happens, a null-pointer
> dereference can occur when complete() is called:
> 
>   complete(&rdma->cm_done);
> 
> To prevent such a potential null-pointer dereference, add a defensive check
> before invoking complete().

In practice it doesn't really make sense for rdma to be NULL here
though -- and we can make sure of it by assigning client->trans before
creating the cm_id and get rid of the other null check instead:
-----
diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index b5ceae1cccbb..f2d81f3ef255 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -145,8 +145,7 @@ p9_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 		break;
 
 	case RDMA_CM_EVENT_DISCONNECTED:
-		if (rdma)
-			rdma->state = P9_RDMA_CLOSED;
+		rdma->state = P9_RDMA_CLOSED;
 		c->status = Disconnected;
 		break;
 
@@ -541,15 +540,15 @@ rdma_create_trans(struct p9_client *client, struct fs_context *fc)
 	if (!rdma)
 		return -ENOMEM;
 
+	/* Associate the client with the transport */
+	client->trans = rdma;
+
 	/* Create the RDMA CM ID */
 	rdma->cm_id = rdma_create_id(&init_net, p9_cm_event_handler, client,
 				    RDMA_PS_TCP, IB_QPT_RC);
 	if (IS_ERR(rdma->cm_id))
 		goto error;
 
-	/* Associate the client with the transport */
-	client->trans = rdma;
-
 	/* Bind to a privileged port if we need to */
 	if (opts.privport) {
 		err = p9_rdma_bind_privport(rdma);
-----

What do you think?
-- 
Dominique

Re: [PATCH] net: 9p: Fix a possible null-pointer dereference in p9_cm_event_handler()

Posted by Tuo Li 1 month, 3 weeks ago

On Thu, Dec 18, 2025 at 8:56 PM <asmadeus@codewreck.org> wrote:
>
> Tuo Li wrote on Thu, Dec 18, 2025 at 03:10:12PM +0800:
> > In p9_cm_event_handler(), rdma is checked in the RDMA_CM_EVENT_DISCONNECTED
> > case, indicating that it may be NULL. If this happens, a null-pointer
> > dereference can occur when complete() is called:
> >
> >   complete(&rdma->cm_done);
> >
> > To prevent such a potential null-pointer dereference, add a defensive check
> > before invoking complete().
>
> In practice it doesn't really make sense for rdma to be NULL here
> though -- and we can make sure of it by assigning client->trans before
> creating the cm_id and get rid of the other null check instead:
> -----
> diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
> index b5ceae1cccbb..f2d81f3ef255 100644
> --- a/net/9p/trans_rdma.c
> +++ b/net/9p/trans_rdma.c
> @@ -145,8 +145,7 @@ p9_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
>                 break;
>
>         case RDMA_CM_EVENT_DISCONNECTED:
> -               if (rdma)
> -                       rdma->state = P9_RDMA_CLOSED;
> +               rdma->state = P9_RDMA_CLOSED;
>                 c->status = Disconnected;
>                 break;
>
> @@ -541,15 +540,15 @@ rdma_create_trans(struct p9_client *client, struct fs_context *fc)
>         if (!rdma)
>                 return -ENOMEM;
>
> +       /* Associate the client with the transport */
> +       client->trans = rdma;
> +
>         /* Create the RDMA CM ID */
>         rdma->cm_id = rdma_create_id(&init_net, p9_cm_event_handler, client,
>                                     RDMA_PS_TCP, IB_QPT_RC);
>         if (IS_ERR(rdma->cm_id))
>                 goto error;
>
> -       /* Associate the client with the transport */
> -       client->trans = rdma;
> -
>         /* Bind to a privileged port if we need to */
>         if (opts.privport) {
>                 err = p9_rdma_bind_privport(rdma);
> -----
>
> What do you think?
> --
> Dominique

Thanks for the clarification.
It indeed makes no sense for rdma to be NULL here.
The updated fix looks reasonable to me.

Sincerely,
Tuo Li

Re: [PATCH] net: 9p: Fix a possible null-pointer dereference in p9_cm_event_handler()

Posted by Christian Schoenebeck 1 month, 3 weeks ago

On Thursday, 18 December 2025 08:10:12 CET Tuo Li wrote:
> In p9_cm_event_handler(), rdma is checked in the RDMA_CM_EVENT_DISCONNECTED
> case, indicating that it may be NULL. If this happens, a null-pointer
> dereference can occur when complete() is called:
> 
>   complete(&rdma->cm_done);
> 
> To prevent such a potential null-pointer dereference, add a defensive check
> before invoking complete().
> 
> Signed-off-by: Tuo Li <islituo@gmail.com>
> ---

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>