On 12-05-2023 18:24, Kevin Brodsky wrote:
On 10/05/2023 13:52, Tudor Cretu wrote:
diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c index 70056c27d778..d6e6227caab0 100644 --- a/io_uring/kbuf.c +++ b/io_uring/kbuf.c @@ -587,7 +587,10 @@ int io_register_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg) pages_size = io_in_compat64(ctx) ? size_mul(sizeof(struct compat_io_uring_buf), reg.ring_entries) : size_mul(sizeof(struct io_uring_buf), reg.ring_entries); - /* TODO [PCuABI] - capability checks for uaccess */
+ if (!check_user_ptr_rw((void __user *)reg.ring_addr, pages_size))
Maybe a question both for you and Tudor: do we actually have any idea whether the operation that will be performed on the registered buffer is R or W at this point? If not this is a pretty serious issue, as we must not reject say a read-only pointer if the buffer is only ever read from, not written to. The same question applies to all users of io_pin_pages(); for io_import_fixed() it looks the prototype already tells us about that.
IIUC, the registered buffers are only accesible by SQEs requests that read or receive data from a file or socket, so it should be enough to check only for read permission here.
I think you're right, though it's the other way round: read operations write to the provide buffer! FWIW the definitive list of operations allowed to use IOSQE_BUFFER_SELECT can be found in opdef.c (.buffer_select must be set to 1), there are 4 currently and all are reads. Which I suppose makes sense given the use-case.
This is also what I deduced. Also, thank you for the correction, that's right, these need to have write permissions.
For the io_pin_pages() in rsrc.c, they could be used for both read abd write operations as far as I can tell...
That seems to be he case, in fact AFAICT this can only be used together with IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED.
I've reached the same conclusion.
Overall I feel like we need to have a deeper think about this. There are multiple situations to consider, and multiple places where the checking could happen. __io_import_iovec() summarises the multiple situations quite well.
From what I've gathered:
- For IORING_REGISTER_BUFFERS and friends, it's fairly straightforward:
they can only be used together with IORING_OP_{READ,WRITE}_FIXED. Crucially, these requests are still required to specify addr and len, within the registered region. One option here would be to check only when processing the operation, not at the point of registration, since the latter can only be used through the former.
I think doing the check only when processing the operation is fine. But we should leave a comment at the registration point on why there is no need to check.
- For IORING_REGISTER_PBUF_RING and friends, the situation is different.
They can be used with operations specifying IOSQE_BUFFER_SELECT (4 allowed, see above), but here the operation does not specify the addr / len. We could still do the checking when processing the operation, since io_buffer_select() returns the original user pointer. It could be better to do it at the point of registration instead though (which is possible permission-wise because we know the operation will write to the buffer, as per above).
I think just doing the check at the registration point sounds good.
I'm sure I've missed something, so I'd be very grateful if someone could dig deeper into this and figure out all the angles. We should come up with a consistent approach, for instance checking as much as possible as soon as possible (when registering) or checking only at the point of processing the operation, not at the point of registration.
I've gone through the code and I confirm that your summary above is exhaustive and... exceptionally concise, well done!
I agree that a consistent approach would be nice, but I don't really think it's worth complicating the code in the case of IORING_REGISTER_BUFFERS or double-checking in the case of IORING_REGISTER_PBUF_RING. As long as all the possible registration-operation paths are checked and the decisions are documented, it should be enough. After all, these are exceptions to the standard... This is not a strong opinion, of course.
Thanks, Tudor
Kevin