[PATCH 1/2] rdma/cm: Fix crash in request handlers

[PATCH 1/2] rdma/cm: Fix crash in request handlers

[Hefty, Sean]

Doug Ledford and RedHat reported a crash when running the rdma_cm on a real
time OS.  The crash has the following call trace:

    cm_process_work
       cma_req_handler
          cma_disable_callback
          rdma_create_id
             kzalloc
             init_completion
          cma_get_net_info
          cma_save_net_info
          cma_any_addr
             cma_zero_addr
          rdma_translate_ip
             rdma_copy_addr
          cma_acquire_dev
             rdma_addr_get_sgid
             ib_find_cached_gid
             cma_attach_to_dev
          ucma_event_handler
             kzalloc
             ib_copy_ah_attr_to_user
          cma_comp
 
[ preempted ]
 
    cma_write
        copy_from_user
        ucma_destroy_id
           copy_from_user
           _ucma_find_context
           ucma_put_ctx
           ucma_free_ctx
              rdma_destroy_id
                 cma_exch
                 cma_cancel_operation
                 rdma_node_get_transport

        rt_mutex_slowunlock
        bad_area_nosemaphore
        oops_enter

They were able to reproduce the crash multiple times with the following
details:

    Crash seems to always happen on the:
            mutex_unlock(&conn_id->handler_mutex);
    as conn_id looks to have been freed during this code path.

An examination of the code shows that a race exists in the request handlers.
When a new connection request is received, the rdma_cm allocates a new
connection identifier.  This identifier has a single reference count on it.
If a user calls rdma_destroy_id() from another thread after receiving a
callback, rdma_destroy_id will proceed to destroy the id and free
the associated memory.  However, the request handlers may still be in
the process of running.  When control returns to the request handlers,
they can attempt to access the newly created identifiers.

Fix this by holding a reference on the newly created rdma_cm_id until
the request handler is through accessing it.

Signed-off-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
This is obviously serious enough to cause a crash, but the bug has also been there
for a while before anyone detected it.  I have no real opinion on whether this
targets 2.6.38 (if that's even possible at this point) or 2.6.39.

 drivers/infiniband/core/cma.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 6884da2..e450c5a 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1210,6 +1210,11 @@ static int cma_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *ib_event)
 	cm_id->context = conn_id;
 	cm_id->cm_handler = cma_ib_handler;
 
+	/*
+	 * Protect against the user destroying conn_id from another thread
+	 * until we're done accessing it.
+	 */
+	atomic_inc(&conn_id->refcount);
 	ret = conn_id->id.event_handler(&conn_id->id, &event);
 	if (!ret) {
 		/*
@@ -1222,8 +1227,10 @@ static int cma_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *ib_event)
 			ib_send_cm_mra(cm_id, CMA_CM_MRA_SETTING, NULL, 0);
 		mutex_unlock(&lock);
 		mutex_unlock(&conn_id->handler_mutex);
+		cma_deref_id(conn_id);
 		goto out;
 	}
+	cma_deref_id(conn_id);
 
 	/* Destroy the CM ID by returning a non-zero value. */
 	conn_id->cm_id.ib = NULL;
@@ -1425,17 +1432,25 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id,
 	event.param.conn.private_data_len = iw_event->private_data_len;
 	event.param.conn.initiator_depth = attr.max_qp_init_rd_atom;
 	event.param.conn.responder_resources = attr.max_qp_rd_atom;
+
+	/*
+	 * Protect against the user destroying conn_id from another thread
+	 * until we're done accessing it.
+	 */
+	atomic_inc(&conn_id->refcount);
 	ret = conn_id->id.event_handler(&conn_id->id, &event);
 	if (ret) {
 		/* User wants to destroy the CM ID */
 		conn_id->cm_id.iw = NULL;
 		cma_exch(conn_id, CMA_DESTROYING);
 		mutex_unlock(&conn_id->handler_mutex);
+		cma_deref_id(conn_id);
 		rdma_destroy_id(&conn_id->id);
 		goto out;
 	}
 
 	mutex_unlock(&conn_id->handler_mutex);
+	cma_deref_id(conn_id);
 
 out:
 	if (dev)


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] rdma/cm: Fix crash in request handlers

[Doug Ledford]

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

