[PATCH for-5.6] io_uring: fix use-after-free by io_cleanup_req()

[PATCH for-5.6] io_uring: fix use-after-free by io_cleanup_req()

[Pavel Begunkov]

io_cleanup_req() should be called before req->io is freed, and so
shouldn't be after __io_free_req() -> __io_req_aux_free(). Also,
it will be ignored for in io_free_req_many(), which use
__io_req_aux_free().

Place cleanup_req() into __io_req_aux_free().

Fixes: 99bc4c38537d774 ("io_uring: fix iovec leaks")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 11627818104e..c39a81f8f83d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1296,6 +1296,9 @@ static void __io_req_aux_free(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
+	if (req->flags & REQ_F_NEED_CLEANUP)
+		io_cleanup_req(req);
+
 	kfree(req->io);
 	if (req->file)
 		io_put_file(ctx, req->file, (req->flags & REQ_F_FIXED_FILE));
@@ -1307,9 +1310,6 @@ static void __io_free_req(struct io_kiocb *req)
 {
 	__io_req_aux_free(req);
 
-	if (req->flags & REQ_F_NEED_CLEANUP)
-		io_cleanup_req(req);
-
 	if (req->flags & REQ_F_INFLIGHT) {
 		struct io_ring_ctx *ctx = req->ctx;
 		unsigned long flags;
-- 
2.24.0

Re: [PATCH for-5.6] io_uring: fix use-after-free by io_cleanup_req()

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 1385 bytes --]

On 19/02/2020 00:19, Pavel Begunkov wrote:
> io_cleanup_req() should be called before req->io is freed, and so
> shouldn't be after __io_free_req() -> __io_req_aux_free(). Also,
> it will be ignored for in io_free_req_many(), which use
> __io_req_aux_free().

My bad, this got into rc2, and should be pretty easy to hit.
Please, patch this up.

> Place cleanup_req() into __io_req_aux_free().
> 
> Fixes: 99bc4c38537d774 ("io_uring: fix iovec leaks")
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 11627818104e..c39a81f8f83d 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1296,6 +1296,9 @@ static void __io_req_aux_free(struct io_kiocb *req)
>  {
>  	struct io_ring_ctx *ctx = req->ctx;
>  
> +	if (req->flags & REQ_F_NEED_CLEANUP)
> +		io_cleanup_req(req);
> +
>  	kfree(req->io);
>  	if (req->file)
>  		io_put_file(ctx, req->file, (req->flags & REQ_F_FIXED_FILE));
> @@ -1307,9 +1310,6 @@ static void __io_free_req(struct io_kiocb *req)
>  {
>  	__io_req_aux_free(req);
>  
> -	if (req->flags & REQ_F_NEED_CLEANUP)
> -		io_cleanup_req(req);
> -
>  	if (req->flags & REQ_F_INFLIGHT) {
>  		struct io_ring_ctx *ctx = req->ctx;
>  		unsigned long flags;
> 

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-5.6] io_uring: fix use-after-free by io_cleanup_req()

[Jens Axboe]

On 2/18/20 2:19 PM, Pavel Begunkov wrote:
> io_cleanup_req() should be called before req->io is freed, and so
> shouldn't be after __io_free_req() -> __io_req_aux_free(). Also,
> it will be ignored for in io_free_req_many(), which use
> __io_req_aux_free().
> 
> Place cleanup_req() into __io_req_aux_free().

I've applied this, but would be great if you could generate 5.x
patches against io_uring-5.x. This one didn't apply, as it's
done behind some of the 5.7 series you have.

-- 
Jens Axboe

Re: [PATCH for-5.6] io_uring: fix use-after-free by io_cleanup_req()

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 670 bytes --]

On 19/02/2020 03:13, Jens Axboe wrote:
> On 2/18/20 2:19 PM, Pavel Begunkov wrote:
>> io_cleanup_req() should be called before req->io is freed, and so
>> shouldn't be after __io_free_req() -> __io_req_aux_free(). Also,
>> it will be ignored for in io_free_req_many(), which use
>> __io_req_aux_free().
>>
>> Place cleanup_req() into __io_req_aux_free().
> 
> I've applied this, but would be great if you could generate 5.x
> patches against io_uring-5.x. This one didn't apply, as it's
> done behind some of the 5.7 series you have.
> 

Thanks, I cherry-picked it by didn't regenerate the patch.
I'll double check the next time

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]