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@xxxxxxxxxxxxxxxxx>
---
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
