IORING_OP_PROVIDE_BUFFERS range validation issue

[lano1106]

the problem is definitely in my code but I think that this should definitely be addressed as this might be a security issue allowing someone to make the kernel write pretty much anywhere.

I provide to io_uring an 8 buffers long GID. So bid valid range should be 0-7.

My code is storing -1 into a the cqe associated user structure unsigned short bid var (65535) when it is processing a recv() error. It should not return the invalid bid value back to io_uring in that situation but it did mistakenly.

io_uring did accept back this invalid bid but more worrisome, it did handle another recv() by writing into this reused invalid bid...
I am not sure if the kernel did succeed in writing there but my app did SEGV when trying to access the memory...

I assume that the kernel did succeed to write there fine or else recv call would have failed but I caught the problem by analyzing a core dump following a SEGV while processing a successful recv() cqe containing the bid 65535!

buffers base address was: 0x556c3a9f5000
invalid bid buffer address outside the process virtual space: 0x556c8a9f0000
individual bufsz: 20480

IMHO, this is clearly hacking material...

[lano1106]

ps. it did happen with kernel v6.0.7 but this is certainly there for every io_uring version...

also, this is through the legacy buffer provisioning setup. Not with the newer buffer ring system...

[axboe]

I'll take a look, do you have the code you used? Will save me some time. In general, we should be doing access_ok() for any provided buffer. If the index is missed and we allowed the recv or write, then presumably that buffer index ended up with valid memory for that process. But I'll double check.

[lano1106]

very trivial to reproduce IMHO... I initialized buffers with those parameters:
[2022-11-09 12:56:44] INFO WSCONT/initGroupBuffer Provide 8 20480 bytes buffers to gid 2 from address 0x0x556c3a9f5000 (163840)

I believe that you do not even need to start using the buffers to do:
io_uring_prep_provide_buffers(sqe, 0x0x556c3a9f5000, 20480, 1, 2, 65535);

next and check what io_uring will do with it...

My confidence level that there is a real problem concerning this is very high...

[axboe]

Can you please describe in detail what you are doing. It may be obvious to you since you've been looking at it, but snippets of details do not provide a full picture for me.

Looking at provide buffers, the first thing we do is:

        if (check_mul_overflow((unsigned long)p->len, (unsigned long)p->nbufs,
				&size))
		return -EOVERFLOW;
	if (check_add_overflow((unsigned long)p->addr, size, &tmp_check))
		return -EOVERFLOW;

	size = (unsigned long)p->len * p->nbufs;
	if (!access_ok(u64_to_user_ptr(p->addr), size))
		return -EFAULT;

which will check if that range is valid for the process. This is just a basic check, once you do recv/read later on to use the buffer, then we may indeed fail writing to it. Which should trigger an -EFAULT cqe->res or similar.

It's very possible that I'm missing something here, which is why I asked for the details on the issue.

[lano1106]

You are really near the problem. Notice that the offset (aka seq->off or bid) is not applied before calling access_ok() !!

[axboe]

What offset?

Edit: the offset is just the buffer group ID, that doesn't factor into the range at all. If you disagree, please be explicit and let's avoid guessing games on my part...

[lano1106]

seq->off

the buffer address becomes:
sqe->addr+seq->len*seq->off

io_provide_buffers_prep() is testing the base address of the group buffers. not the bid buffer address

[axboe]

The range provided is addr + nbufs * len_of_each_buf. sqe->off is the buffer group ID where the buffers will be stored, selected by the program setting IOSQE_BUFFER_SELECT and picking that ->buf_group for it.

For the provided buffer, sqe->off has nothing to do with the address, it's just the group indexing.

[lano1106]

seq->off contains the bid.

p->bgid = READ_ONCE(sqe->buf_group);
tmp = READ_ONCE(sqe->off);
	if (tmp > USHRT_MAX)
		return -E2BIG;
	p->bid = tmp;

maybe I did misuse the API but I have always provided the group base address when returning individual BID buffer and it does work perfectly fine...

[axboe]

When you issue a read/recv, you just provide the buffer group, but the buffer itself. And yes, sqe->off will be the address + offset for the operation on the selected buffer. For the provide buffer operation itself, sqe->off is which group to provide the buffer in.

