* [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).