From 984824db844a9bd6e9e15ee469241982526a6ccd Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 18 May 2022 10:40:01 +0200 Subject: [PATCH] io_uring: don't use ERR_PTR for user pointers ERR_PTR abuses the high bits of a pointer to transport error information. This is only safe for kernel pointers and not user pointers. Fix io_buffer_select and its helpers to just return NULL for failure and get rid of this abuse. Signed-off-by: Christoph Hellwig Link: https://lore.kernel.org/r/20220518084005.3255380-3-hch@lst.de Signed-off-by: Jens Axboe --- fs/io_uring.c | 83 +++++++++++++++++++++++---------------------------- 1 file changed, 37 insertions(+), 46 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 53ddfb57e4be..9cac9b0ffebe 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3512,11 +3512,8 @@ static int io_buffer_add_list(struct io_ring_ctx *ctx, } static void __user *io_provided_buffer_select(struct io_kiocb *req, size_t *len, - struct io_buffer_list *bl, - unsigned int issue_flags) + struct io_buffer_list *bl) { - void __user *ret = ERR_PTR(-ENOBUFS); - if (!list_empty(&bl->buf_list)) { struct io_buffer *kbuf; @@ -3527,11 +3524,9 @@ static void __user *io_provided_buffer_select(struct io_kiocb *req, size_t *len, req->flags |= REQ_F_BUFFER_SELECTED; req->kbuf = kbuf; req->buf_index = kbuf->bid; - ret = u64_to_user_ptr(kbuf->addr); + return u64_to_user_ptr(kbuf->addr); } - - io_ring_submit_unlock(req->ctx, issue_flags); - return ret; + return NULL; } static void __user *io_ring_buffer_select(struct io_kiocb *req, size_t *len, @@ -3544,7 +3539,7 @@ static void __user *io_ring_buffer_select(struct io_kiocb *req, size_t *len, if (unlikely(smp_load_acquire(&br->tail) == head)) { io_ring_submit_unlock(req->ctx, issue_flags); - return ERR_PTR(-ENOBUFS); + return NULL; } head &= bl->mask; @@ -3562,22 +3557,19 @@ static void __user *io_ring_buffer_select(struct io_kiocb *req, size_t *len, req->buf_list = bl; req->buf_index = buf->bid; - if (!(issue_flags & IO_URING_F_UNLOCKED)) - return u64_to_user_ptr(buf->addr); - - /* - * If we came in unlocked, we have no choice but to - * consume the buffer here. This does mean it'll be - * pinned until the IO completes. But coming in - * unlocked means we're in io-wq context, hence there - * should be no further retry. For the locked case, the - * caller must ensure to call the commit when the - * transfer completes (or if we get -EAGAIN and must - * poll or retry). - */ - req->buf_list = NULL; - bl->head++; - io_ring_submit_unlock(req->ctx, issue_flags); + if (issue_flags & IO_URING_F_UNLOCKED) { + /* + * If we came in unlocked, we have no choice but to consume the + * buffer here. This does mean it'll be pinned until the IO + * completes. But coming in unlocked means we're in io-wq + * context, hence there should be no further retry. For the + * locked case, the caller must ensure to call the commit when + * the transfer completes (or if we get -EAGAIN and must poll + * or retry). + */ + req->buf_list = NULL; + bl->head++; + } return u64_to_user_ptr(buf->addr); } @@ -3586,20 +3578,19 @@ static void __user *io_buffer_select(struct io_kiocb *req, size_t *len, { struct io_ring_ctx *ctx = req->ctx; struct io_buffer_list *bl; + void __user *ret = NULL; io_ring_submit_lock(req->ctx, issue_flags); bl = io_buffer_get_list(ctx, req->buf_index); - if (unlikely(!bl)) { - io_ring_submit_unlock(req->ctx, issue_flags); - return ERR_PTR(-ENOBUFS); + if (likely(bl)) { + if (bl->buf_nr_pages) + ret = io_ring_buffer_select(req, len, bl, issue_flags); + else + ret = io_provided_buffer_select(req, len, bl); } - - /* selection helpers drop the submit lock again, if needed */ - if (bl->buf_nr_pages) - return io_ring_buffer_select(req, len, bl, issue_flags); - - return io_provided_buffer_select(req, len, bl, issue_flags); + io_ring_submit_unlock(req->ctx, issue_flags); + return ret; } #ifdef CONFIG_COMPAT @@ -3621,8 +3612,8 @@ static ssize_t io_compat_import(struct io_kiocb *req, struct iovec *iov, len = clen; buf = io_buffer_select(req, &len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; req->rw.addr = (unsigned long) buf; iov[0].iov_base = buf; req->rw.len = iov[0].iov_len = (compat_size_t) len; @@ -3644,8 +3635,8 @@ static ssize_t __io_iov_buffer_select(struct io_kiocb *req, struct iovec *iov, if (len < 0) return -EINVAL; buf = io_buffer_select(req, &len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; req->rw.addr = (unsigned long) buf; iov[0].iov_base = buf; req->rw.len = iov[0].iov_len = len; @@ -3702,8 +3693,8 @@ static struct iovec *__io_import_iovec(int rw, struct io_kiocb *req, if (opcode == IORING_OP_READ || opcode == IORING_OP_WRITE) { if (io_do_buffer_select(req)) { buf = io_buffer_select(req, &sqe_len, issue_flags); - if (IS_ERR(buf)) - return ERR_CAST(buf); + if (!buf) + return ERR_PTR(-ENOBUFS); req->rw.addr = (unsigned long) buf; req->rw.len = sqe_len; } @@ -4642,8 +4633,8 @@ static int io_nop(struct io_kiocb *req, unsigned int issue_flags) size_t len = 1; buf = io_buffer_select(req, &len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; } __io_req_complete(req, issue_flags, 0, io_put_kbuf(req, issue_flags)); @@ -5779,8 +5770,8 @@ static int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags) void __user *buf; buf = io_buffer_select(req, &sr->len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; kmsg->fast_iov[0].iov_base = buf; kmsg->fast_iov[0].iov_len = sr->len; iov_iter_init(&kmsg->msg.msg_iter, READ, kmsg->fast_iov, 1, @@ -5843,8 +5834,8 @@ static int io_recv(struct io_kiocb *req, unsigned int issue_flags) void __user *buf; buf = io_buffer_select(req, &sr->len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; sr->buf = buf; }