On Wed, 2011-02-23 at 08:11 -0800, Hefty, Sean wrote:
> Doug Ledford and RedHat reported a crash when running the rdma_cm on a real
> time OS.  The crash has the following call trace:
> 
>     cm_process_work
>        cma_req_handler
>           cma_disable_callback
>           rdma_create_id
>              kzalloc
>              init_completion
>           cma_get_net_info
>           cma_save_net_info
>           cma_any_addr
>              cma_zero_addr
>           rdma_translate_ip
>              rdma_copy_addr
>           cma_acquire_dev
>              rdma_addr_get_sgid
>              ib_find_cached_gid
>              cma_attach_to_dev
>           ucma_event_handler
>              kzalloc
>              ib_copy_ah_attr_to_user
>           cma_comp
>  
> [ preempted ]
>  
>     cma_write
>         copy_from_user
>         ucma_destroy_id
>            copy_from_user
>            _ucma_find_context
>            ucma_put_ctx
>            ucma_free_ctx
>               rdma_destroy_id
>                  cma_exch
>                  cma_cancel_operation
>                  rdma_node_get_transport
> 
>         rt_mutex_slowunlock
>         bad_area_nosemaphore
>         oops_enter
> 
> They were able to reproduce the crash multiple times with the following
> details:
> 
>     Crash seems to always happen on the:
>             mutex_unlock(&conn_id->handler_mutex);
>     as conn_id looks to have been freed during this code path.
> 
> An examination of the code shows that a race exists in the request handlers.
> When a new connection request is received, the rdma_cm allocates a new
> connection identifier.  This identifier has a single reference count on it.
> If a user calls rdma_destroy_id() from another thread after receiving a
> callback, rdma_destroy_id will proceed to destroy the id and free
> the associated memory.  However, the request handlers may still be in
> the process of running.  When control returns to the request handlers,
> they can attempt to access the newly created identifiers.
> 
> Fix this by holding a reference on the newly created rdma_cm_id until
> the request handler is through accessing it.
> 
> Signed-off-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Acked-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

> ---
> This is obviously serious enough to cause a crash, but the bug has also been there
> for a while before anyone detected it.  I have no real opinion on whether this
> targets 2.6.38 (if that's even possible at this point) or 2.6.39.
> 
>  drivers/infiniband/core/cma.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 6884da2..e450c5a 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -1210,6 +1210,11 @@ static int cma_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *ib_event)
>  	cm_id->context = conn_id;
>  	cm_id->cm_handler = cma_ib_handler;
>  
> +	/*
> +	 * Protect against the user destroying conn_id from another thread
> +	 * until we're done accessing it.
> +	 */
> +	atomic_inc(&conn_id->refcount);
>  	ret = conn_id->id.event_handler(&conn_id->id, &event);
>  	if (!ret) {
>  		/*
> @@ -1222,8 +1227,10 @@ static int cma_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *ib_event)
>  			ib_send_cm_mra(cm_id, CMA_CM_MRA_SETTING, NULL, 0);
>  		mutex_unlock(&lock);
>  		mutex_unlock(&conn_id->handler_mutex);
> +		cma_deref_id(conn_id);
>  		goto out;
>  	}
> +	cma_deref_id(conn_id);
>  
>  	/* Destroy the CM ID by returning a non-zero value. */
>  	conn_id->cm_id.ib = NULL;
> @@ -1425,17 +1432,25 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id,
>  	event.param.conn.private_data_len = iw_event->private_data_len;
>  	event.param.conn.initiator_depth = attr.max_qp_init_rd_atom;
>  	event.param.conn.responder_resources = attr.max_qp_rd_atom;
> +
> +	/*
> +	 * Protect against the user destroying conn_id from another thread
> +	 * until we're done accessing it.
> +	 */
> +	atomic_inc(&conn_id->refcount);
>  	ret = conn_id->id.event_handler(&conn_id->id, &event);
>  	if (ret) {
>  		/* User wants to destroy the CM ID */
>  		conn_id->cm_id.iw = NULL;
>  		cma_exch(conn_id, CMA_DESTROYING);
>  		mutex_unlock(&conn_id->handler_mutex);
> +		cma_deref_id(conn_id);
>  		rdma_destroy_id(&conn_id->id);
>  		goto out;
>  	}
>  
>  	mutex_unlock(&conn_id->handler_mutex);
> +	cma_deref_id(conn_id);
>  
>  out:
>  	if (dev)
> 
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

Infiniband specific RPMs available at
              http://people.redhat.com/dledford/Infiniband


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

Re: [PATCH 1/2] rdma/cm: Fix crash in request handlers

[Roland Dreier]

On Wed, Feb 23, 2011 at 8:11 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -1210,6 +1210,11 @@ static int cma_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *ib_event)
>        cm_id->context = conn_id;
>        cm_id->cm_handler = cma_ib_handler;
>
> +       /*
> +        * Protect against the user destroying conn_id from another thread
> +        * until we're done accessing it.
> +        */
> +       atomic_inc(&conn_id->refcount);

