From: Chris Mason The aio ring lives in a file on the aio pseudo filesystem and the ring VMA uses filemap_fault(). aio_ctx_aops doesn't have a ->read_folio because aio_setup_ring() puts all folios of the ring into the page cache. So a fault on the ring only works as long as it finds its folio in the page cache. If it doesn't, filemap_fault() calls ->read_folio without checking and we jump to NULL. We already fixed this twice this year. A forked child could inherit the ring mapping, fixed by commit c03ce4173c7b ("fs: aio: set VMA_DONTCOPY_BIT in mmap to fix NULL-pointer-dereference error"). And a partial mremap() could leave part of the ring mapped, fixed by commit 3adf7ae18bf4 ("fs: aio: reject partial mremap to avoid Null-pointer-dereference error"). Both times a VMA outlived the ring and the ring file was truncated under it. There's a third way to get there. kill_ioctx() and the error path of ioctx_alloc() unmap the ring with vm_munmap() and ignore the return value. If vm_munmap() has to wait for mmap_lock and a fatal signal is pending it returns -EINTR and the VMA stays. free_ioctx() then calls put_aio_ring_file() which truncates the ring file to zero. A fault that passed the i_size check in filemap_fault() before that doesn't find its folio anymore, allocates a new one and calls ->read_folio: fault on the ring io_destroy() with SIGKILL pending ================= ================================= vm_munmap() returns -EINTR filemap_fault() i_size check passes free_ioctx() put_aio_ring_file() truncate_setsize(inode, 0) no folio, allocate a new one filemap_read_folio() ->read_folio == NULL All that's needed is a second process that shares the mm. If a CLONE_VM child calls io_destroy() and gets SIGKILLed while it waits for mmap_lock the ring stays mapped for the threads of the parent. Any user can do that. Instead of plugging the routes one by one give the ring a ->read_folio that unlocks the folio and returns -EIO. All folios of a live ring are in the page cache so a fault can only end up there if the ring was truncated. The fault now gets SIGBUS. Nothing else reads the ring file and the aio filesystem uses noop_backing_dev_info so there's no readahead either. Making vm_munmap() unkillable in those two places would only close this one route and bring back what commit ae7987835643 ("mm: make vm_munmap killable") fixed. Fixes: 36bc08cc0170 ("fs/aio: Add support to aio ring pages migration") Cc: stable@vger.kernel.org Signed-off-by: Chris Mason Signed-off-by: Christian Brauner (Amutable) --- fs/aio.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/fs/aio.c b/fs/aio.c index 4f32b45cb8a0..893db6961ad3 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -528,7 +528,18 @@ static int aio_migrate_folio(struct address_space *mapping, struct folio *dst, #define aio_migrate_folio NULL #endif +/* + * filemap_fault() reads a folio that it did not find in the page cache. All + * folios of a live ring are there, so the ring was truncated under the fault. + */ +static int aio_read_folio(struct file *file, struct folio *folio) +{ + folio_unlock(folio); + return -EIO; +} + static const struct address_space_operations aio_ctx_aops = { + .read_folio = aio_read_folio, .dirty_folio = noop_dirty_folio, .migrate_folio = aio_migrate_folio, }; -- 2.53.0 FUSE passthrough opens the backing file anew whenever the FUSE file is opened. fuse_passthrough_open() passes the flags of that open to backing_file_open() which opens the backing file with vfs_open() and the credentials of the server. But vfs_open() doesn't go through may_open(). So the backing inode is never checked for S_APPEND and S_IMMUTABLE. The checks that did run were done on the FUSE inode and that one never has either flag. So anyone who may open the FUSE file for writing can overwrite an append-only backing file anywhere or write to an immutable one. The server only has to register the file and a read-only or even an O_PATH file descriptor is enough for that. Registering requires CAP_SYS_ADMIN but the writer can be any user that the server lets open the file. The FUSE inode is checked instead of the backing inode in a few more places: - do_mmap() refuses a shared mapping of an append-only file that was opened for writing. A FUSE file opened with O_RDWR | O_APPEND passes the open check and can then be mapped shared and written to at any offset. - fcntl(F_SETFL) and RWF_NOAPPEND refuse to take append mode away from a file on an append-only inode. On the FUSE file both succeed. A synchronous write still appends because the backing file was opened with O_APPEND. But an asynchronous write is submitted with the flags of the user's kiocb and lands at the given offset. Do the checks on the backing inode. backing_file_open() refuses to open an immutable inode for writing and an append-only inode for writing without O_APPEND, just like may_open(). backing_file_mmap() refuses a shared mapping of an append-only inode, just like do_mmap(). And writes and splices to a backing file in append mode fail if append mode was taken away from the user file. A FUSE file that was opened before the backing file became append-only can still write anywhere. That's what happens on the native filesystem as well and the backing file of such a file isn't in append mode. Two differences remain. F_SETFL succeeds and the next write fails. And FUSE reports EIO for a refused open because it turns every error from opening the backing file into EIO. overlayfs uses the same helpers but it mirrors S_APPEND and S_IMMUTABLE in its own inode and a chattr through the overlay never sets them on the upper inode. So for overlayfs this only changes something if the upper inode is changed behind the overlay's back. Fixes: 4a90451bbc7f ("fuse: implement open in passthrough mode") Reported-by: Chris Mason Cc: stable@vger.kernel.org # v6.9+ Signed-off-by: Christian Brauner (Amutable) --- fs/backing-file.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/fs/backing-file.c b/fs/backing-file.c index 5614cb7801e1..2424dc38d21a 100644 --- a/fs/backing-file.c +++ b/fs/backing-file.c @@ -36,9 +36,21 @@ struct file *backing_file_open(const struct file *user_file, int flags, const struct cred *cred) { const struct path *user_path = file_user_path(user_file); + struct inode *real_inode = d_inode(real_path->dentry); struct file *f; int error; + /* + * may_open() checked the inode of the user file, which need not carry + * S_APPEND and S_IMMUTABLE of the backing inode. + */ + if ((flags & O_ACCMODE) != O_RDONLY) { + if (IS_IMMUTABLE(real_inode)) + return ERR_PTR(-EPERM); + if (IS_APPEND(real_inode) && !(flags & O_APPEND)) + return ERR_PTR(-EPERM); + } + f = alloc_empty_backing_file(flags, cred, user_file); if (IS_ERR(f)) return f; @@ -263,6 +275,14 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter, if (!iov_iter_count(iter)) return 0; + /* + * F_SETFL and RWF_NOAPPEND can take append mode away from the user + * file. They checked its inode, not the backing one. + */ + if ((file->f_flags & O_APPEND) && !(flags & IOCB_APPEND) && + IS_APPEND(file_inode(file))) + return -EPERM; + ret = file_remove_privs(iocb->ki_filp); if (ret) return ret; @@ -309,6 +329,10 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe, if (!out->f_op->splice_write) return -EINVAL; + /* splice refuses a user file in append mode, so F_SETFL cleared it */ + if ((out->f_flags & O_APPEND) && IS_APPEND(file_inode(out))) + return -EPERM; + ret = file_remove_privs(iocb->ki_filp); if (ret) return ret; @@ -338,6 +362,10 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma, if (!can_mmap_file(file)) return -ENODEV; + /* do_mmap() checked the inode of the user file */ + if ((vma->vm_flags & VM_SHARED) && IS_APPEND(file_inode(file))) + return -EACCES; + vma_set_file(vma, file); scoped_with_creds(ctx->cred) { -- 2.53.0 From: Chris Mason Since commit 111a12b422a8 ("io_uring/rsrc: reject zero-length fixed buffer import") io_import_fixed() hands out an empty ITER_BVEC for a zero-length request. Its count is 0 and its bvec array is NULL. iov_iter_alignment() only checks the count for ITER_UBUF. For ITER_BVEC it calls iov_iter_alignment_bvec() and the do-while loop in there reads bvec->bv_len before it looks at the count. So it dereferences NULL. btrfs checks the alignment of a direct read with iov_iter_alignment() before it looks at the count and f2fs does the same. So any user that can open a file on btrfs can oops the kernel with a single zero-length IORING_OP_READ_FIXED on an O_DIRECT file descriptor. With IOSQE_ASYNC the oops happens in an io-wq worker. The dead worker stays on the worker list and when the task that submitted the request exits io-wq writes to the freed task_struct of the worker. That's just fallout from the oops though. Empty iterators with a NULL array aren't unusual. afs, tls and tipc create them as well. So leave io_uring alone and make iov_iter_alignment() return 0 for an empty iterator of any type. Fixes: 111a12b422a8 ("io_uring/rsrc: reject zero-length fixed buffer import") Cc: stable@vger.kernel.org Signed-off-by: Chris Mason Signed-off-by: Christian Brauner (Amutable) --- lib/iov_iter.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 2072c04e99d0..56c59a2b589d 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -848,6 +848,10 @@ unsigned long iov_iter_alignment(const struct iov_iter *i) return 0; } + /* An empty iterator may have no segment array at all. */ + if (!i->count) + return 0; + /* iovec and kvec have identical layouts */ if (likely(iter_is_iovec(i) || iov_iter_is_kvec(i))) return iov_iter_alignment_iovec(i); -- 2.53.0