| Message ID | 9b5cd13bc9e9c570978ec25b25ba5e4081b3d56b.1703126594.git.nabijaczleweli@nabijaczleweli.xyz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Avoid unprivileged splice(file->)/(->socket) pipe exclusion | expand |
On Thu, 21 Dec 2023 at 04:09, Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> wrote: > > Potentially-blocking splice_reads are allowed for normal filesystems > like NFS because they're blessed by root. > > FUSE is commonly used suid-root, and allows anyone to trivially create > a file that, when spliced from, will just sleep forever with the pipe > lock held. > > The only way IPC to the fusing process could be avoided is if > !(ff->open_flags & FOPEN_DIRECT_IO) and the range was already cached > and we weren't past the end. Just refuse it. How is this not going to cause regressions out there? We need to find an alternative to refusing splice, since this is not going to fly, IMO. Thanks, Miklos
On Wed, Jan 10, 2024 at 02:43:04PM +0100, Miklos Szeredi wrote: > On Thu, 21 Dec 2023 at 04:09, Ahelenia Ziemiańska > <nabijaczleweli@nabijaczleweli.xyz> wrote: > > Potentially-blocking splice_reads are allowed for normal filesystems > > like NFS because they're blessed by root. > > > > FUSE is commonly used suid-root, and allows anyone to trivially create > > a file that, when spliced from, will just sleep forever with the pipe > > lock held. > > > > The only way IPC to the fusing process could be avoided is if > > !(ff->open_flags & FOPEN_DIRECT_IO) and the range was already cached > > and we weren't past the end. Just refuse it. > How is this not going to cause regressions out there? In "[PATCH v2 14/11] fuse: allow splicing to trusted mounts only" splicing is re-enabled for mounts made by the real root. > We need to find an alternative to refusing splice, since this is not > going to fly, IMO. The alternative is to not hold the lock. See the references in the cover letter for why this wasn't done. IMO a potential slight perf hit flies more than a total exclusion on the pipe.
On Wed, 10 Jan 2024 at 16:19, Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> wrote: > > We need to find an alternative to refusing splice, since this is not > > going to fly, IMO. > The alternative is to not hold the lock. See the references in the > cover letter for why this wasn't done. IMO a potential slight perf > hit flies more than a total exclusion on the pipe. IDGI. This will make splice(2) return EINVAL for unprivileged fuse files, right? That would be a regression, not a perf hit, if the application is not falling back to plain read; a reasonable scenario, considering splice from files (including fuse) has worked on linux for a *long* time. Thanks, Mikos
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index a660f1f21540..20bb16ddfcc9 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -3200,6 +3200,21 @@ static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off, return ret; } +static long fuse_splice_read(struct file *in, loff_t *ppos, + struct pipe_inode_info *pipe, size_t len, + unsigned int flags) +{ + struct inode *inode = file_inode(in); + + if (fuse_is_bad(inode)) + return -EIO; + + if (get_fuse_conn(inode)->trusted) + return filemap_splice_read(in, ppos, pipe, len, flags); + + return -EINVAL; +} + static const struct file_operations fuse_file_operations = { .llseek = fuse_file_llseek, .read_iter = fuse_file_read_iter, @@ -3212,7 +3227,7 @@ static const struct file_operations fuse_file_operations = { .lock = fuse_file_lock, .get_unmapped_area = thp_get_unmapped_area, .flock = fuse_file_flock, - .splice_read = filemap_splice_read, + .splice_read = fuse_splice_read, .splice_write = iter_file_splice_write, .unlocked_ioctl = fuse_file_ioctl, .compat_ioctl = fuse_file_compat_ioctl, diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 1df83eebda92..463c5d4ad8b4 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -818,6 +818,9 @@ struct fuse_conn { /* Is statx not implemented by fs? */ unsigned int no_statx:1; + /* Do we trust this connection to always respond? */ + bool trusted:1; + /** The number of requests waiting for completion */ atomic_t num_waiting; diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 5f1be1da92ce..fce0fe24899a 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -1448,6 +1448,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) fc->delete_stale = true; fc->auto_submounts = true; fc->sync_fs = true; + fc->trusted = true; /* Tell FUSE to split requests that exceed the virtqueue's size */ fc->max_pages_limit = min_t(unsigned int, fc->max_pages_limit,
Potentially-blocking splice_reads are allowed for normal filesystems like NFS because they're blessed by root. FUSE is commonly used suid-root, and allows anyone to trivially create a file that, when spliced from, will just sleep forever with the pipe lock held. The only way IPC to the fusing process could be avoided is if !(ff->open_flags & FOPEN_DIRECT_IO) and the range was already cached and we weren't past the end. Just refuse it. virtiofs behaves like a normal filesystem and can only be mounted by root, it's unaffected by use of a new "trusted" connection flag. This may be extended to include real FUSE mounts by processes which aren't suid, to match the semantics for normal filesystems. Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> --- fs/fuse/file.c | 17 ++++++++++++++++- fs/fuse/fuse_i.h | 3 +++ fs/fuse/virtio_fs.c | 1 + 3 files changed, 20 insertions(+), 1 deletion(-)