Since we kind of keep dancing around what the issue is here (not sure why you're not just stating it?!), here's what I did:

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdlib.h>
#include <string.h>
#include <liburing.h>

int main(int argc, char *argv[])
{
	struct io_uring ring;
	struct io_uring_sqe *sqe;
	struct io_uring_cqe *cqe;
	unsigned long *ptr;
	char foo[64];
	int fds[2];

	io_uring_queue_init(8, &ring, 0);
	sqe = io_uring_get_sqe(&ring);
	io_uring_prep_provide_buffers(sqe, 0x556c3a9f5000, 20480, 1, 2, 65535);
	io_uring_submit(&ring);
	io_uring_wait_cqe(&ring, &cqe);
	printf("pbuf cqe res %d\n", cqe->res);
	io_uring_cqe_seen(&ring, cqe);

	if (pipe(fds) < 0) {
		perror("pipe");
		return 1;
	}

	sqe = io_uring_get_sqe(&ring);
	io_uring_prep_read(sqe, fds[0], NULL, 20480, 0);
	sqe->flags |= IOSQE_BUFFER_SELECT;
	sqe->buf_group = 2;

	io_uring_submit(&ring);

	memset(foo, 0x89, sizeof(foo));
	write(fds[1], foo, sizeof(foo));

	io_uring_wait_cqe(&ring, &cqe);
	printf("read cqe res %d\n", cqe->res);
	io_uring_cqe_seen(&ring, cqe);

	return 0;
}

which returns:

axboe@m1pro-kvm ~> ./ol
pbuf cqe res 0
read cqe res -14

which is what I'd expect. The access_ok() is just a basic check, the actual read hits -EFAULT when trying to use a buffer in that group. In the spirit of not wasting time, please be explicit in a) what you are running exactly, and b) what you see in terms of output.

[lano1106]

First, your test will obviously not work since you provide a random address. I got 0x556c3a9f5000 from posix_memalign().

Next, I do pass a valid but wrong address. I started to mix recv() with recvmsg() to use kTLS with TLSv1.2 and I didn't know if recvmsg() could use provided buffers too. So I have static buffer for recvmsg and I set bid to 65535 when handling those recvmsg to differentiate them from recv() results. The mess up the code did was to return the recvmsg buffer with bid 65535 in a very specific context.

but what is not validated by io_provide_buffers_prep() is the BID value. So when io_uring does return me the invalid BID value and when I recalculate the BID address from the group base address, I get the SEGV address.

IOW, the returned buffer, yet valid, did not belong to the group buffer.

this is totally optional but maybe a max_bid value could stored when the buffer list is created so that the bid value could be validated when individual buffers are returned...

Beside that io_uring is all fine.

[axboe]

If allocate memory and write beyond that memory in the application, that may not be an immediate segfault. But it's undefined behavior, depends on what the heap looks like. Is it possible that this is what you are seeing? Doesn't really matter if you or the kernel writes to it in that regard.

I agree that if BID rolls around for provide buffer prep, that should be an EINVAL condition. If you pass 65535 then you should only be able to add a single buffer, which would then have ID 65535. I don't think we need to store that, since 64k-1 is always going to be the max value.

But I don't think there are any security concerns here, just confusing since we don't EINVAL if you attempt to add something where "start ID + nbufs" > 64k-1.

[axboe]

Something like this:

diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index 25cd724ade18..e2c46889d5fa 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -346,6 +346,8 @@ int io_provide_buffers_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe
 	tmp = READ_ONCE(sqe->off);
 	if (tmp > USHRT_MAX)
 		return -E2BIG;
+	if (tmp + p->nbufs >= USHRT_MAX)
+		return -EINVAL;
 	p->bid = tmp;
 	return 0;
 }

Can you confirm this fixes the case for you? If so, I'm going to queue it up and add attribution to you as the reporter. Is Olivier Langlois <olivier@trillion01.com> the right name and email to use?

[lano1106]

you are 100% right about finally not being a security problem.

I know that sometimes my explanations are not 100% clear. Let me try to reword the situation. io_uring does not validate the provided bid value.

So if I create 8 buffers group.

If I return to io_uring a bogus individual buffer with a bid value outside the valid range, io_uring will accept it as long as the buffer address is valid... This will in turn make the application explode when the invalid bid is returned...

This is totally arguable whether or not io_uring should/could shield the user about this but that was surprising to me... I just wanted to report this discovery...

Yes, the email address you have is good

[axboe]

I guess you could argue that the wrap is expected, so when you do:

Add 128 buffers starting at 65535

You get the following range of BIDs: [65535, 0, 1, 2, ...] and this patch would break that use case, which very well might be a valid use case.

[lano1106]

you are right again about breaking a valid use case.

I am not sure whether or not it would be a good thing to validate the bid value.

if it is, you cannot validate it solely from the info passed along the request imho. Some info about the group would need to be stored when the group is created. Not much would be needed. Only the group base address and the max_bid would be required.

With this info, you could validate that

1. The passed address is inside the group space
2. bid is within the group

[axboe]

You can validate the the ID is unique, as that's the only problematic case. Having the values wrap might be confusing, but it could also be a valid use case. There's no max group buffer ID being different, they are all 64k-1 max. That's irrespective of what you first asked to store in there with your first IORING_OP_PROVIDE_BUFFERS command.

This leaves me confused about your 1 and 2 points here. There's no need for the buffers within a group to have any relation to each other, and bid is always within the group as it's 0..64k-1 always.

[lano1106]

I see that there is still a misunderstanding about what we each are saying. It is not impossible that it is caused by me not using correctly the API.

To me, there are 2 steps performed by the IORING_OP_PROVIDE_BUFFERS request.

1. An initial call to create a buffer group - io_uring_prep_provide_buffers(sqe, 0x556c3a9f5000, 20480, 8, 2, 0);
2. Return an individual buffer to an existing group io_uring_prep_provide_buffers(sqe, 0x556c3a9f5000+3*20480, 20480, 1, 2, 3)

If calling io_uring_prep_provide_buffers() in context no.2 you could do:

io_uring_prep_provide_buffers(sqe, any_random_valid_address, 20480, 1, 2, 65535)

and this would currently work.

I guess that what you are saying is that you could technically add new additional buffers to an existing group and that would be perfectly a valid operation... I have never used the API that way... My problem is when I return a buffer to the group after io_uring did return it to its user... Maybe there is no easy way to deal wit that...

[axboe]

I think your misunderstanding is in thinking that when the buffer group is created in step 1, that defines the limit of that group. There is no such thing, the limits are ALWAYS 0..65535 for the BID in any group. This is why the group creation isn't a separate thing from adding buffers, the fact that it gets done automatically when you add buffers to a group that doesn't currently exist is purely implementation detail and in no way defines the limits of the group.

Not sure where this came from, it's surely not documented in any way that an initial buffer add would state anything about future limits of that group.

[axboe]

That said, clearly it would be a good idea to note this in the man page explicitly.

[lano1106]

AFAIK, it is stated nowhere. At least when I did write that code. The doc has tremendously improved since then.

[axboe]

Sent out the patch and added a man page comment as well on the range, hope that helps. I'll close this one.