This is a good catch, but I'm not sure I see why this is the right fix.

What prevents the destroy from happening right before the atomic_inc here?
Does this just make the race window much smaller?

>                mutex_unlock(&conn_id->handler_mutex);
> +               cma_deref_id(conn_id);
>                rdma_destroy_id(&conn_id->id);

likewise this seems to drop the additional reference, and then use
the conn_id.  Why can't it be destroyed right after the cma_deref_id
leading to use-after-free?

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] rdma/cm: Fix crash in request handlers

[Hefty, Sean]

> > --- a/drivers/infiniband/core/cma.c
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -1210,6 +1210,11 @@ static int cma_req_handler(struct ib_cm_id *cm_id,
> struct ib_cm_event *ib_event)
> >        cm_id->context = conn_id;
> >        cm_id->cm_handler = cma_ib_handler;
> >
> > +       /*
> > +        * Protect against the user destroying conn_id from another
> thread
> > +        * until we're done accessing it.
> > +        */
> > +       atomic_inc(&conn_id->refcount);
> 
> This is a good catch, but I'm not sure I see why this is the right fix.
> 
> What prevents the destroy from happening right before the atomic_inc here?
> Does this just make the race window much smaller?

This is for a new request.  Until we call the user's event_handler (next line after bumping the refcount) with the new id, they can't destroy it.

> >                mutex_unlock(&conn_id->handler_mutex);
> > +               cma_deref_id(conn_id);
> >                rdma_destroy_id(&conn_id->id);
> 
> likewise this seems to drop the additional reference, and then use
> the conn_id.  Why can't it be destroyed right after the cma_deref_id
> leading to use-after-free?

That is a double free error by the user.  If they return a non-zero value from the callback, that indicates that the rdma_cm should destroy the id.  The user cannot use the id at that point.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] rdma/cm: Fix crash in request handlers

[Roland Dreier]

On Mon, Mar 14, 2011 at 5:12 PM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> likewise this seems to drop the additional reference, and then use
>> the conn_id.  Why can't it be destroyed right after the cma_deref_id
>> leading to use-after-free?
>
> That is a double free error by the user.  If they return a non-zero value from the callback, that indicates that the rdma_cm should destroy the id.  The user cannot use the id at that point.

Doesn't that mean unprivileged userspace could trigger a use-after-free
in the kernel?  (and it might be malicious code, not buggy userspace)

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] rdma/cm: Fix crash in request handlers

[Roland Dreier]

On Mon, Mar 14, 2011 at 7:27 PM, Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Doesn't that mean unprivileged userspace could trigger a use-after-free
> in the kernel?  (and it might be malicious code, not buggy userspace)

From reading the code a bit, I guess ucma is OK in this area.  I do see
what seems like an exploitable race in ucma_create_id():

 - one thread create an id with an invalid userspace pointer
   (so the copy_to_user in ucma_create_id returns -EFAULT
   and calls rdma_destroy_id before idr_remove)
 - another thread guess the id that is going to be returned and
   call ucma_destroy_id()

if the second thread hits the window where the cm_id is
destroyed but the ctx is still in the idr, it can trigger a double free.

But this is a completely different bug.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] rdma/cm: Fix crash in request handlers

[Hefty, Sean]

>  - one thread create an id with an invalid userspace pointer
>    (so the copy_to_user in ucma_create_id returns -EFAULT
>    and calls rdma_destroy_id before idr_remove)
>  - another thread guess the id that is going to be returned and
>    call ucma_destroy_id()
> 
> if the second thread hits the window where the cm_id is
> destroyed but the ctx is still in the idr, it can trigger a double free.

I think we'd have to hold the file->mut around the entire ucma_create_id() operation to fix this.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] rdma/cm: Fix crash in request handlers

[Doug Ledford]

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

On Mon, 2011-03-14 at 21:22 -0700, Roland Dreier wrote:
> On Mon, Mar 14, 2011 at 7:27 PM, Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > Doesn't that mean unprivileged userspace could trigger a use-after-free
> > in the kernel?  (and it might be malicious code, not buggy userspace)
> 
> >From reading the code a bit, I guess ucma is OK in this area.  I do see
> what seems like an exploitable race in ucma_create_id():
> 
>  - one thread create an id with an invalid userspace pointer
>    (so the copy_to_user in ucma_create_id returns -EFAULT
>    and calls rdma_destroy_id before idr_remove)
>  - another thread guess the id that is going to be returned and
>    call ucma_destroy_id()
> 
> if the second thread hits the window where the cm_id is
> destroyed but the ctx is still in the idr, it can trigger a double free.
> 
> But this is a completely different bug.
> 
>  - R.

