[PATCH 5.6] io_uring: NULL-deref for IOSQE_{ASYNC,DRAIN}

[PATCH 5.6] io_uring: NULL-deref for IOSQE_{ASYNC,DRAIN}

[Pavel Begunkov]

Processing links, io_submit_sqe() prepares requests, drops sqes, and
passes them with sqe=NULL to io_queue_sqe(). There IOSQE_DRAIN and/or
IOSQE_ASYNC requests will go through the same prep, which doesn't expect
sqe=NULL and fail with NULL pointer deference.

Always do full prepare including io_alloc_async_ctx() for linked
requests, and then it can skip the second preparation.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 55afae6f0cf4..9d43efbec960 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4813,6 +4813,9 @@ static int io_req_defer_prep(struct io_kiocb *req,
 {
 	ssize_t ret = 0;
 
+	if (!sqe)
+		return 0;
+
 	if (io_op_defs[req->opcode].file_table) {
 		ret = io_grab_files(req);
 		if (unlikely(ret))
@@ -5655,6 +5658,11 @@ static bool io_submit_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 		if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
 			req->flags |= REQ_F_LINK;
 			INIT_LIST_HEAD(&req->link_list);
+
+			if (io_alloc_async_ctx(req)) {
+				ret = -EAGAIN;
+				goto err_req;
+			}
 			ret = io_req_defer_prep(req, sqe);
 			if (ret)
 				req->flags |= REQ_F_FAIL_LINK;
-- 
2.24.0

Re: [PATCH 5.6] io_uring: NULL-deref for IOSQE_{ASYNC,DRAIN}

[Pavel Begunkov]

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

On 13/03/2020 22:29, Pavel Begunkov wrote:
> Processing links, io_submit_sqe() prepares requests, drops sqes, and
> passes them with sqe=NULL to io_queue_sqe(). There IOSQE_DRAIN and/or
> IOSQE_ASYNC requests will go through the same prep, which doesn't expect
> sqe=NULL and fail with NULL pointer deference.
> 
> Always do full prepare including io_alloc_async_ctx() for linked
> requests, and then it can skip the second preparation.

BTW, linked_timeout test fails for a good reason. The test passes NULL buffer to
writev and expects it to -EFAULT in io_req_defer_prep(). However,
io_submit_sqe() catches this case (see head of a link case), sets
REQ_F_FAIL_LINK and allows it to fail with -ECANCELED in io_queue_link_head().


> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 55afae6f0cf4..9d43efbec960 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -4813,6 +4813,9 @@ static int io_req_defer_prep(struct io_kiocb *req,
>  {
>  	ssize_t ret = 0;
>  
> +	if (!sqe)
> +		return 0;
> +
>  	if (io_op_defs[req->opcode].file_table) {
>  		ret = io_grab_files(req);
>  		if (unlikely(ret))
> @@ -5655,6 +5658,11 @@ static bool io_submit_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  		if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
>  			req->flags |= REQ_F_LINK;
>  			INIT_LIST_HEAD(&req->link_list);
> +
> +			if (io_alloc_async_ctx(req)) {
> +				ret = -EAGAIN;
> +				goto err_req;
> +			}
>  			ret = io_req_defer_prep(req, sqe);
>  			if (ret)
>  				req->flags |= REQ_F_FAIL_LINK;
> 

-- 
Pavel Begunkov


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

Re: [PATCH 5.6] io_uring: NULL-deref for IOSQE_{ASYNC,DRAIN}

[Pavel Begunkov]

On 13/03/2020 22:29, Pavel Begunkov wrote:
> Processing links, io_submit_sqe() prepares requests, drops sqes, and
> passes them with sqe=NULL to io_queue_sqe(). There IOSQE_DRAIN and/or
> IOSQE_ASYNC requests will go through the same prep, which doesn't expect
> sqe=NULL and fail with NULL pointer deference.
> 
> Always do full prepare including io_alloc_async_ctx() for linked
> requests, and then it can skip the second preparation.

Hmm, found unreliably failing the across-fork test. I don't know whether it's
this patch specific, but need to take a look there first.

> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 55afae6f0cf4..9d43efbec960 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -4813,6 +4813,9 @@ static int io_req_defer_prep(struct io_kiocb *req,
>  {
>  	ssize_t ret = 0;
>  
> +	if (!sqe)
> +		return 0;
> +
>  	if (io_op_defs[req->opcode].file_table) {
>  		ret = io_grab_files(req);
>  		if (unlikely(ret))
> @@ -5655,6 +5658,11 @@ static bool io_submit_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  		if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
>  			req->flags |= REQ_F_LINK;
>  			INIT_LIST_HEAD(&req->link_list);
> +
> +			if (io_alloc_async_ctx(req)) {
> +				ret = -EAGAIN;
> +				goto err_req;
> +			}
>  			ret = io_req_defer_prep(req, sqe);
>  			if (ret)
>  				req->flags |= REQ_F_FAIL_LINK;
> 

-- 
Pavel Begunkov

Re: [PATCH 5.6] io_uring: NULL-deref for IOSQE_{ASYNC,DRAIN}

[Pavel Begunkov]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

On 13/03/2020 23:28, Pavel Begunkov wrote:
> Hmm, found unreliably failing the across-fork test. I don't know whether it's
> this patch specific, but need to take a look there first.

It's good to go, just used outdated tests.
The reproducer is attached.

> 
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  fs/io_uring.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 55afae6f0cf4..9d43efbec960 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -4813,6 +4813,9 @@ static int io_req_defer_prep(struct io_kiocb *req,
>>  {
>>  	ssize_t ret = 0;
>>  
>> +	if (!sqe)
>> +		return 0;
>> +
>>  	if (io_op_defs[req->opcode].file_table) {
>>  		ret = io_grab_files(req);
>>  		if (unlikely(ret))
>> @@ -5655,6 +5658,11 @@ static bool io_submit_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>>  		if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
>>  			req->flags |= REQ_F_LINK;
>>  			INIT_LIST_HEAD(&req->link_list);
>> +
>> +			if (io_alloc_async_ctx(req)) {
>> +				ret = -EAGAIN;
>> +				goto err_req;
>> +			}
>>  			ret = io_req_defer_prep(req, sqe);
>>  			if (ret)
>>  				req->flags |= REQ_F_FAIL_LINK;
>>
> 

-- 
Pavel Begunkov

[-- Attachment #2: read-write2.c --]
[-- Type: text/x-csrc, Size: 2609 bytes --]

#include <errno.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/poll.h>
#include <sys/eventfd.h>
#include "liburing.h"

#define BS		4096
static struct iovec vecs[1];
static int no_read;

static int create_file(const char *file)
{
	ssize_t ret;
	char *buf;
	int fd;

	buf = malloc(BS);
	memset(buf, 0xaa, BS);

	fd = open(file, O_WRONLY | O_CREAT, 0644);
	if (fd < 0) {
		perror("open file");
		return 1;
	}
	ret = write(fd, buf, BS);
	close(fd);
	return ret != BS;
}


static int create_buffers(void)
{
	if (posix_memalign(&vecs[0].iov_base, BS, BS))
		return 1;
	vecs[0].iov_len = BS;
	return 0;
}

static int test_io(const char *file)
{
	const int nr_links = 100;
	const int link_len = 100;
	const int nr_sqes = nr_links * link_len;
	struct io_uring_sqe *sqe;
	struct io_uring_cqe *cqe;
	struct io_uring ring;
	int i, fd, ret;
	static int warned;

	fd = open(file, O_WRONLY);
	if (fd < 0) {
		perror("file open");
		goto err;
	}

	ret = io_uring_queue_init(nr_sqes, &ring, 0);
	if (ret) {
		fprintf(stderr, "ring create failed: %d\n", ret);
		goto err;
	}

	for (int i = 0; i < nr_links; ++i) {
		for (int j = 0; j < link_len; ++j) {
			sqe = io_uring_get_sqe(&ring);
			if (!sqe) {
				fprintf(stderr, "sqe get failed\n");
				goto err;
			}
			io_uring_prep_writev(sqe, fd, &vecs[0], 1, 0);
			sqe->flags |= IOSQE_ASYNC;
			if (j != link_len - 1)
				sqe->flags |= IOSQE_IO_LINK;
		}
	}

	ret = io_uring_submit(&ring);
	if (ret != nr_sqes) {
		fprintf(stderr, "submit got %d, wanted %d\n", ret, nr_sqes);
		goto err;
	}

	for (i = 0; i < nr_sqes; i++) {
		ret = io_uring_wait_cqe(&ring, &cqe);
		if (ret) {
			fprintf(stderr, "wait_cqe=%d\n", ret);
			goto err;
		}
		if (cqe->res == -EINVAL) {
			if (!warned) {
				fprintf(stdout, "Non-vectored IO not "
					"supported, skipping\n");
				warned = 1;
				no_read = 1;
			}
		} else if (cqe->res != BS) {
			fprintf(stderr, "cqe res %d, wanted %d\n", cqe->res, BS);
			goto err;
		}
		io_uring_cqe_seen(&ring, cqe);
	}

	io_uring_queue_exit(&ring);
	close(fd);
	return 0;
err:
	if (fd != -1)
		close(fd);
	return 1;
}

int main(int argc, char *argv[])
{
	if (create_file(".basic-rw")) {
		fprintf(stderr, "file creation failed\n");
		goto err;
	}
	if (create_buffers()) {
		fprintf(stderr, "file creation failed\n");
		goto err;
	}


	test_io(".basic-rw");
	unlink(".basic-rw");
	return 0;
err:
	unlink(".basic-rw");
	return 1;
}

Re: [PATCH 5.6] io_uring: NULL-deref for IOSQE_{ASYNC,DRAIN}

[Jens Axboe]

On 3/13/20 1:29 PM, Pavel Begunkov wrote:
> Processing links, io_submit_sqe() prepares requests, drops sqes, and
> passes them with sqe=NULL to io_queue_sqe(). There IOSQE_DRAIN and/or
> IOSQE_ASYNC requests will go through the same prep, which doesn't expect
> sqe=NULL and fail with NULL pointer deference.
> 
> Always do full prepare including io_alloc_async_ctx() for linked
> requests, and then it can skip the second preparation.

Thanks, applied.

-- 
Jens Axboe

Re: [PATCH 5.6] io_uring: NULL-deref for IOSQE_{ASYNC,DRAIN}

[Jens Axboe]

On 3/13/20 3:29 PM, Pavel Begunkov wrote:
> On 13/03/2020 23:28, Pavel Begunkov wrote:
>> Hmm, found unreliably failing the across-fork test. I don't know whether it's
>> this patch specific, but need to take a look there first.
> 
> It's good to go, just used outdated tests.
> The reproducer is attached.

I integrated this into the existing read-write in liburing, thanks.

-- 
Jens Axboe