[linux-morello] Re: [PATCH v2 7/8] io_uring: Add explicit capability checks

On 05/06/2023 17:46, Luca Vizzarro wrote:

Whenever a GUP call is made, a user address in the form of a 64-bit integer is used to lookup its corresponding page. When working in PCuABI, this means that the metadata of the user capability gets discarded, hence any access made by the GUP is not checked in hardware.

This commit introduces explicit capability checks whenever a call to the current mm through the GUP functions is made.

Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com

include/linux/io_uring.h | 2 +- io_uring/kbuf.c | 10 +++++++++- io_uring/net.c | 3 +-- io_uring/rsrc.c | 17 ++++++++++++++--- io_uring/rsrc.h | 4 ++-- io_uring/rw.c | 2 +- io_uring/uring_cmd.c | 6 +++++- 7 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h index 0ded9e271523..b10dd794e7ab 100644 --- a/include/linux/io_uring.h +++ b/include/linux/io_uring.h @@ -36,7 +36,7 @@ struct io_uring_cmd { }; #if defined(CONFIG_IO_URING) -int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, +int io_uring_cmd_import_fixed(user_uintptr_t ubuf, unsigned long len, int rw, struct iov_iter *iter, void *ioucmd); void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret, ssize_t res2); void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd, diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c index 70056c27d778..525a4f51b6b2 100644 --- a/io_uring/kbuf.c +++ b/io_uring/kbuf.c @@ -494,6 +494,9 @@ static int io_add_buffers(struct io_ring_ctx *ctx, struct io_provide_buf *pbuf, cond_resched(); }

• if (!check_user_ptr_rw(pbuf->addr, addr - pbuf->addr))

It is too late to make this check. At this point all the buffers have already been added to bl.

The natural place to add the check would be in io_provide_buffers_prep(), just after access_ok(). In general check_user_ptr* is naturally paired with access_ok(), as they have a similar role.

That being said, we may not want a check here at all in the end, see my latest reply on v1 [1].

[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...

• return -EFAULT;
  

• return i ? 0 : -ENOMEM;

} @@ -587,7 +590,12 @@ 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_read((void __user *)reg.ring_addr, pages_size)) {

I think it would be better to have this check earlier, just after the sanity checks for ring_addr and ring_entries. This requires moving the pages_size calculation too, but I think it makes more sense and avoids any side-effect in case of failure (so the kfree() is not needed any more).

• kfree(free_bl);
  

• return -EFAULT;
  

• }
• pages = io_pin_pages(reg.ring_addr, pages_size, &nr_pages); if (IS_ERR(pages)) { kfree(free_bl);

diff --git a/io_uring/net.c b/io_uring/net.c index cebbec06e670..677c01699007 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -1088,9 +1088,8 @@ int io_send_zc(struct io_kiocb *req, unsigned int issue_flags) return io_setup_async_addr(req, &__address, issue_flags); if (zc->flags & IORING_RECVSEND_FIXED_BUF) {

• /* TODO [PCuABI] - capability checks for uaccess */
  

  ret = io_import_fixed(ITER_SOURCE, &msg.msg_iter, req->imu,

• 			user_ptr_addr(zc->buf), zc->len);
  

• 			(user_uintptr_t)zc->buf, zc->len);
  

  if (unlikely(ret)) return ret; msg.sg_from_iter = io_sg_from_iter;

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index 9e716fef91d7..29402d47015f 100644 --- a/io_uring/rsrc.c +++ b/io_uring/rsrc.c @@ -1285,7 +1285,12 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, struct iovec *iov, return 0; ret = -ENOMEM;

• /* TODO [PCuABI] - capability checks for uaccess */

• /*

• * Explicit user pointer checking is not needed during registration as
  

• * we are already doing it in operation. Specifically whenever we import
  

• * user data for either IORING_OP_{READ,WRITE}_FIXED operations.
  

Not very clear, we don't really import user data in such operations, we rather use the pre-registered buffers. It would also be good to specify that we check the addr field of the SQE (which must point within the registered buffer), not the pointer to the buffer itself.

• */
  

  pages = io_pin_pages(user_ptr_addr(iov->iov_base), iov->iov_len, &nr_pages); if (IS_ERR(pages)) {

@@ -1317,7 +1322,7 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, struct iovec *iov, size -= vec_len; } /* store original address for later verification */

• imu->ubuf = user_ptr_addr(iov->iov_base);

• imu->ubuf = (user_uintptr_t)iov->iov_base; imu->ubuf_end = imu->ubuf + iov->iov_len; imu->nr_bvecs = nr_pages; *pimu = imu;

@@ -1396,11 +1401,17 @@ int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg, int io_import_fixed(int ddir, struct iov_iter *iter, struct io_mapped_ubuf *imu,

• 	   u64 buf_addr, size_t len)
  

• 	   user_uintptr_t buf_addr, size_t len)
  

