On 09/08/2023 12:07, 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 | 6 +++--- io_uring/kbuf.c | 26 ++++++++++++++------------ io_uring/net.c | 3 +-- io_uring/rsrc.c | 18 +++++++++++++++--- io_uring/rsrc.h | 2 +- io_uring/rw.c | 3 +-- io_uring/uring_cmd.c | 2 +- 7 files changed, 36 insertions(+), 24 deletions(-)
diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h index 7fe31b2cd02f..d285acebeee2 100644 --- a/include/linux/io_uring.h +++ b/include/linux/io_uring.h @@ -42,7 +42,7 @@ static inline const void *io_uring_sqe_cmd(const struct io_uring_sqe *sqe) } #if defined(CONFIG_IO_URING) -int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, +int io_uring_cmd_import_fixed(void __user *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, unsigned issue_flags); @@ -72,8 +72,8 @@ static inline void io_uring_free(struct task_struct *tsk) __io_uring_free(tsk); } #else -static inline int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
struct iov_iter *iter, void *ioucmd)+static inline int io_uring_cmd_import_fixed(void __user *ubuf, unsigned long len,
int rw, struct iov_iter *iter, void *ioucmd){ return -EOPNOTSUPP; } diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c index 625fba732db0..37a37383df47 100644 --- a/io_uring/kbuf.c +++ b/io_uring/kbuf.c @@ -561,7 +561,6 @@ static int io_pin_pbuf_ring(struct io_uring_buf_reg *reg, struct page **pages; int nr_pages;
- /* TODO [PCuABI] - capability checks for uaccess */ pages = io_pin_pages(reg->ring_addr, ring_size, &nr_pages); if (IS_ERR(pages)) return PTR_ERR(pages);
@@ -620,6 +619,17 @@ int io_register_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg) if (copy_io_uring_buf_reg_from_user(ctx, ®, arg)) return -EFAULT;
- if (!is_power_of_2(reg.ring_entries))
return -EINVAL;- /* cannot disambiguate full vs empty due to head/tail size */
- if (reg.ring_entries >= 65536)
return -EINVAL;- ring_size = reg.ring_entries * (io_in_compat64(ctx) ?
sizeof(struct compat_io_uring_buf) :sizeof(struct io_uring_buf));- if (reg.resv[0] || reg.resv[1] || reg.resv[2]) return -EINVAL; if (reg.flags & ~IOU_PBUF_RING_MMAP)
@@ -629,18 +639,14 @@ int io_register_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg) return -EFAULT; if (reg.ring_addr & ~PAGE_MASK) return -EINVAL;
if (unlikely(!check_user_ptr_read((void __user *)reg.ring_addr,ring_size)))
Nit: could fold that check together with !reg.ring_addr above. Also I wouldn't bother with unlikely() since the other checks don't use it (and are not any more likely to fail).
return -EFAULT;
- if (!is_power_of_2(reg.ring_entries))
return -EINVAL;- /* cannot disambiguate full vs empty due to head/tail size */
- if (reg.ring_entries >= 65536)
return -EINVAL;- if (unlikely(reg.bgid < BGID_ARRAY && !ctx->io_bl)) { int ret = io_init_bl_list(ctx); if (ret)
@@ -658,10 +664,6 @@ int io_register_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg) return -ENOMEM; }
- ring_size = reg.ring_entries * (io_in_compat64(ctx) ?
sizeof(struct compat_io_uring_buf) :sizeof(struct io_uring_buf));- if (!(reg.flags & IOU_PBUF_RING_MMAP)) ret = io_pin_pbuf_ring(®, ring_size, bl); else
diff --git a/io_uring/net.c b/io_uring/net.c index eb4795553bf8..0f3dbe4da64b 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -1133,9 +1133,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 */user_ptr_addr(zc->buf), zc->len);
zc->buf, zc->len);diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index 44abf9c74b86..217ac109f832 100644 --- a/io_uring/rsrc.c +++ b/io_uring/rsrc.c @@ -1192,7 +1192,14 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, struct iovec *iov, return 0; ret = -ENOMEM;
- /* TODO [PCuABI] - capability checks for uaccess */
- /*
* When working in PCuABI, explicit user pointer checking is not needed* during registration as we are already doing it in operation.* Specifically whenever we use the user data provided in the* pre-registered buffers for either IORING_OP_{READ,WRITE}_FIXED* operations.
It would be more accurate and generic to say it happens in io_import_fixed() just below (also covers IORING_RECVSEND_FIXED_BUF). It's not really clear what "whenever we use the data" means, in fact we have to make an explicit check _before_ we access the data.
*/@@ -1240,7 +1247,7 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, struct iovec *iov, off = user_ptr_addr(iov->iov_base) & ~PAGE_MASK; size = iov->iov_len; /* store original address for later verification */
- imu->ubuf = user_ptr_addr(iov->iov_base);
- imu->ubuf = (user_uintptr_t)iov->iov_base;
Looks like a leftover from v2?
imu->ubuf_end = imu->ubuf + iov->iov_len; imu->nr_bvecs = nr_pages; *pimu = imu; @@ -1327,8 +1334,9 @@ 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)
void __user *ubuf, size_t len){
- u64 buf_addr = user_ptr_addr(ubuf); u64 buf_end; size_t offset;
@@ -1339,6 +1347,10 @@ int io_import_fixed(int ddir, struct iov_iter *iter, /* not inside the mapped region */ if (unlikely(buf_addr < imu->ubuf || buf_end > imu->ubuf_end)) return -EFAULT;
- if (unlikely(ddir == ITER_SOURCE && !check_user_ptr_read(ubuf, len)))
return -EFAULT;- if (unlikely(ddir == ITER_DEST && !check_user_ptr_write(ubuf, len)))
return -EFAULT;
Nit: could also merge these two ifs.
Kevin
/* * Might not be a start of buffer, set size appropriately diff --git a/io_uring/rsrc.h b/io_uring/rsrc.h index 0a8a95e9b99e..67a4929ec782 100644 --- a/io_uring/rsrc.h +++ b/io_uring/rsrc.h @@ -65,7 +65,7 @@ int io_queue_rsrc_removal(struct io_rsrc_data *data, unsigned idx, void *rsrc); int io_import_fixed(int ddir, struct iov_iter *iter, struct io_mapped_ubuf *imu,
u64 buf_addr, size_t len);
void __user *buf_addr, size_t len);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 e2107505649d..7ac10b37dece 100644 --- a/io_uring/rw.c +++ b/io_uring/rw.c @@ -370,8 +370,7 @@ static struct iovec *__io_import_iovec(int ddir, struct io_kiocb *req, ssize_t ret; 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);
ret = io_import_fixed(ddir, iter, req->imu, rw->addr, rw->len);diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index 5e32db48696d..535bc82a4ef8 100644 --- a/io_uring/uring_cmd.c +++ b/io_uring/uring_cmd.c @@ -148,7 +148,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(void __user *ubuf, unsigned long len, int rw, struct iov_iter *iter, void *ioucmd) { struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);