On 3/13/21 12:01 PM, Jens Axboe wrote: > On 3/13/21 8:34 AM, Jens Axboe wrote: >> On 3/13/21 1:02 AM, yangerkun wrote: >>> >>> >>> 在 2021/3/9 19:23, yangerkun 写道: >>>> >>>> >>>> 在 2021/3/8 22:22, Pavel Begunkov 写道: >>>>> On 08/03/2021 14:16, Pavel Begunkov wrote: >>>>>> From: "Matthew Wilcox (Oracle)" <willy@xxxxxxxxxxxxx> >>>>>> >>>>>> You can't call idr_remove() from within a idr_for_each() callback, >>>>>> but you can call xa_erase() from an xa_for_each() loop, so switch the >>>>>> entire personality_idr from the IDR to the XArray. This manifests as a >>>>>> use-after-free as idr_for_each() attempts to walk the rest of the node >>>>>> after removing the last entry from it. >>>>> >>>>> yangerkun, can you test it and similarly take care of buffer idr? >>>> >>>> Will try it latter :) >>> >>> Sorry for the latter reply. The patch pass the testcase. >>> >>> Besides, should we apply this patch first to deal with the same UAF for >>> io_buffer_idr before convert to XArray? >>> >>> https://lore.kernel.org/io-uring/20210308065903.2228332-2-yangerkun@xxxxxxxxxx/T/#u >> >> Agree, and then defer an xarray conversion to 5.13. I'll take a look at >> your patch and get it applied. > > That one is very broken, it both fails removal cases and it's got leak > issues too. > > I'm going to take a look at just doing xarray instead. Something like the below - tested as working for me, and has no leaks. From: Jens Axboe <axboe@xxxxxxxxx> Subject: [PATCH] io_uring: convert io_buffer_idr to XArray Like we did for the personality idr, convert the IO buffer idr to use XArray. This avoids a use-after-free on removal of entries, since idr doesn't like doing so from inside an iterator. Fixes: 5a2e745d4d43 ("io_uring: buffer registration infrastructure") Cc: stable@xxxxxxxxxxxxxxx Reported-by: Hulk Robot <hulkci@xxxxxxxxxx> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx> --- fs/io_uring.c | 49 ++++++++++++++++++++++--------------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 05adc4887ef3..3d259a120f0d 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -402,7 +402,8 @@ struct io_ring_ctx { struct socket *ring_sock; #endif - struct idr io_buffer_idr; + struct xarray io_buffer; + u32 io_buffer_next; struct xarray personalities; u32 pers_next; @@ -1135,7 +1136,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p) init_waitqueue_head(&ctx->cq_wait); INIT_LIST_HEAD(&ctx->cq_overflow_list); init_completion(&ctx->ref_comp); - idr_init(&ctx->io_buffer_idr); + xa_init_flags(&ctx->io_buffer, XA_FLAGS_ALLOC1); xa_init_flags(&ctx->personalities, XA_FLAGS_ALLOC1); mutex_init(&ctx->uring_lock); init_waitqueue_head(&ctx->wait); @@ -2843,7 +2844,7 @@ static struct io_buffer *io_buffer_select(struct io_kiocb *req, size_t *len, lockdep_assert_held(&req->ctx->uring_lock); - head = idr_find(&req->ctx->io_buffer_idr, bgid); + head = xa_load(&req->ctx->io_buffer, bgid); if (head) { if (!list_empty(&head->list)) { kbuf = list_last_entry(&head->list, struct io_buffer, @@ -2851,7 +2852,7 @@ static struct io_buffer *io_buffer_select(struct io_kiocb *req, size_t *len, list_del(&kbuf->list); } else { kbuf = head; - idr_remove(&req->ctx->io_buffer_idr, bgid); + __xa_erase(&req->ctx->io_buffer, bgid); } if (*len > kbuf->len) *len = kbuf->len; @@ -3892,7 +3893,7 @@ static int __io_remove_buffers(struct io_ring_ctx *ctx, struct io_buffer *buf, } i++; kfree(buf); - idr_remove(&ctx->io_buffer_idr, bgid); + __xa_erase(&ctx->io_buffer, bgid); return i; } @@ -3910,7 +3911,7 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags) lockdep_assert_held(&ctx->uring_lock); ret = -ENOENT; - head = idr_find(&ctx->io_buffer_idr, p->bgid); + head = xa_load(&ctx->io_buffer, p->bgid); if (head) ret = __io_remove_buffers(ctx, head, p->bgid, p->nbufs); if (ret < 0) @@ -3993,21 +3994,20 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags) lockdep_assert_held(&ctx->uring_lock); - list = head = idr_find(&ctx->io_buffer_idr, p->bgid); + list = head = xa_load(&ctx->io_buffer, p->bgid); ret = io_add_buffers(p, &head); - if (ret < 0) - goto out; + if (ret >= 0 && !list) { + u32 id = -1U; - if (!list) { - ret = idr_alloc(&ctx->io_buffer_idr, head, p->bgid, p->bgid + 1, - GFP_KERNEL); - if (ret < 0) { + ret = __xa_alloc_cyclic(&ctx->io_buffer, &id, head, + XA_LIMIT(0, USHRT_MAX), + &ctx->io_buffer_next, GFP_KERNEL); + if (ret < 0) __io_remove_buffers(ctx, head, p->bgid, -1U); - goto out; - } + else + ret = id; } -out: if (ret < 0) req_set_fail_links(req); @@ -8333,19 +8333,14 @@ static int io_eventfd_unregister(struct io_ring_ctx *ctx) return -ENXIO; } -static int __io_destroy_buffers(int id, void *p, void *data) -{ - struct io_ring_ctx *ctx = data; - struct io_buffer *buf = p; - - __io_remove_buffers(ctx, buf, id, -1U); - return 0; -} - static void io_destroy_buffers(struct io_ring_ctx *ctx) { - idr_for_each(&ctx->io_buffer_idr, __io_destroy_buffers, ctx); - idr_destroy(&ctx->io_buffer_idr); + struct io_buffer *buf; + unsigned long index; + + xa_for_each(&ctx->io_buffer, index, buf) + __io_remove_buffers(ctx, buf, index, -1U); + xa_destroy(&ctx->io_buffer); } static void io_req_cache_free(struct list_head *list, struct task_struct *tsk) -- 2.30.2 -- Jens Axboe