Heh, nice.  This would be a good example of why I prefer that rules such
as "cm_destroy_id/rdma_destroy_id/ucma_destroy_id can only be called
once per ID" be coded into the destroy function themselves and enforced.
As long as you are talking about user space guessing ID's, it is
patently impossible to guarantee that we won't call a destroy function
more than once.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

Infiniband specific RPMs available at
              http://people.redhat.com/dledford/Infiniband


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

[PATCH] rdma/ucm: Fix race in ucma_create_id

[Hefty, Sean]

The following problem was reported by Roland Dreier based
on code inspection:

	I do see what seems like an exploitable race in
	ucma_create_id():

 	- one thread create an id with an invalid userspace
	  pointer (so the copy_to_user in ucma_create_id
	  returns -EFAULT and calls rdma_destroy_id before
	  idr_remove)
 	- another thread guess the id that is going to be
	  returned and call ucma_destroy_id()

	if the second thread hits the window where the cm_id is
	destroyed but the ctx is still in the idr, it can
	trigger a double free.

There is an issue here that the second thread can try to use
the new rdma_cm_id in another call after it has been destroyed.
(The problem isn't just restricted to the second thread calling
ucma_destroy_id.)  Fix this by holding the file->mux around
the entire creation process, so another thread cannot find the
id until it has been fully created.

Signed-off-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/core/ucma.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index ca12acf..fd6b980 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -384,31 +384,32 @@ static ssize_t ucma_create_id(struct ucma_file *file,
 
 	mutex_lock(&file->mut);
 	ctx = ucma_alloc_ctx(file);
-	mutex_unlock(&file->mut);
-	if (!ctx)
+	if (!ctx) {
+		mutex_unlock(&file->mut);
 		return -ENOMEM;
+	}
 
 	ctx->uid = cmd.uid;
 	ctx->cm_id = rdma_create_id(ucma_event_handler, ctx, cmd.ps);
 	if (IS_ERR(ctx->cm_id)) {
 		ret = PTR_ERR(ctx->cm_id);
-		goto err1;
+		goto err;
 	}
 
 	resp.id = ctx->id;
 	if (copy_to_user((void __user *)(unsigned long)cmd.response,
 			 &resp, sizeof(resp))) {
 		ret = -EFAULT;
-		goto err2;
+		goto err;
 	}
+	mutex_unlock(&file->mut);
 	return 0;
 
-err2:
-	rdma_destroy_id(ctx->cm_id);
-err1:
-	mutex_lock(&mut);
+err:
 	idr_remove(&ctx_idr, ctx->id);
 	mutex_unlock(&mut);
+	if (!IS_ERR(ctx->cm_id))
+		rdma_destroy_id(ctx->cm_id);
 	kfree(ctx);
 	return ret;
 }


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] rdma/ucm: Fix race in ucma_create_id

[Hefty, Sean]

ignore this - there are 2 locks being used in ucma_create_id

I will resubmit


> diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
> index ca12acf..fd6b980 100644
> --- a/drivers/infiniband/core/ucma.c
> +++ b/drivers/infiniband/core/ucma.c
> @@ -384,31 +384,32 @@ static ssize_t ucma_create_id(struct ucma_file *file,
> 
>  	mutex_lock(&file->mut);
>  	ctx = ucma_alloc_ctx(file);
> -	mutex_unlock(&file->mut);
> -	if (!ctx)
> +	if (!ctx) {
> +		mutex_unlock(&file->mut);
>  		return -ENOMEM;
> +	}
> 
>  	ctx->uid = cmd.uid;
>  	ctx->cm_id = rdma_create_id(ucma_event_handler, ctx, cmd.ps);
>  	if (IS_ERR(ctx->cm_id)) {
>  		ret = PTR_ERR(ctx->cm_id);
> -		goto err1;
> +		goto err;
>  	}
> 
>  	resp.id = ctx->id;
>  	if (copy_to_user((void __user *)(unsigned long)cmd.response,
>  			 &resp, sizeof(resp))) {
>  		ret = -EFAULT;
> -		goto err2;
> +		goto err;
>  	}
> +	mutex_unlock(&file->mut);
>  	return 0;
> 
> -err2:
> -	rdma_destroy_id(ctx->cm_id);
> -err1:
> -	mutex_lock(&mut);
> +err:
>  	idr_remove(&ctx_idr, ctx->id);
>  	mutex_unlock(&mut);
> +	if (!IS_ERR(ctx->cm_id))
> +		rdma_destroy_id(ctx->cm_id);
>  	kfree(ctx);
>  	return ret;
>  }
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html