* [PATCH] io_uring: fix possible use after free to sqd
@ 2020-10-15 9:13 Xiaoguang Wang
2020-10-15 10:01 ` Stefano Garzarella
0 siblings, 1 reply; 5+ messages in thread
From: Xiaoguang Wang @ 2020-10-15 9:13 UTC (permalink / raw)
To: io-uring; +Cc: axboe, joseph.qi, Xiaoguang Wang
Reading codes finds a possible use after free issue to sqd:
thread1 | thread2
==> io_attach_sq_data() |
===> sqd = ctx_attach->sq_data;|
| ==> io_put_sq_data()
| ===> refcount_dec_and_test(&sqd->refs)
| If sqd->refs is zero, will free sqd.
|
===> refcount_inc(&sqd->refs); |
|
| ====> kfree(sqd);
===> now use after free to sqd |
Use refcount_inc_not_zero() to fix this issue.
Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
---
fs/io_uring.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 33b5cf18bb51..48e230feb704 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6868,7 +6868,11 @@ static struct io_sq_data *io_attach_sq_data(struct io_uring_params *p)
return ERR_PTR(-EINVAL);
}
- refcount_inc(&sqd->refs);
+ if (!refcount_inc_not_zero(&sqd->refs)) {
+ fdput(f);
+ return ERR_PTR(-EINVAL);
+ }
+
fdput(f);
return sqd;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] io_uring: fix possible use after free to sqd 2020-10-15 9:13 [PATCH] io_uring: fix possible use after free to sqd Xiaoguang Wang @ 2020-10-15 10:01 ` Stefano Garzarella 2020-10-15 10:46 ` Xiaoguang Wang 0 siblings, 1 reply; 5+ messages in thread From: Stefano Garzarella @ 2020-10-15 10:01 UTC (permalink / raw) To: Xiaoguang Wang; +Cc: io-uring, axboe, joseph.qi On Thu, Oct 15, 2020 at 05:13:35PM +0800, Xiaoguang Wang wrote: > Reading codes finds a possible use after free issue to sqd: > thread1 | thread2 > ==> io_attach_sq_data() | > ===> sqd = ctx_attach->sq_data;| > | ==> io_put_sq_data() > | ===> refcount_dec_and_test(&sqd->refs) > | If sqd->refs is zero, will free sqd. > | > ===> refcount_inc(&sqd->refs); | > | > | ====> kfree(sqd); > ===> now use after free to sqd | > IIUC the io_attach_sq_data() is called only during the setup of an io_uring context, before that the fd is returned to the user space. So, I'm not sure a second thread can call io_put_sq_data() while the first thread is in io_attach_sq_data(). Can you check if this situation can really happen? Thanks, Stefano > Use refcount_inc_not_zero() to fix this issue. > > Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com> > --- > fs/io_uring.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index 33b5cf18bb51..48e230feb704 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -6868,7 +6868,11 @@ static struct io_sq_data *io_attach_sq_data(struct io_uring_params *p) > return ERR_PTR(-EINVAL); > } > > - refcount_inc(&sqd->refs); > + if (!refcount_inc_not_zero(&sqd->refs)) { > + fdput(f); > + return ERR_PTR(-EINVAL); > + } > + > fdput(f); > return sqd; > } > -- > 2.17.2 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] io_uring: fix possible use after free to sqd 2020-10-15 10:01 ` Stefano Garzarella @ 2020-10-15 10:46 ` Xiaoguang Wang 2020-10-15 11:07 ` Stefano Garzarella 0 siblings, 1 reply; 5+ messages in thread From: Xiaoguang Wang @ 2020-10-15 10:46 UTC (permalink / raw) To: Stefano Garzarella; +Cc: io-uring, axboe, joseph.qi hi， > On Thu, Oct 15, 2020 at 05:13:35PM +0800, Xiaoguang Wang wrote: >> Reading codes finds a possible use after free issue to sqd: >> thread1 | thread2 >> ==> io_attach_sq_data() | >> ===> sqd = ctx_attach->sq_data;| >> | ==> io_put_sq_data() >> | ===> refcount_dec_and_test(&sqd->refs) >> | If sqd->refs is zero, will free sqd. >> | >> ===> refcount_inc(&sqd->refs); | >> | >> | ====> kfree(sqd); >> ===> now use after free to sqd | >> > > IIUC the io_attach_sq_data() is called only during the setup of an > io_uring context, before that the fd is returned to the user space. Sorry I didn't make it clear in commit message. In io_attach_sq_data(), we'll try to attach to a previous io_uring instance indicated by p->wq_fd, this p->wq_fd could be closed independently. Regards, Xiaoguang Wang > > So, I'm not sure a second thread can call io_put_sq_data() while the > first thread is in io_attach_sq_data(). > > Can you check if this situation can really happen? > > Thanks, > Stefano > >> Use refcount_inc_not_zero() to fix this issue. >> >> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com> >> --- >> fs/io_uring.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index 33b5cf18bb51..48e230feb704 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -6868,7 +6868,11 @@ static struct io_sq_data *io_attach_sq_data(struct io_uring_params *p) >> return ERR_PTR(-EINVAL); >> } >> >> - refcount_inc(&sqd->refs); >> + if (!refcount_inc_not_zero(&sqd->refs)) { >> + fdput(f); >> + return ERR_PTR(-EINVAL); >> + } >> + >> fdput(f); >> return sqd; >> } >> -- >> 2.17.2 >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] io_uring: fix possible use after free to sqd 2020-10-15 10:46 ` Xiaoguang Wang @ 2020-10-15 11:07 ` Stefano Garzarella 2020-10-15 11:24 ` Xiaoguang Wang 0 siblings, 1 reply; 5+ messages in thread From: Stefano Garzarella @ 2020-10-15 11:07 UTC (permalink / raw) To: Xiaoguang Wang; +Cc: io-uring, axboe, joseph.qi On Thu, Oct 15, 2020 at 06:46:34PM +0800, Xiaoguang Wang wrote: > hi閿涳拷 > > > On Thu, Oct 15, 2020 at 05:13:35PM +0800, Xiaoguang Wang wrote: > > > Reading codes finds a possible use after free issue to sqd: > > > thread1 | thread2 > > > ==> io_attach_sq_data() | > > > ===> sqd = ctx_attach->sq_data;| > > > | ==> io_put_sq_data() > > > | ===> refcount_dec_and_test(&sqd->refs) > > > | If sqd->refs is zero, will free sqd. > > > | > > > ===> refcount_inc(&sqd->refs); | > > > | > > > | ====> kfree(sqd); > > > ===> now use after free to sqd | > > > > > > > IIUC the io_attach_sq_data() is called only during the setup of an > > io_uring context, before that the fd is returned to the user space. > Sorry I didn't make it clear in commit message. > In io_attach_sq_data(), we'll try to attach to a previous io_uring instance > indicated by p->wq_fd, this p->wq_fd could be closed independently. Thanks to clarify! Got it. Also in this case, IIUC io_put_sq_data() is called only when io_uring_release() is invoked on the previous io_uring instance. Since we are taking a reference with fdget at the begin of io_attach_sq_data() and we release this reference only at the end of io_attach_sq_data(), can io_put_sq_data() runs in another thread while we have the reference? Thanks, Stefano > > Regards, > Xiaoguang Wang > > > > So, I'm not sure a second thread can call io_put_sq_data() while the > > first thread is in io_attach_sq_data(). > > > > Can you check if this situation can really happen? > > > > Thanks, > > Stefano > > > > > Use refcount_inc_not_zero() to fix this issue. > > > > > > Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com> > > > --- > > > fs/io_uring.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/io_uring.c b/fs/io_uring.c > > > index 33b5cf18bb51..48e230feb704 100644 > > > --- a/fs/io_uring.c > > > +++ b/fs/io_uring.c > > > @@ -6868,7 +6868,11 @@ static struct io_sq_data *io_attach_sq_data(struct io_uring_params *p) > > > return ERR_PTR(-EINVAL); > > > } > > > - refcount_inc(&sqd->refs); > > > + if (!refcount_inc_not_zero(&sqd->refs)) { > > > + fdput(f); > > > + return ERR_PTR(-EINVAL); > > > + } > > > + > > > fdput(f); > > > return sqd; > > > } > > > -- > > > 2.17.2 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] io_uring: fix possible use after free to sqd 2020-10-15 11:07 ` Stefano Garzarella @ 2020-10-15 11:24 ` Xiaoguang Wang 0 siblings, 0 replies; 5+ messages in thread From: Xiaoguang Wang @ 2020-10-15 11:24 UTC (permalink / raw) To: Stefano Garzarella; +Cc: io-uring, axboe, joseph.qi > On Thu, Oct 15, 2020 at 06:46:34PM +0800, Xiaoguang Wang wrote: >> hi閿涳拷 >> >>> On Thu, Oct 15, 2020 at 05:13:35PM +0800, Xiaoguang Wang wrote: >>>> Reading codes finds a possible use after free issue to sqd: >>>> thread1 | thread2 >>>> ==> io_attach_sq_data() | >>>> ===> sqd = ctx_attach->sq_data;| >>>> | ==> io_put_sq_data() >>>> | ===> refcount_dec_and_test(&sqd->refs) >>>> | If sqd->refs is zero, will free sqd. >>>> | >>>> ===> refcount_inc(&sqd->refs); | >>>> | >>>> | ====> kfree(sqd); >>>> ===> now use after free to sqd | >>>> >>> >>> IIUC the io_attach_sq_data() is called only during the setup of an >>> io_uring context, before that the fd is returned to the user space. >> Sorry I didn't make it clear in commit message. >> In io_attach_sq_data(), we'll try to attach to a previous io_uring instance >> indicated by p->wq_fd, this p->wq_fd could be closed independently. > > Thanks to clarify! Got it. > > Also in this case, IIUC io_put_sq_data() is called only when > io_uring_release() is invoked on the previous io_uring instance. > > Since we are taking a reference with fdget at the begin of io_attach_sq_data() > and we release this reference only at the end of io_attach_sq_data(), > can io_put_sq_data() runs in another thread while we have the reference? Oh, right, thanks. Jens, you can ignore this patch, thanks. Regards, Xiaoguang Wang > > Thanks, > Stefano > >> >> Regards, >> Xiaoguang Wang >>> >>> So, I'm not sure a second thread can call io_put_sq_data() while the >>> first thread is in io_attach_sq_data(). >>> >>> Can you check if this situation can really happen? >>> >>> Thanks, >>> Stefano >>> >>>> Use refcount_inc_not_zero() to fix this issue. >>>> >>>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com> >>>> --- >>>> fs/io_uring.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>> index 33b5cf18bb51..48e230feb704 100644 >>>> --- a/fs/io_uring.c >>>> +++ b/fs/io_uring.c >>>> @@ -6868,7 +6868,11 @@ static struct io_sq_data *io_attach_sq_data(struct io_uring_params *p) >>>> return ERR_PTR(-EINVAL); >>>> } >>>> - refcount_inc(&sqd->refs); >>>> + if (!refcount_inc_not_zero(&sqd->refs)) { >>>> + fdput(f); >>>> + return ERR_PTR(-EINVAL); >>>> + } >>>> + >>>> fdput(f); >>>> return sqd; >>>> } >>>> -- >>>> 2.17.2 >>>> >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-15 11:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-15 9:13 [PATCH] io_uring: fix possible use after free to sqd Xiaoguang Wang 2020-10-15 10:01 ` Stefano Garzarella 2020-10-15 10:46 ` Xiaoguang Wang 2020-10-15 11:07 ` Stefano Garzarella 2020-10-15 11:24 ` Xiaoguang Wang
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).