fuse: Add support for passthrough read/write

fs/fuse/passthrough.c +0 -0

fs/fuse/passthrough.c +29 -0

+static ssize_t fuse_passthrough_read_write_iter(struct kiocb *iocb,
+					    struct iov_iter *iter, int do_write)
+{
+	ssize_t ret_val;
+	struct fuse_file *ff;
+	struct file *fuse_file, *passthrough_filp;
+	struct inode *fuse_inode, *passthrough_inode;
+
+	ff = iocb->ki_filp->private_data;
+	fuse_file = iocb->ki_filp;
+	passthrough_filp = ff->passthrough_filp;
+
+	/* lock passthrough file to prevent it from being released */
+	get_file(passthrough_filp);
+	iocb->ki_filp = passthrough_filp;
+	fuse_inode = fuse_file->f_path.dentry->d_inode;
+	passthrough_inode = file_inode(passthrough_filp);
+
+	if (do_write) {
+		if (!passthrough_filp->f_op->write_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->write_iter(iocb, iter);
+
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED) {
+			fsstack_copy_inode_size(fuse_inode, passthrough_inode);
+			fsstack_copy_attr_times(fuse_inode, passthrough_inode);
+		}
+	} else {
+		if (!passthrough_filp->f_op->read_iter)

Quoted 1 line · Nikhilesh Reddy

fs/fuse/passthrough.c +0 -0

[...]

Quoted 35 lines

fs/fuse/passthrough.c +35 -0

+static ssize_t fuse_passthrough_read_write_iter(struct kiocb *iocb,
+					    struct iov_iter *iter, int do_write)
+{
+	ssize_t ret_val;
+	struct fuse_file *ff;
+	struct file *fuse_file, *passthrough_filp;
+	struct inode *fuse_inode, *passthrough_inode;
+
+	ff = iocb->ki_filp->private_data;
+	fuse_file = iocb->ki_filp;
+	passthrough_filp = ff->passthrough_filp;
+
+	/* lock passthrough file to prevent it from being released */
+	get_file(passthrough_filp);
+	iocb->ki_filp = passthrough_filp;
+	fuse_inode = fuse_file->f_path.dentry->d_inode;
+	passthrough_inode = file_inode(passthrough_filp);
+
+	if (do_write) {
+		if (!passthrough_filp->f_op->write_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->write_iter(iocb, iter);
+
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED) {
+			fsstack_copy_inode_size(fuse_inode, passthrough_inode);
+			fsstack_copy_attr_times(fuse_inode, passthrough_inode);
+		}
+	} else {
+		if (!passthrough_filp->f_op->read_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->read_iter(iocb, iter);
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED)
+			fsstack_copy_attr_atime(fuse_inode, passthrough_inode);
+	}
+

fs/fuse/passthrough.c line 105

Quoted 4 lines

fs/fuse/passthrough.c +4 -0

+	iocb->ki_filp = fuse_file;
+
+	/* unlock passthrough file */
+	fput(passthrough_filp);

Why the get_file() and fput() in this method? This doesn't look right. There is no lock you're releasing between get_file() and fput(). What are they intended for?

Quoted 38 lines · Jann Horn

Quoted 1 line · Nikhilesh Reddy

fs/fuse/passthrough.c +0 -0

[...]

Quoted 35 lines

fs/fuse/passthrough.c +35 -0

+static ssize_t fuse_passthrough_read_write_iter(struct kiocb *iocb,
+					    struct iov_iter *iter, int do_write)
+{
+	ssize_t ret_val;
+	struct fuse_file *ff;
+	struct file *fuse_file, *passthrough_filp;
+	struct inode *fuse_inode, *passthrough_inode;
+
+	ff = iocb->ki_filp->private_data;
+	fuse_file = iocb->ki_filp;
+	passthrough_filp = ff->passthrough_filp;
+
+	/* lock passthrough file to prevent it from being released */
+	get_file(passthrough_filp);
+	iocb->ki_filp = passthrough_filp;
+	fuse_inode = fuse_file->f_path.dentry->d_inode;
+	passthrough_inode = file_inode(passthrough_filp);
+
+	if (do_write) {
+		if (!passthrough_filp->f_op->write_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->write_iter(iocb, iter);
+
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED) {
+			fsstack_copy_inode_size(fuse_inode, passthrough_inode);
+			fsstack_copy_attr_times(fuse_inode, passthrough_inode);
+		}
+	} else {
+		if (!passthrough_filp->f_op->read_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->read_iter(iocb, iter);
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED)
+			fsstack_copy_attr_atime(fuse_inode, passthrough_inode);
+	}
+

Quoted 8 lines

fs/fuse/passthrough.c line 105

Quoted 4 lines

fs/fuse/passthrough.c +4 -0

+	iocb->ki_filp = fuse_file;
+
+	/* unlock passthrough file */
+	fput(passthrough_filp);

Why the get_file() and fput() in this method? This doesn't look right. There is no lock you're releasing between get_file() and fput(). What are they intended for?

Hi

Thanks for reviewing the code.

The passthrough file could be released under our feet say if the userspace fuse daemon crashed or was killed ( while we are processing the read or the write) causing bad things to happen.
The calls here are to increase the count temporarily and then decrease it so that we dont release in the middle of a write and everything is gracefully handled...

I have a comment right before the get_file call above saying the same thing. Please let me know if you have any more questions.

Quoted 59 lines · Nikhilesh Reddy

Quoted 38 lines · Jann Horn

Quoted 1 line · Nikhilesh Reddy

fs/fuse/passthrough.c +0 -0

[...]

Quoted 35 lines

fs/fuse/passthrough.c +35 -0

+static ssize_t fuse_passthrough_read_write_iter(struct kiocb *iocb,
+					    struct iov_iter *iter, int do_write)
+{
+	ssize_t ret_val;
+	struct fuse_file *ff;
+	struct file *fuse_file, *passthrough_filp;
+	struct inode *fuse_inode, *passthrough_inode;
+
+	ff = iocb->ki_filp->private_data;
+	fuse_file = iocb->ki_filp;
+	passthrough_filp = ff->passthrough_filp;
+
+	/* lock passthrough file to prevent it from being released */
+	get_file(passthrough_filp);
+	iocb->ki_filp = passthrough_filp;
+	fuse_inode = fuse_file->f_path.dentry->d_inode;
+	passthrough_inode = file_inode(passthrough_filp);
+
+	if (do_write) {
+		if (!passthrough_filp->f_op->write_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->write_iter(iocb, iter);
+
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED) {
+			fsstack_copy_inode_size(fuse_inode, passthrough_inode);
+			fsstack_copy_attr_times(fuse_inode, passthrough_inode);
+		}
+	} else {
+		if (!passthrough_filp->f_op->read_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->read_iter(iocb, iter);
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED)
+			fsstack_copy_attr_atime(fuse_inode, passthrough_inode);
+	}
+

Quoted 8 lines

fs/fuse/passthrough.c line 105

Quoted 4 lines

fs/fuse/passthrough.c +4 -0

+	iocb->ki_filp = fuse_file;
+
+	/* unlock passthrough file */
+	fput(passthrough_filp);

Why the get_file() and fput() in this method? This doesn't look right. There is no lock you're releasing between get_file() and fput(). What are they intended for?

Hi

Thanks for reviewing the code.

The passthrough file could be released under our feet say if the userspace fuse daemon crashed or was killed ( while we are processing the read or the write) causing bad things to happen.
The calls here are to increase the count temporarily and then decrease it so that we dont release in the middle of a write and everything is gracefully handled...

Quoted 2 lines

I have a comment right before the get_file call above saying the same thing. Please let me know if you have any more questions.

If that is the case, why can't the passthrough file be released before the get_file() call, e.g. while the core processing the filesystem read request is entering fuse_passthrough_read_write_iter()?

As far as I can tell, you can drop the get_file() and fput() calls. fuse_setup_passthrough() already took a reference to the file for you, that reference can only be dropped in fuse_passthrough_release(), and the VFS ensures that no release call happens while a read or write is pending.

Quoted 67 lines · Jann Horn

Quoted 59 lines · Nikhilesh Reddy

Quoted 38 lines · Jann Horn

Quoted 1 line · Nikhilesh Reddy

fs/fuse/passthrough.c +0 -0

[...]

Quoted 35 lines

fs/fuse/passthrough.c +35 -0

+static ssize_t fuse_passthrough_read_write_iter(struct kiocb *iocb,
+					    struct iov_iter *iter, int do_write)
+{
+	ssize_t ret_val;
+	struct fuse_file *ff;
+	struct file *fuse_file, *passthrough_filp;
+	struct inode *fuse_inode, *passthrough_inode;
+
+	ff = iocb->ki_filp->private_data;
+	fuse_file = iocb->ki_filp;
+	passthrough_filp = ff->passthrough_filp;
+
+	/* lock passthrough file to prevent it from being released */
+	get_file(passthrough_filp);
+	iocb->ki_filp = passthrough_filp;
+	fuse_inode = fuse_file->f_path.dentry->d_inode;
+	passthrough_inode = file_inode(passthrough_filp);
+
+	if (do_write) {
+		if (!passthrough_filp->f_op->write_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->write_iter(iocb, iter);
+
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED) {
+			fsstack_copy_inode_size(fuse_inode, passthrough_inode);
+			fsstack_copy_attr_times(fuse_inode, passthrough_inode);
+		}
+	} else {
+		if (!passthrough_filp->f_op->read_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->read_iter(iocb, iter);
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED)
+			fsstack_copy_attr_atime(fuse_inode, passthrough_inode);
+	}
+

Quoted 8 lines

fs/fuse/passthrough.c line 105

Quoted 4 lines

fs/fuse/passthrough.c +4 -0

+	iocb->ki_filp = fuse_file;
+
+	/* unlock passthrough file */
+	fput(passthrough_filp);

Why the get_file() and fput() in this method? This doesn't look right. There is no lock you're releasing between get_file() and fput(). What are they intended for?

Hi

Thanks for reviewing the code.

The passthrough file could be released under our feet say if the userspace fuse daemon crashed or was killed ( while we are processing the read or the write) causing bad things to happen.
The calls here are to increase the count temporarily and then decrease it so that we dont release in the middle of a write and everything is gracefully handled...

Quoted 2 lines

I have a comment right before the get_file call above saying the same thing. Please let me know if you have any more questions.

If that is the case, why can't the passthrough file be released before the get_file() call, e.g. while the core processing the filesystem read request is entering fuse_passthrough_read_write_iter()?

Quoted 5 lines

As far as I can tell, you can drop the get_file() and fput() calls. fuse_setup_passthrough() already took a reference to the file for you, that reference can only be dropped in fuse_passthrough_release(), and the VFS ensures that no release call happens while a read or write is pending.

I just feel uncomfortable with dropping them. I thought they could be released ( i/o ) takes longer than the actual execution... but if i can be sure of it then maybe..

Add support for filesystem passthrough read/write of files when enabled in userspace through the option FUSE_PASSTHROUGH.

fs/fuse/passthrough.c +0 -0

fs/fuse/passthrough.c +12 -0

+static ssize_t fuse_passthrough_read_write_iter(struct kiocb *iocb,
+					    struct iov_iter *iter, int do_write)
+{
+	ssize_t ret_val;
+	struct fuse_file *ff;
+	struct file *fuse_file, *passthrough_filp;
+	struct inode *fuse_inode, *passthrough_inode;
+
+	ff = iocb->ki_filp->private_data;
+	fuse_file = iocb->ki_filp;
+	passthrough_filp = ff->passthrough_filp;
+

fs/fuse/passthrough.c +40 -0

+
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED) {
+			fsstack_copy_inode_size(fuse_inode, passthrough_inode);
+			fsstack_copy_attr_times(fuse_inode, passthrough_inode);
+		}
+	} else {
+		if (!passthrough_filp->f_op->read_iter)
+			return -EIO;
+		ret_val = passthrough_filp->f_op->read_iter(iocb, iter);
+		if (ret_val >= 0 || ret_val == -EIOCBQUEUED)
+			fsstack_copy_attr_atime(fuse_inode, passthrough_inode);
+	}
+
+	iocb->ki_filp = fuse_file;
+
+	/* unlock passthrough file */
+	fput(passthrough_filp);
+
+	return ret_val;
+}
+
+ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+	return fuse_passthrough_read_write_iter(iocb, to, 0);
+}
+
+ssize_t fuse_passthrough_write_iter(struct kiocb *iocb, struct iov_iter *from)
+{
+	return fuse_passthrough_read_write_iter(iocb, from, 1);
+}
+
+void fuse_passthrough_release(struct fuse_file *ff)
+{
+	if (!(ff->passthrough_filp))
+		return;
+
+	/* Release the passthrough file. */
+	fput(ff->passthrough_filp);
+	ff->passthrough_filp = NULL;
+}

include/uapi/linux/fuse.h +2 -1

index c9aca04..a08933a 100644
@@ -250,6 +250,7 @@ struct fuse_file_lock {
 #define FUSE_ASYNC_DIO		(1 << 15)
 #define FUSE_WRITEBACK_CACHE	(1 << 16)
 #define FUSE_NO_OPEN_SUPPORT	(1 << 17)
+#define FUSE_PASSTHROUGH	(1 << 18)
 
 /**
  * CUSE INIT request/reply flags

@@ -480,7 +481,7 @@ struct fuse_create_in {
 struct fuse_open_out {
 	uint64_t	fh;
 	uint32_t	open_flags;
-	uint32_t	padding;
+	int32_t         passthrough_fd;
 };
 
 struct fuse_release_in {

Signature · 10 lines

1.8.2.1


-- 
Thanks
Nikhilesh Reddy

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Uh... how do you know at this point that the file is actually writable? Normally, e.g. vfs_write() will ensure that the file is writable, and e.g. generic_file_write_iter() won't check for writability as far as I can tell. This might allow someone to use the passthrough mechanism to overwrite a file he is only allowed to read, but not write, like /etc/passwd.

Also, I think this might bypass mandatory locks, the security_file_permission hook (which seems like a bad idea anyway though), inotify/fsnotify and sb_start_write.

Hi
Thanks for your review again :)

Quoted 7 lines

Uh... how do you know at this point that the file is actually writable? Normally, e.g. vfs_write() will ensure that the file is writable, and e.g. generic_file_write_iter() won't check for writability as far as I can tell. This might allow someone to use the passthrough mechanism to overwrite a file he is only allowed to read, but not write, like /etc/passwd.

I considered adding the checks ( the same ones that VFS does) but not sure if we need to.
So the user will need to construct a fuse filesystem ( that opens for O_READONLY even though the user asks for a O_RDWR from the FUSE open) and then mount it , with CAP_SYS_ADMIN for which you need to be root but once he has that he should be able to easily get to the files without needing to go through FUSE right using CAP_DAC_OVERRIDE?

Am i missing something? Please do help me understand.

But yes if really needed I can add additional checks once i understand it

Quoted 4 lines

Also, I think this might bypass mandatory locks, the security_file_permission hook (which seems like a bad idea anyway though), inotify/fsnotify and sb_start_write.

Can you please elaborate/clarify further? I am am not sure what you mean.

Quoted 18 lines · Nikhilesh Reddy

Hi
Thanks for your review again :)

Quoted 7 lines

Uh... how do you know at this point that the file is actually writable? Normally, e.g. vfs_write() will ensure that the file is writable, and e.g. generic_file_write_iter() won't check for writability as far as I can tell. This might allow someone to use the passthrough mechanism to overwrite a file he is only allowed to read, but not write, like /etc/passwd.

I considered adding the checks ( the same ones that VFS does) but not sure if we need to.
So the user will need to construct a fuse filesystem ( that opens for O_READONLY even though the user asks for a O_RDWR from the FUSE open) and then mount it , with CAP_SYS_ADMIN for which you need to be root but once he has that he should be able to easily get to the files without needing to go through FUSE right using CAP_DAC_OVERRIDE?

Quoted 3 lines

Am i missing something? Please do help me understand.

But yes if really needed I can add additional checks once i understand it

On most Linux desktop systems, and on many servers, the userland "fuse" package is installed, which ships with a setuid root helper "fusermount":

$ ls -l /bin/fusermount
-rwsr-xr-x 1 root root 30800 May 21  2015 /bin/fusermount

This setuid helper allows any user to mount FUSE filesystems anywhere he wants. This works as follows: main() calls mount_fuse(), which opens /dev/fuse by calling open_fuse_device(). mount_fuse() then makes sure that the caller has write access to the directory he is about to mount over using check_perm(), then calls mount() via do_mount(). mount_fuse() returns the /dev/fuse fd, which is then sent to the invoker of fusermount through a unix domain socket.

(What the setuid binary does control are the mount options; those are used to enforce that the user can't mount filesystems that are accessible for other users.)

Note that at no point, any data is sent to or read from the FUSE control fd by fusermount. Therefore, the init reply that is processed in process_init_reply() and determines whether passthrough will be enabled is controlled by the unprivileged caller.

This fusermount mechanism is used by pseudo-filesystems like sshfs in order to allow unprivileged users to use them.

fusermount aside, there is also an (as far as I know) pending patch with the intent to make FUSE usable in user namespaces, which is going to allow unprivileged users to mount FUSE filesystems even without a userspace helper: https://lkml.org/lkml/2015/12/2/472

(Note that this is very different from CUSE, which by design must never be exposed to non-root code.)

Quoted 6 lines

Quoted 5 lines

Also, I think this might bypass mandatory locks, the security_file_permission hook (which seems like a bad idea anyway though), inotify/fsnotify and sb_start_write.

Can you please elaborate/clarify further? I am am not sure what you mean.

Quoted 49 lines · Jann Horn

Quoted 18 lines · Nikhilesh Reddy

Hi
Thanks for your review again :)

Quoted 7 lines

Uh... how do you know at this point that the file is actually writable? Normally, e.g. vfs_write() will ensure that the file is writable, and e.g. generic_file_write_iter() won't check for writability as far as I can tell. This might allow someone to use the passthrough mechanism to overwrite a file he is only allowed to read, but not write, like /etc/passwd.

I considered adding the checks ( the same ones that VFS does) but not sure if we need to.
So the user will need to construct a fuse filesystem ( that opens for O_READONLY even though the user asks for a O_RDWR from the FUSE open) and then mount it , with CAP_SYS_ADMIN for which you need to be root but once he has that he should be able to easily get to the files without needing to go through FUSE right using CAP_DAC_OVERRIDE?

Quoted 3 lines

Am i missing something? Please do help me understand.

But yes if really needed I can add additional checks once i understand it

On most Linux desktop systems, and on many servers, the userland "fuse" package is installed, which ships with a setuid root helper "fusermount":

$ ls -l /bin/fusermount
-rwsr-xr-x 1 root root 30800 May 21  2015 /bin/fusermount

This setuid helper allows any user to mount FUSE filesystems anywhere he wants. This works as follows: main() calls mount_fuse(), which opens /dev/fuse by calling open_fuse_device(). mount_fuse() then makes sure that the caller has write access to the directory he is about to mount over using check_perm(), then calls mount() via do_mount(). mount_fuse() returns the /dev/fuse fd, which is then sent to the invoker of fusermount through a unix domain socket.

(What the setuid binary does control are the mount options; those are used to enforce that the user can't mount filesystems that are accessible for other users.)

Note that at no point, any data is sent to or read from the FUSE control fd by fusermount. Therefore, the init reply that is processed in process_init_reply() and determines whether passthrough will be enabled is controlled by the unprivileged caller.

This fusermount mechanism is used by pseudo-filesystems like sshfs in order to allow unprivileged users to use them.

Quoted 7 lines

fusermount aside, there is also an (as far as I know) pending patch with the intent to make FUSE usable in user namespaces, which is going to allow unprivileged users to mount FUSE filesystems even without a userspace helper: https://lkml.org/lkml/2015/12/2/472

(Note that this is very different from CUSE, which by design must never be exposed to non-root code.)

Thanks for the explanation ..I am convinced :)
will add the checks when i send out the next version ( Probably by end of
the week.. hopefully will be the last version :) :) )

Something on the lines of
if (!(file->f_mode & FMODE_WRITE))

	return -EBADF;
if (!(file->f_mode & FMODE_CAN_WRITE))
	return -EINVAL;

And
if (!(file->f_mode & FMODE_READ))

	return -EBADF;
if (!(file->f_mode & FMODE_CAN_READ))
	return -EINVAL;

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Also, I think this might bypass mandatory locks, the security_file_permission hook (which seems like a bad idea anyway though), inotify/fsnotify and sb_start_write.

Quoted 1 line

Quoted 1 line

Can you please elaborate/clarify further? I am am not sure what you mean.

Can you please also explain what you meant by :
"might bypass mandatory locks, the security_file_permission hook (which seems like a bad idea anyway though), inotify/fsnotify and sb_start_write."

+++ b/fs/fuse/passthrough.c

+void fuse_setup_passthrough(struct fuse_conn *fc, struct fuse_req *req)

fs/fuse/passthrough.c new file +1 -0

+	daemon_fd = (int)open_out->passthrough_fd;

+++ b/include/uapi/linux/fuse.h