* [PATCH] infiniband: fix a possible use-after-free bug
@ 2018-06-01 18:31 Cong Wang
2018-06-04 16:23 ` Gi-Oh Kim
2018-06-04 16:42 ` Jason Gunthorpe
0 siblings, 2 replies; 6+ messages in thread
From: Cong Wang @ 2018-06-01 18:31 UTC (permalink / raw)
To: linux-kernel
Cc: noamr, Cong Wang, Sean Hefty, Doug Ledford, Jason Gunthorpe, linux-rdma
ucma_process_join() will free the new allocated "mc" struct,
if there is any error after that, especially the copy_to_user().
But in parallel, ucma_leave_multicast() could find this "mc"
through idr_find() before ucma_process_join() frees it, since it
is already published.
So "mc" could be used in ucma_leave_multicast() after it is been
allocated and freed in ucma_process_join(), since we don't refcnt
it.
Fix this by separating "publish" from ID allocation, so that we
can get an ID first and publish it later after copy_to_user().
Fixes c8f6a362bf3e ("RDMA/cma: Add multicast communication support")
Reported-by: Noam Rathaus <noamr@beyondsecurity.com>
Cc: Sean Hefty <sean.hefty@intel.com>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: linux-rdma@vger.kernel.org
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
drivers/infiniband/core/ucma.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index eab43b17e9cf..ec8fb289621f 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -235,7 +235,7 @@ static struct ucma_multicast* ucma_alloc_multicast(struct ucma_context *ctx)
return NULL;
mutex_lock(&mut);
- mc->id = idr_alloc(&multicast_idr, mc, 0, 0, GFP_KERNEL);
+ mc->id = idr_alloc(&multicast_idr, NULL, 0, 0, GFP_KERNEL);
mutex_unlock(&mut);
if (mc->id < 0)
goto error;
@@ -1421,6 +1421,10 @@ static ssize_t ucma_process_join(struct ucma_file *file,
goto err3;
}
+ mutex_lock(&mut);
+ idr_replace(&multicast_idr, mc, mc->id);
+ mutex_unlock(&mut);
+
mutex_unlock(&file->mut);
ucma_put_ctx(ctx);
return 0;
--
2.13.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] infiniband: fix a possible use-after-free bug 2018-06-01 18:31 [PATCH] infiniband: fix a possible use-after-free bug Cong Wang @ 2018-06-04 16:23 ` Gi-Oh Kim 2018-06-04 16:30 ` Cong Wang 2018-06-04 16:46 ` Jason Gunthorpe 2018-06-04 16:42 ` Jason Gunthorpe 1 sibling, 2 replies; 6+ messages in thread From: Gi-Oh Kim @ 2018-06-04 16:23 UTC (permalink / raw) To: Cong Wang Cc: linux-kernel, noamr, Sean Hefty, Doug Ledford, Jason Gunthorpe, linux-rdma On Fri, Jun 1, 2018 at 8:31 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > ucma_process_join() will free the new allocated "mc" struct, > if there is any error after that, especially the copy_to_user(). > > But in parallel, ucma_leave_multicast() could find this "mc" > through idr_find() before ucma_process_join() frees it, since it > is already published. > > So "mc" could be used in ucma_leave_multicast() after it is been > allocated and freed in ucma_process_join(), since we don't refcnt > it. > > Fix this by separating "publish" from ID allocation, so that we > can get an ID first and publish it later after copy_to_user(). > > Fixes c8f6a362bf3e ("RDMA/cma: Add multicast communication support") > Reported-by: Noam Rathaus <noamr@beyondsecurity.com> > Cc: Sean Hefty <sean.hefty@intel.com> > Cc: Doug Ledford <dledford@redhat.com> > Cc: Jason Gunthorpe <jgg@ziepe.ca> > Cc: linux-rdma@vger.kernel.org > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > drivers/infiniband/core/ucma.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > index eab43b17e9cf..ec8fb289621f 100644 > --- a/drivers/infiniband/core/ucma.c > +++ b/drivers/infiniband/core/ucma.c > @@ -235,7 +235,7 @@ static struct ucma_multicast* ucma_alloc_multicast(struct ucma_context *ctx) > return NULL; > > mutex_lock(&mut); > - mc->id = idr_alloc(&multicast_idr, mc, 0, 0, GFP_KERNEL); > + mc->id = idr_alloc(&multicast_idr, NULL, 0, 0, GFP_KERNEL); > mutex_unlock(&mut); > if (mc->id < 0) > goto error; > @@ -1421,6 +1421,10 @@ static ssize_t ucma_process_join(struct ucma_file *file, > goto err3; > } > > + mutex_lock(&mut); > + idr_replace(&multicast_idr, mc, mc->id); > + mutex_unlock(&mut); > + > mutex_unlock(&file->mut); > ucma_put_ctx(ctx); > return 0; > -- > 2.13.0 > Hi, Your patch is reasonable to me. Can I ask a question for that? Could it be solved by asymmetric locking as following? diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c index eab43b17e9cf..d8b256baec31 100644 --- a/drivers/infiniband/core/ucma.c +++ b/drivers/infiniband/core/ucma.c @@ -1493,6 +1493,7 @@ static ssize_t ucma_leave_multicast(struct ucma_file *file, if (copy_from_user(&cmd, inbuf, sizeof(cmd))) return -EFAULT; + mutex_lock(&mc->ctx->file->mut); mutex_lock(&mut); mc = idr_find(&multicast_idr, cmd.id); if (!mc) @@ -1507,11 +1508,11 @@ static ssize_t ucma_leave_multicast(struct ucma_file *file, if (IS_ERR(mc)) { ret = PTR_ERR(mc); + mutex_unlock(&mc->ctx->file->mut); goto out; } rdma_leave_multicast(mc->ctx->cm_id, (struct sockaddr *) &mc->addr); - mutex_lock(&mc->ctx->file->mut); ucma_cleanup_mc_events(mc); list_del(&mc->list); mutex_unlock(&mc->ctx->file->mut); -- GIOH KIM Linux Kernel Entwickler ProfitBricks GmbH Greifswalder Str. 207 D - 10405 Berlin Tel: +49 176 2697 8962 Fax: +49 30 577 008 299 Email: gi-oh.kim@profitbricks.com URL: https://www.profitbricks.de Sitz der Gesellschaft: Berlin Registergericht: Amtsgericht Charlottenburg, HRB 125506 B Geschäftsführer: Achim Weiss, Matthias Steinberg, Christoph Steffens ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] infiniband: fix a possible use-after-free bug 2018-06-04 16:23 ` Gi-Oh Kim @ 2018-06-04 16:30 ` Cong Wang 2018-06-04 16:46 ` Jason Gunthorpe 1 sibling, 0 replies; 6+ messages in thread From: Cong Wang @ 2018-06-04 16:30 UTC (permalink / raw) To: Gi-Oh Kim Cc: LKML, Noam Rathaus, Sean Hefty, Doug Ledford, Jason Gunthorpe, linux-rdma On Mon, Jun 4, 2018 at 9:23 AM, Gi-Oh Kim <gi-oh.kim@profitbricks.com> wrote: > Hi, > > Your patch is reasonable to me. > Can I ask a question for that? > Could it be solved by asymmetric locking as following? Maybe, if you are sure taking file->mut is safe in that place. Can you double check atomic_inc_not_zero(&mc->ctx->ref)? I am too far away from familiar with infiniband. ;) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] infiniband: fix a possible use-after-free bug 2018-06-04 16:23 ` Gi-Oh Kim 2018-06-04 16:30 ` Cong Wang @ 2018-06-04 16:46 ` Jason Gunthorpe 2018-06-04 16:52 ` Gi-Oh Kim 1 sibling, 1 reply; 6+ messages in thread From: Jason Gunthorpe @ 2018-06-04 16:46 UTC (permalink / raw) To: Gi-Oh Kim Cc: Cong Wang, linux-kernel, noamr, Sean Hefty, Doug Ledford, linux-rdma On Mon, Jun 04, 2018 at 06:23:20PM +0200, Gi-Oh Kim wrote: > On Fri, Jun 1, 2018 at 8:31 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > ucma_process_join() will free the new allocated "mc" struct, > > if there is any error after that, especially the copy_to_user(). > > > > But in parallel, ucma_leave_multicast() could find this "mc" > > through idr_find() before ucma_process_join() frees it, since it > > is already published. > > > > So "mc" could be used in ucma_leave_multicast() after it is been > > allocated and freed in ucma_process_join(), since we don't refcnt > > it. > > > > Fix this by separating "publish" from ID allocation, so that we > > can get an ID first and publish it later after copy_to_user(). > > > > Fixes c8f6a362bf3e ("RDMA/cma: Add multicast communication support") > > Reported-by: Noam Rathaus <noamr@beyondsecurity.com> > > Cc: Sean Hefty <sean.hefty@intel.com> > > Cc: Doug Ledford <dledford@redhat.com> > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > Cc: linux-rdma@vger.kernel.org > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > drivers/infiniband/core/ucma.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > > index eab43b17e9cf..ec8fb289621f 100644 > > +++ b/drivers/infiniband/core/ucma.c > > @@ -235,7 +235,7 @@ static struct ucma_multicast* ucma_alloc_multicast(struct ucma_context *ctx) > > return NULL; > > > > mutex_lock(&mut); > > - mc->id = idr_alloc(&multicast_idr, mc, 0, 0, GFP_KERNEL); > > + mc->id = idr_alloc(&multicast_idr, NULL, 0, 0, GFP_KERNEL); > > mutex_unlock(&mut); > > if (mc->id < 0) > > goto error; > > @@ -1421,6 +1421,10 @@ static ssize_t ucma_process_join(struct ucma_file *file, > > goto err3; > > } > > > > + mutex_lock(&mut); > > + idr_replace(&multicast_idr, mc, mc->id); > > + mutex_unlock(&mut); > > + > > mutex_unlock(&file->mut); > > ucma_put_ctx(ctx); > > return 0; > > > > > Hi, > > Your patch is reasonable to me. > Can I ask a question for that? > Could it be solved by asymmetric locking as following? No, there are many other paths that touch multicast_idr that don't hold both locks, we should protect all of them from accessing an incompletely initialized structure. Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] infiniband: fix a possible use-after-free bug 2018-06-04 16:46 ` Jason Gunthorpe @ 2018-06-04 16:52 ` Gi-Oh Kim 0 siblings, 0 replies; 6+ messages in thread From: Gi-Oh Kim @ 2018-06-04 16:52 UTC (permalink / raw) To: Jason Gunthorpe Cc: Cong Wang, linux-kernel, noamr, Sean Hefty, Doug Ledford, linux-rdma On Mon, Jun 4, 2018 at 6:46 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > On Mon, Jun 04, 2018 at 06:23:20PM +0200, Gi-Oh Kim wrote: >> On Fri, Jun 1, 2018 at 8:31 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> > ucma_process_join() will free the new allocated "mc" struct, >> > if there is any error after that, especially the copy_to_user(). >> > >> > But in parallel, ucma_leave_multicast() could find this "mc" >> > through idr_find() before ucma_process_join() frees it, since it >> > is already published. >> > >> > So "mc" could be used in ucma_leave_multicast() after it is been >> > allocated and freed in ucma_process_join(), since we don't refcnt >> > it. >> > >> > Fix this by separating "publish" from ID allocation, so that we >> > can get an ID first and publish it later after copy_to_user(). >> > >> > Fixes c8f6a362bf3e ("RDMA/cma: Add multicast communication support") >> > Reported-by: Noam Rathaus <noamr@beyondsecurity.com> >> > Cc: Sean Hefty <sean.hefty@intel.com> >> > Cc: Doug Ledford <dledford@redhat.com> >> > Cc: Jason Gunthorpe <jgg@ziepe.ca> >> > Cc: linux-rdma@vger.kernel.org >> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >> > drivers/infiniband/core/ucma.c | 6 +++++- >> > 1 file changed, 5 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c >> > index eab43b17e9cf..ec8fb289621f 100644 >> > +++ b/drivers/infiniband/core/ucma.c >> > @@ -235,7 +235,7 @@ static struct ucma_multicast* ucma_alloc_multicast(struct ucma_context *ctx) >> > return NULL; >> > >> > mutex_lock(&mut); >> > - mc->id = idr_alloc(&multicast_idr, mc, 0, 0, GFP_KERNEL); >> > + mc->id = idr_alloc(&multicast_idr, NULL, 0, 0, GFP_KERNEL); >> > mutex_unlock(&mut); >> > if (mc->id < 0) >> > goto error; >> > @@ -1421,6 +1421,10 @@ static ssize_t ucma_process_join(struct ucma_file *file, >> > goto err3; >> > } >> > >> > + mutex_lock(&mut); >> > + idr_replace(&multicast_idr, mc, mc->id); >> > + mutex_unlock(&mut); >> > + >> > mutex_unlock(&file->mut); >> > ucma_put_ctx(ctx); >> > return 0; >> > >> >> >> Hi, >> >> Your patch is reasonable to me. >> Can I ask a question for that? >> Could it be solved by asymmetric locking as following? > > No, there are many other paths that touch multicast_idr that don't > hold both locks, we should protect all of them from accessing an > incompletely initialized structure. > > Jason Understood. Thank you. -- GIOH KIM Linux Kernel Entwickler ProfitBricks GmbH Greifswalder Str. 207 D - 10405 Berlin Tel: +49 176 2697 8962 Fax: +49 30 577 008 299 Email: gi-oh.kim@profitbricks.com URL: https://www.profitbricks.de Sitz der Gesellschaft: Berlin Registergericht: Amtsgericht Charlottenburg, HRB 125506 B Geschäftsführer: Achim Weiss, Matthias Steinberg, Christoph Steffens ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] infiniband: fix a possible use-after-free bug 2018-06-01 18:31 [PATCH] infiniband: fix a possible use-after-free bug Cong Wang 2018-06-04 16:23 ` Gi-Oh Kim @ 2018-06-04 16:42 ` Jason Gunthorpe 1 sibling, 0 replies; 6+ messages in thread From: Jason Gunthorpe @ 2018-06-04 16:42 UTC (permalink / raw) To: Cong Wang; +Cc: linux-kernel, noamr, Sean Hefty, Doug Ledford, linux-rdma On Fri, Jun 01, 2018 at 11:31:44AM -0700, Cong Wang wrote: > ucma_process_join() will free the new allocated "mc" struct, > if there is any error after that, especially the copy_to_user(). > > But in parallel, ucma_leave_multicast() could find this "mc" > through idr_find() before ucma_process_join() frees it, since it > is already published. > > So "mc" could be used in ucma_leave_multicast() after it is been > allocated and freed in ucma_process_join(), since we don't refcnt > it. > > Fix this by separating "publish" from ID allocation, so that we > can get an ID first and publish it later after copy_to_user(). > > Fixes c8f6a362bf3e ("RDMA/cma: Add multicast communication support") > Reported-by: Noam Rathaus <noamr@beyondsecurity.com> > Cc: Sean Hefty <sean.hefty@intel.com> > Cc: Doug Ledford <dledford@redhat.com> > Cc: Jason Gunthorpe <jgg@ziepe.ca> > Cc: linux-rdma@vger.kernel.org > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > drivers/infiniband/core/ucma.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) Looks good to me, and we already fixed the same sort of bug in the non-multicast IDs.. Applied to for-next Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-04 16:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-01 18:31 [PATCH] infiniband: fix a possible use-after-free bug Cong Wang 2018-06-04 16:23 ` Gi-Oh Kim 2018-06-04 16:30 ` Cong Wang 2018-06-04 16:46 ` Jason Gunthorpe 2018-06-04 16:52 ` Gi-Oh Kim 2018-06-04 16:42 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).