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.
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.
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. - 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'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.
Kevin