On 2/7/24 1:56 AM, Christian Brauner wrote: > On Tue, Feb 06, 2024 at 03:47:18PM -0800, Bart Van Assche wrote: >> Calling kiocb_set_cancel_fn() without knowing whether the caller >> submitted a struct kiocb or a struct aio_kiocb is unsafe. Fix this by >> introducing the cancel_kiocb() method in struct file_operations. The >> following call trace illustrates that without this patch an >> out-of-bounds write happens if I/O is submitted by io_uring (from a >> phone with an ARM CPU and kernel 6.1): >> >> WARNING: CPU: 3 PID: 368 at fs/aio.c:598 kiocb_set_cancel_fn+0x9c/0xa8 >> Call trace: >> kiocb_set_cancel_fn+0x9c/0xa8 >> ffs_epfile_read_iter+0x144/0x1d0 >> io_read+0x19c/0x498 >> io_issue_sqe+0x118/0x27c >> io_submit_sqes+0x25c/0x5fc >> __arm64_sys_io_uring_enter+0x104/0xab0 >> invoke_syscall+0x58/0x11c >> el0_svc_common+0xb4/0xf4 >> do_el0_svc+0x2c/0xb0 >> el0_svc+0x2c/0xa4 >> el0t_64_sync_handler+0x68/0xb4 >> el0t_64_sync+0x1a4/0x1a8 >> >> The following patch has been used as the basis of this patch: Christoph >> Hellwig, "[PATCH 08/32] aio: replace kiocb_set_cancel_fn with a >> cancel_kiocb file operation", May 2018 >> (https://lore.kernel.org/all/20180515194833.6906-9-hch@xxxxxx/). > > What's changed that voids Al's objections on that patch from 2018? > Specifically > https://lore.kernel.org/all/20180406021553.GS30522@xxxxxxxxxxxxxxxxxx > https://lore.kernel.org/all/20180520052720.GY30522@xxxxxxxxxxxxxxxxxx Nothing, it's definitely still broken in the same UAF way. But so is the cancelation bits, the assumption of the kiocb being contained within some aio specific struct is just awful. Does the cancelation support even work, or is needed? It would seem a lot more prudent to get rid of it. Barring that, we should probably do something like the below upfront until the whole thing is sorted out. diff --git a/fs/aio.c b/fs/aio.c index bb2ff48991f3..159648f43238 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -593,6 +593,10 @@ void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel) struct kioctx *ctx = req->ki_ctx; unsigned long flags; + /* kiocb didn't come from aio, so just ignore it */ + if (!(iocb->ki_flags & IOCB_FROM_AIO)) + return; + if (WARN_ON_ONCE(!list_empty(&req->ki_list))) return; @@ -1509,7 +1513,7 @@ static int aio_prep_rw(struct kiocb *req, const struct iocb *iocb) req->ki_complete = aio_complete_rw; req->private = NULL; req->ki_pos = iocb->aio_offset; - req->ki_flags = req->ki_filp->f_iocb_flags; + req->ki_flags = req->ki_filp->f_iocb_flags | IOCB_FROM_AIO; if (iocb->aio_flags & IOCB_FLAG_RESFD) req->ki_flags |= IOCB_EVENTFD; if (iocb->aio_flags & IOCB_FLAG_IOPRIO) { diff --git a/include/linux/fs.h b/include/linux/fs.h index ed5966a70495..2870b3d61866 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -352,6 +352,10 @@ enum rw_hint { * unrelated IO (like cache flushing, new IO generation, etc). */ #define IOCB_DIO_CALLER_COMP (1 << 22) + /* + * kiocb came from fs/aio.c + */ +#define IOCB_FROM_AIO (1 << 23) /* for use in trace events */ #define TRACE_IOCB_STRINGS \ -- Jens Axboe