{ u64 buf_end; size_t offset;

• if (ddir == ITER_SOURCE &&

•    !check_user_ptr_write((void __user *)buf_addr, len))
  

• return -EFAULT;
  

• if (ddir == ITER_DEST &&

•    !check_user_ptr_read((void __user *)buf_addr, len))
  

Isn't read/write the wrong way round? ITER_SOURCE replaces WRITE precisely because the latter was causing confusions - see Al Viro's commit message (patch 1).

Also I'd put these checks last, after the overflow check and the range check.

• return -EFAULT;
  

  if (WARN_ON_ONCE(!imu)) return -EFAULT; if (unlikely(check_add_overflow(buf_addr, (u64)len, &buf_end)))

diff --git a/io_uring/rsrc.h b/io_uring/rsrc.h index 81445a477622..2eae25325e20 100644 --- a/io_uring/rsrc.h +++ b/io_uring/rsrc.h @@ -46,7 +46,7 @@ struct io_rsrc_node { }; struct io_mapped_ubuf {

• u64 ubuf;

• user_uintptr_t ubuf; u64 ubuf_end; unsigned int nr_bvecs; unsigned long acct_pages;

@@ -66,7 +66,7 @@ void io_rsrc_node_switch(struct io_ring_ctx *ctx, int io_import_fixed(int ddir, struct iov_iter *iter, struct io_mapped_ubuf *imu,

• 	   u64 buf_addr, size_t len);
  

• 	   user_uintptr_t buf_addr, size_t len);
  

In general we should prefer void __user * to represent user pointers, as this is clearer / more standard, and provides much better type safety (any integer can be passed if we use user_uintptr_t). Also, much fewer casts needed in this case! user_uintptr_t is mainly useful to represent values that may or may not be a user pointer, while here we are always dealing with a user pointer.

void __io_sqe_buffers_unregister(struct io_ring_ctx *ctx); int io_sqe_buffers_unregister(struct io_ring_ctx *ctx); diff --git a/io_uring/rw.c b/io_uring/rw.c index 07b9c58e782c..02d1831cd390 100644 --- a/io_uring/rw.c +++ b/io_uring/rw.c @@ -365,7 +365,7 @@ static struct iovec *__io_import_iovec(int ddir, struct io_kiocb *req, if (opcode == IORING_OP_READ_FIXED || opcode == IORING_OP_WRITE_FIXED) { ret = io_import_fixed(ddir, iter, req->imu,

• 		      user_ptr_addr(rw->addr), rw->len);
  

• 		      (user_uintptr_t)rw->addr, rw->len);
  

  if (ret) return ERR_PTR(ret); return NULL;

diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index 4d2d2e3f885e..9ce8f49d369f 100644 --- a/io_uring/uring_cmd.c +++ b/io_uring/uring_cmd.c @@ -100,6 +100,10 @@ int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) index = array_index_nospec(req->buf_index, ctx->nr_user_bufs); req->imu = ctx->user_bufs[index]; io_req_set_rsrc_node(req, ctx, 0);

• if (!check_user_ptr_rw((void __user *)req->imu->ubuf,
  

• 		       req->imu->ubuf_end - req->imu->ubuf))
  

I don't think we should do this. For buffers registered with IORING_REGISTER_BUFFERS & co, our agreed approach is to check only SQE::adr and not the pointer to the registered buffer. This patch already ensures that SQE::addr is checked for all the affected operations (READ_FIXED, WRITE_FIXED, SEND_ZC and URING_CMD), but for URING_CMD it ends up checking both SQE::addr (via io_uring_cmd_import_fixed()) and the pre-registered buffer pointer here.

That also means that we shouldn't need the change to struct io_mapped_buf::ubuf; we do not actually need to keep the original pointer to the registered buffer as things stand.

Kevin

• 	return -EFAULT;
  

  } ioucmd->cmd = sqe->cmd; ioucmd->cmd_op = READ_ONCE(sqe->cmd_op);

@@ -153,7 +157,7 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) return IOU_ISSUE_SKIP_COMPLETE; } -int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, +int io_uring_cmd_import_fixed(user_uintptr_t ubuf, unsigned long len, int rw, struct iov_iter *iter, void *ioucmd) { struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);