[PATCH v3 0/6] fuse: Implement FUSE_HANDLE_KILLPRIV_V2 and enable SB_NOSEC

[PATCH v3 0/6] fuse: Implement FUSE_HANDLE_KILLPRIV_V2 and enable SB_NOSEC

[Vivek Goyal]

Hi All,

Please find attached V3 of the patches to enable SB_NOSEC for fuse. I
posted V1 and V2 here.

v2:
https://lore.kernel.org/linux-fsdevel/20200916161737.38028-1-vgoyal@redhat.com/
v1:
https://lore.kernel.org/linux-fsdevel/20200724183812.19573-1-vgoyal@redhat.com/

Changes since v2:

- Based on Miklos's feedback, dropped a patch where we send ATTR_MODE as
  that's racy. To help the case of writeback_cache with killpriv_v2, I
  fallback to a synchronous WRITE if suid/sgid is set on file.

I have generated these patches on top of.

https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git/log/?h=for-nex
+t

I have taken care of feedback from last round. For the case of random
write peformance has jumped from 50MB/s to 250MB/s. So I am really
looking forward to these changes so that fuse/virtiofs performance
can be improved for direct random writes.

Thanks
Vivek


Vivek Goyal (6):
  fuse: Introduce the notion of FUSE_HANDLE_KILLPRIV_V2
  fuse: Set FUSE_WRITE_KILL_PRIV in cached write path
  fuse: setattr should set FATTR_KILL_PRIV upon size change
  fuse: Don't send ATTR_MODE to kill suid/sgid for handle_killpriv_v2
  fuse: Add a flag FUSE_OPEN_KILL_PRIV for open() request
  fuse: Support SB_NOSEC flag to improve direct write performance

 fs/fuse/dir.c             |  4 +++-
 fs/fuse/file.c            | 16 +++++++++++++++-
 fs/fuse/fuse_i.h          |  6 ++++++
 fs/fuse/inode.c           | 17 ++++++++++++++++-
 include/uapi/linux/fuse.h | 18 +++++++++++++++++-
 5 files changed, 57 insertions(+), 4 deletions(-)

-- 
2.25.4

[PATCH v3 1/6] fuse: Introduce the notion of FUSE_HANDLE_KILLPRIV_V2

[Vivek Goyal]

We already have FUSE_HANDLE_KILLPRIV flag that says that file server will
remove suid/sgid/caps on truncate/chown/write. But that's little different
from what Linux VFS implements.

To be consistent with Linux VFS behavior what we want is.

- caps are always cleared on chown/write/truncate
- suid is always cleared on chown, while for truncate/write it is cleared
  only if caller does not have CAP_FSETID.
- sgid is always cleared on chown, while for truncate/write it is cleared
  only if caller does not have CAP_FSETID as well as file has group execute
  permission.

As previous flag did not provide above semantics. Implement a V2 of the
protocol with above said constraints.

Server does not know if caller has CAP_FSETID or not. So for the case
of write()/truncate(), client will send information in special flag to
indicate whether to kill priviliges or not. These changes are in subsequent
patches.

FUSE_HANDLE_KILLPRIV_V2 relies on WRITE being sent to server to clear
suid/sgid/security.capability. But with ->writeback_cache, WRITES are
cached in guest. So it is not recommended to use FUSE_HANDLE_KILLPRIV_V2
and writeback_cache together. Though it probably might be good enough
for lot of use cases.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/fuse_i.h          | 6 ++++++
 fs/fuse/inode.c           | 5 ++++-
 include/uapi/linux/fuse.h | 7 +++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index dbaae2f6c73e..3dd1578be405 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -631,6 +631,12 @@ struct fuse_conn {
 	/* show legacy mount options */
 	unsigned int legacy_opts_show:1;
 
+	/** fs kills suid/sgid/cap on write/chown/trunc. suid is
+	    killed on write/trunc only if caller did not have CAP_FSETID.
+	    sgid is killed on write/truncate only if caller did not have
+	    CAP_FSETID as well as file has group execute permission. */
+	unsigned handle_killpriv_v2:1;
+
 	/*
 	 * The following bitfields are only for optimization purposes
 	 * and hence races in setting them will not cause malfunction
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index d252237219bf..20740b61f12b 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -993,6 +993,8 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_args *args,
 			    !fuse_dax_check_alignment(fc, arg->map_alignment)) {
 				ok = false;
 			}
+			if (arg->flags & FUSE_HANDLE_KILLPRIV_V2)
+				fc->handle_killpriv_v2 = 1;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1035,7 +1037,8 @@ void fuse_send_init(struct fuse_conn *fc)
 		FUSE_WRITEBACK_CACHE | FUSE_NO_OPEN_SUPPORT |
 		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
 		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
-		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA;
+		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
+		FUSE_HANDLE_KILLPRIV_V2;
 #ifdef CONFIG_FUSE_DAX
 	if (fc->dax)
 		ia->in.flags |= FUSE_MAP_ALIGNMENT;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 8899e4862309..3ae3f222a0ed 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -172,6 +172,7 @@
  *  - add FUSE_WRITE_KILL_PRIV flag
  *  - add FUSE_SETUPMAPPING and FUSE_REMOVEMAPPING
  *  - add map_alignment to fuse_init_out, add FUSE_MAP_ALIGNMENT flag
+ *  - add FUSE_HANDLE_KILLPRIV_V2
  */
 
 #ifndef _LINUX_FUSE_H
@@ -316,6 +317,11 @@ struct fuse_file_lock {
  * FUSE_MAP_ALIGNMENT: init_out.map_alignment contains log2(byte alignment) for
  *		       foffset and moffset fields in struct
  *		       fuse_setupmapping_out and fuse_removemapping_one.
+ * FUSE_HANDLE_KILLPRIV_V2: fs kills suid/sgid/cap on write/chown/trunc.
+ * 			Upon write/truncate suid/sgid is only killed if caller
+ * 			does not have CAP_FSETID. Additionally upon
+ * 			write/truncate sgid is killed only if file has group
+ * 			execute permission. (Same as Linux VFS behavior).
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -344,6 +350,7 @@ struct fuse_file_lock {
 #define FUSE_NO_OPENDIR_SUPPORT (1 << 24)
 #define FUSE_EXPLICIT_INVAL_DATA (1 << 25)
 #define FUSE_MAP_ALIGNMENT	(1 << 26)
+#define FUSE_HANDLE_KILLPRIV_V2	(1 << 27)
 
 /**
  * CUSE INIT request/reply flags
-- 
2.25.4

[PATCH v3 2/6] fuse: Set FUSE_WRITE_KILL_PRIV in cached write path

[Vivek Goyal]

With HANDLE_KILLPRIV_V2, server will need to kill suid/sgid if caller
does not have CAP_FSETID. We already have a flag FUSE_WRITE_KILL_PRIV
in WRITE request and we already set it in direct I/O path.

To make it work in cached write path also, start setting FUSE_WRITE_KILL_PRIV
in this path too.

Set it only if fc->handle_killpriv_v2 is set. Otherwise client is responsible
for kill suid/sgid.

In case of direct I/O we set FUSE_WRITE_KILL_PRIV unconditionally because
we do't call file_remove_privs() in that path (with cache=none option).

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 172a0b1aa634..e40428f3d0f1 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1095,6 +1095,8 @@ static ssize_t fuse_send_write_pages(struct fuse_io_args *ia,
 
 	fuse_write_args_fill(ia, ff, pos, count);
 	ia->write.in.flags = fuse_write_flags(iocb);
+	if (fc->handle_killpriv_v2 && !capable(CAP_FSETID))
+		ia->write.in.write_flags |= FUSE_WRITE_KILL_PRIV;
 
 	err = fuse_simple_request(fc, &ap->args);
 	if (!err && ia->write.out.size > count)
-- 
2.25.4

[PATCH v3 3/6] fuse: setattr should set FATTR_KILL_PRIV upon size change

[Vivek Goyal]

If fc->handle_killpriv_v2 is enabled, we expect file server to clear
suid/sgid/security.capbility upon chown/truncate/write as appropriate.

Upon truncate (ATTR_SIZE), suid/sgid is cleared only if caller does
not have CAP_FSETID. File server does not know whether caller has
CAP_FSETID or not. Hence set FATTR_KILL_PRIV upon truncate to let
file server know that caller does not have CAP_FSETID and it should
kill suid/sgid as appropriate.

We don't have to send this information for chown (ATTR_UID/ATTR_GID)
as that always clears suid/sgid irrespective of capabilities of
calling process.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/dir.c             | 2 ++
 include/uapi/linux/fuse.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index c4a01290aec6..ecdb7895c156 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1575,6 +1575,8 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
 		/* For mandatory locking in truncate */
 		inarg.valid |= FATTR_LOCKOWNER;
 		inarg.lock_owner = fuse_lock_owner_id(fc, current->files);
+		if (fc->handle_killpriv_v2 && !capable(CAP_FSETID))
+			inarg.valid |= FATTR_KILL_PRIV;
 	}
 	fuse_setattr_fill(fc, &args, inode, &inarg, &outarg);
 	err = fuse_simple_request(fc, &args);
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 3ae3f222a0ed..7b8da0a2de0d 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -269,6 +269,7 @@ struct fuse_file_lock {
 #define FATTR_MTIME_NOW	(1 << 8)
 #define FATTR_LOCKOWNER	(1 << 9)
 #define FATTR_CTIME	(1 << 10)
+#define FATTR_KILL_PRIV	(1 << 14) /* Matches ATTR_KILL_PRIV */
 
 /**
  * Flags returned by the OPEN request
-- 
2.25.4

[PATCH v3 4/6] fuse: Don't send ATTR_MODE to kill suid/sgid for handle_killpriv_v2

[Vivek Goyal]

If client does a write() on a suid/sgid file, VFS will first call
fuse_setattr() with ATTR_KILL_S[UG]ID set. This requires sending
setattr to file server with ATTR_MODE set to kill suid/sgid. But
to do that client needs to know latest mode otherwise it is racy.

To reduce the race window, current code first call fuse_do_getattr()
to get latest ->i_mode and then resets suid/sgid bits and sends rest
to server with setattr(ATTR_MODE). This does not reduce the race
completely but narrows race window significantly.

With fc->handle_killpriv_v2 enabled, it should be possible to remove
this race completely. Do not kill suid/sgid with ATTR_MODE at all. It
will be killed by server when WRITE request is sent to server soon.
This is similar to fc->handle_killpriv logic. V2 is just more refined
version of protocol. Hence this patch does not send ATTR_MODE to
kill suid/sgid if fc->handle_killpriv_v2 is enabled.

This creates an issue if fc->writeback_cache is enabled. In that
case WRITE can be cached in guest and server might not see WRITE
request and hence will not kill suid/sgid. Miklos suggested that
in such cases, we should fallback to a writethrough WRITE instead
and that will generate WRITE request and kill suid/sgid. This patch
implements that too.

But this relies on client seeing the suid/sgid set. If another client
sets suid/sgid and this client does not see it immideately, then we
will not fallback to writethrough WRITE. So this is one limitation
with both fc->handle_killpriv_v2 and fc->writeback_cache enabled.
Both the options are not fully compatible. But might be good enough
for many use cases.

Note: I am not checking whether security.capability is set or not when
      falling back to writethrough path. if suid/sgid is not set and only
      security.capability is set, that will be taken care of by
      file_remove_privs() call in ->writeback_cache path.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/dir.c  | 2 +-
 fs/fuse/file.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index ecdb7895c156..510178594a8d 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1664,7 +1664,7 @@ static int fuse_setattr(struct dentry *entry, struct iattr *attr)
 		 *
 		 * This should be done on write(), truncate() and chown().
 		 */
-		if (!fc->handle_killpriv) {
+		if (!fc->handle_killpriv && !fc->handle_killpriv_v2) {
 			/*
 			 * ia_mode calculation may have used stale i_mode.
 			 * Refresh and recalculate.
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index e40428f3d0f1..ee1bb9bfdcd5 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1260,17 +1260,24 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	ssize_t written_buffered = 0;
 	struct inode *inode = mapping->host;
 	ssize_t err;
+	struct fuse_conn *fc = get_fuse_conn(inode);
 	loff_t endbyte = 0;
 
-	if (get_fuse_conn(inode)->writeback_cache) {
+	if (fc->writeback_cache) {
 		/* Update size (EOF optimization) and mode (SUID clearing) */
 		err = fuse_update_attributes(mapping->host, file);
 		if (err)
 			return err;
 
+		if (fc->handle_killpriv_v2 &&
+		    should_remove_suid(file_dentry(file))) {
+			goto writethrough;
+		}
+
 		return generic_file_write_iter(iocb, from);
 	}
 
+writethrough:
 	inode_lock(inode);
 
 	/* We can write back this queue in page reclaim */
-- 
2.25.4

[PATCH v3 5/6] fuse: Add a flag FUSE_OPEN_KILL_PRIV for open() request

[Vivek Goyal]

With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill
suid/sgid/security.capability on open(O_TRUNC), if server supports
FUSE_ATOMIC_O_TRUNC.

But server needs to kill suid/sgid only if caller does not have
CAP_FSETID. Given server does not have this information, client
needs to send this info to server.

So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells
server to kill suid/sgid(only if group execute is set).

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/file.c            |  5 +++++
 include/uapi/linux/fuse.h | 10 +++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index ee1bb9bfdcd5..5400c6d77701 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -42,6 +42,11 @@ static int fuse_send_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
 	inarg.flags = file->f_flags & ~(O_CREAT | O_EXCL | O_NOCTTY);
 	if (!fc->atomic_o_trunc)
 		inarg.flags &= ~O_TRUNC;
+
+	if (fc->handle_killpriv_v2 && (inarg.flags & O_TRUNC) &&
+	    !capable(CAP_FSETID))
+		inarg.open_flags |= FUSE_OPEN_KILL_PRIV;
+
 	args.opcode = opcode;
 	args.nodeid = nodeid;
 	args.in_numargs = 1;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 7b8da0a2de0d..e20b3ee9d292 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -173,6 +173,7 @@
  *  - add FUSE_SETUPMAPPING and FUSE_REMOVEMAPPING
  *  - add map_alignment to fuse_init_out, add FUSE_MAP_ALIGNMENT flag
  *  - add FUSE_HANDLE_KILLPRIV_V2
+ *  - add FUSE_OPEN_KILL_PRIV
  */
 
 #ifndef _LINUX_FUSE_H
@@ -427,6 +428,13 @@ struct fuse_file_lock {
  */
 #define FUSE_FSYNC_FDATASYNC	(1 << 0)
 
+/**
+ * Open flags
+ * FUSE_OPEN_KILL_PRIV: Kill suid/sgid/security.capability. sgid is cleared
+ * 			only if file has group execute permission.
+ */
+#define FUSE_OPEN_KILL_PRIV	(1 << 0)
+
 enum fuse_opcode {
 	FUSE_LOOKUP		= 1,
 	FUSE_FORGET		= 2,  /* no reply */
@@ -588,7 +596,7 @@ struct fuse_setattr_in {
 
 struct fuse_open_in {
 	uint32_t	flags;
-	uint32_t	unused;
+	uint32_t	open_flags;
 };
 
 struct fuse_create_in {
-- 
2.25.4

[PATCH v3 6/6] fuse: Support SB_NOSEC flag to improve direct write performance

[Vivek Goyal]

virtiofs can be slow with small writes if xattr are enabled and we are
doing cached writes (No direct I/). Ganesh Mahalingam noticed this here.

https://github.com/kata-containers/runtime/issues/2815

Some debugging showed that that file_remove_privs() is called in cached
write path on every write. And everytime it calls
security_inode_need_killpriv() which results in call to
__vfs_getxattr(XATTR_NAME_CAPS). And this goes to file server to fetch
xattr. This extra round trip for every write slows down writes tremendously.

Normally to avoid paying this penalty on every write, vfs has the
notion of caching this information in inode (S_NOSEC). So vfs
sets S_NOSEC, if filesystem opted for it using super block flag
SB_NOSEC. And S_NOSEC is cleared when setuid/setgid bit is set or
when security xattr is set on inode so that next time a write
happens, we check inode again for clearing setuid/setgid bits as well
clear any security.capability xattr.

This seems to work well for local file systems but for remote file
systems it is possible that VFS does not have full picture and a
different client sets setuid/setgid bit or security.capability xattr
on file and that means VFS information about S_NOSEC on another client
will be stale. So for remote filesystems SB_NOSEC was disabled by
default.

commit 9e1f1de02c2275d7172e18dc4e7c2065777611bf
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Fri Jun 3 18:24:58 2011 -0400

    more conservative S_NOSEC handling

That commit mentioned that these filesystems can still make use of
SB_NOSEC as long as they clear S_NOSEC when they are refreshing inode
attriutes from server.

So this patch tries to enable SB_NOSEC on fuse (regular fuse as well
as virtiofs). And clear SB_NOSEC when we are refreshing inode attributes.

This is enabled only if server supports FUSE_HANDLE_KILLPRIV_V2. This
says that server will clear setuid/setgid/security.capability on
chown/truncate/write as apporpriate.

This should provide tighter coherency because now suid/sgid/security.capability
will be cleared even if fuse client cache has not seen these attrs.

Basic idea is that fuse client will trigger suid/sgid/security.capability
clearing based on its attr cache. But even if cache has gone stale,
it is fine because FUSE_HANDLE_KILLPRIV_V2 will make sure WRITE
clear suid/sgid/security.capability.

We make this change only if server supports FUSE_HANDLE_KILLPRIV_V2.
This should make sure that existing filesystems which might be
relying on seucurity.capability always being queried from server
are not impacted.

This tighter coherency relies on WRITE showing up on server (and not
being cached in guest). So writeback_cache mode will not provide that
tight coherency and it is not recommended to use two together. Having
said that it might work reasonably well for lot of use cases.

This change improves random write performance very significantly. I
am running virtiofsd with cache=auto and following fio command.

fio --ioengine=libaio --direct=1  --name=test --filename=/mnt/virtiofs/random_read_write.fio --bs=4k --iodepth=64 --size=4G --readwrite=randwrite

Before this patch I get around 50MB/s and after the patch I get around
250MB/s bandwidth. So improvement is very significant.

Reported-by: "Mahalingam, Ganesh" <ganesh.mahalingam@intel.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/inode.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 20740b61f12b..4b7a043f21ee 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -201,6 +201,16 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
 		inode->i_mode &= ~S_ISVTX;
 
 	fi->orig_ino = attr->ino;
+
+	/*
+	 * We are refreshing inode data and it is possible that another
+	 * client set suid/sgid or security.capability xattr. So clear
+	 * S_NOSEC. Ideally, we could have cleared it only if suid/sgid
+	 * was set or if security.capability xattr was set. But we don't
+	 * know if security.capability has been set or not. So clear it
+	 * anyway. Its less efficient but should is safe.
+	 */
+	inode->i_flags &= ~S_NOSEC;
 }
 
 void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
@@ -993,8 +1003,10 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_args *args,
 			    !fuse_dax_check_alignment(fc, arg->map_alignment)) {
 				ok = false;
 			}
-			if (arg->flags & FUSE_HANDLE_KILLPRIV_V2)
+			if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
 				fc->handle_killpriv_v2 = 1;
+				fc->sb->s_flags |= SB_NOSEC;
+			}
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
-- 
2.25.4

Re: [PATCH v3 5/6] fuse: Add a flag FUSE_OPEN_KILL_PRIV for open() request

[Miklos Szeredi]

On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill
> suid/sgid/security.capability on open(O_TRUNC), if server supports
> FUSE_ATOMIC_O_TRUNC.
>
> But server needs to kill suid/sgid only if caller does not have
> CAP_FSETID. Given server does not have this information, client
> needs to send this info to server.
>
> So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells
> server to kill suid/sgid(only if group execute is set).

This is needed for FUSE_CREATE as well (which may act as a normal open
in case the file exists, and no O_EXCL was specified), right?

I can edit the patch, if you agree.

Thanks,
Miklos

Re: [PATCH v3 3/6] fuse: setattr should set FATTR_KILL_PRIV upon size change

[Miklos Szeredi]

On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> If fc->handle_killpriv_v2 is enabled, we expect file server to clear
> suid/sgid/security.capbility upon chown/truncate/write as appropriate.
>
> Upon truncate (ATTR_SIZE), suid/sgid is cleared only if caller does
> not have CAP_FSETID. File server does not know whether caller has
> CAP_FSETID or not. Hence set FATTR_KILL_PRIV upon truncate to let
> file server know that caller does not have CAP_FSETID and it should
> kill suid/sgid as appropriate.
>
> We don't have to send this information for chown (ATTR_UID/ATTR_GID)
> as that always clears suid/sgid irrespective of capabilities of
> calling process.

I'm  undecided on this.   Would it hurt to set it on chown?  That
might make the logic in some servers simpler, no?

What would be the drawback of setting FATTR_KILL_PRIV for chown as well?

Thanks,
Miklos

Re: [PATCH v3 5/6] fuse: Add a flag FUSE_OPEN_KILL_PRIV for open() request

[Vivek Goyal]

On Fri, Nov 06, 2020 at 02:55:11PM +0100, Miklos Szeredi wrote:
> On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill
> > suid/sgid/security.capability on open(O_TRUNC), if server supports
> > FUSE_ATOMIC_O_TRUNC.
> >
> > But server needs to kill suid/sgid only if caller does not have
> > CAP_FSETID. Given server does not have this information, client
> > needs to send this info to server.
> >
> > So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells
> > server to kill suid/sgid(only if group execute is set).
> 
> This is needed for FUSE_CREATE as well (which may act as a normal open
> in case the file exists, and no O_EXCL was specified), right?

Hi Miklos,

IIUC, In current code we seem to use FUSE_CREATE only if file does not exist.
If file exists, then we probably will take FUSE_OPEN path.

Are you concerned about future proofing where somebody decides to use
FUSE_CREATE for create + open on a file which exists. If yes, I agree that
patching FUSE_CREATE makes sense.

> 
> I can edit the patch, if you agree.

Please do.

Thanks
Vivek

Re: [PATCH v3 5/6] fuse: Add a flag FUSE_OPEN_KILL_PRIV for open() request

[Miklos Szeredi]

On Fri, Nov 6, 2020 at 5:00 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Nov 06, 2020 at 02:55:11PM +0100, Miklos Szeredi wrote:
> > On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill
> > > suid/sgid/security.capability on open(O_TRUNC), if server supports
> > > FUSE_ATOMIC_O_TRUNC.
> > >
> > > But server needs to kill suid/sgid only if caller does not have
> > > CAP_FSETID. Given server does not have this information, client
> > > needs to send this info to server.
> > >
> > > So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells
> > > server to kill suid/sgid(only if group execute is set).
> >
> > This is needed for FUSE_CREATE as well (which may act as a normal open
> > in case the file exists, and no O_EXCL was specified), right?
>
> Hi Miklos,
>
> IIUC, In current code we seem to use FUSE_CREATE only if file does not exist.
> If file exists, then we probably will take FUSE_OPEN path.

That's true if the cache is up to date, one important point for
FUSE_CREATE is that it works atomically even if the cache is stale.
So if cache is negative and we send a FUSE_CREATE it may still open an
*existing* file, and we want to do suid/caps clearing in that case
also, no?

Thanks,
Miklos

Re: [PATCH v3 3/6] fuse: setattr should set FATTR_KILL_PRIV upon size change

[Vivek Goyal]

On Fri, Nov 06, 2020 at 03:39:29PM +0100, Miklos Szeredi wrote:
> On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > If fc->handle_killpriv_v2 is enabled, we expect file server to clear
> > suid/sgid/security.capbility upon chown/truncate/write as appropriate.
> >
> > Upon truncate (ATTR_SIZE), suid/sgid is cleared only if caller does
> > not have CAP_FSETID. File server does not know whether caller has
> > CAP_FSETID or not. Hence set FATTR_KILL_PRIV upon truncate to let
> > file server know that caller does not have CAP_FSETID and it should
> > kill suid/sgid as appropriate.
> >
> > We don't have to send this information for chown (ATTR_UID/ATTR_GID)
> > as that always clears suid/sgid irrespective of capabilities of
> > calling process.
> 
> I'm  undecided on this.   Would it hurt to set it on chown?  That
> might make the logic in some servers simpler, no?
> 
> What would be the drawback of setting FATTR_KILL_PRIV for chown as well?

Hi Miklos,

Thinking loud.

So these are the rules we expect from VFS point of view.

- caps are always cleared on chown/write/truncate
- suid is always cleared on chown, while for truncate/write it is cleared
  only if caller does not have CAP_FSETID.
- sgid is always cleared on chown, while for truncate/write it is cleared
  only if caller does not have CAP_FSETID as well as file has group execute
  permission.

From server point of view, these rules become.

- caps are always cleared on chown/write/truncate
- suid is always cleared on chown, while for truncate/write it is cleared
  only if client set appropriate flag.
  	- For truncate, this flag will either be FUSE_OPEN_KILL_PRIV or
	  FATTR_KILL_PRIV.
	- For write, FUSE_WRITE_KILL_PRIV will be set.
- sgid is always cleared on chown, while for truncate/write it is cleared
  only if caller has set a flag as well as file has group execute permission.
  	- For truncate, this flag will either be FUSE_OPEN_KILL_PRIV or
	  FATTR_KILL_PRIV.
	- For write, FUSE_WRITE_KILL_PRIV will be set.

Above rules assumes that chown() will always clear caps/suid/sgid and
server does not have to rely on any flags.

I think it does not hurt to start passing FATTR_KILL_PRIV for chown()
as well. In that case, server will always clear caps on chown but
clear suid/sgid only if FATTR_KILL_PRIV is set. (Which will always
be set).

So anything is fine. We just need to document it well. I think I will
write it very clearly in qemu patch depending on what goes in kernel.

Thanks
Vivek

Re: [PATCH v3 5/6] fuse: Add a flag FUSE_OPEN_KILL_PRIV for open() request

[Vivek Goyal]

On Fri, Nov 06, 2020 at 05:33:00PM +0100, Miklos Szeredi wrote:
> On Fri, Nov 6, 2020 at 5:00 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Fri, Nov 06, 2020 at 02:55:11PM +0100, Miklos Szeredi wrote:
> > > On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill
> > > > suid/sgid/security.capability on open(O_TRUNC), if server supports
> > > > FUSE_ATOMIC_O_TRUNC.
> > > >
> > > > But server needs to kill suid/sgid only if caller does not have
> > > > CAP_FSETID. Given server does not have this information, client
> > > > needs to send this info to server.
> > > >
> > > > So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells
> > > > server to kill suid/sgid(only if group execute is set).
> > >
> > > This is needed for FUSE_CREATE as well (which may act as a normal open
> > > in case the file exists, and no O_EXCL was specified), right?
> >
> > Hi Miklos,
> >
> > IIUC, In current code we seem to use FUSE_CREATE only if file does not exist.
> > If file exists, then we probably will take FUSE_OPEN path.
> 
> That's true if the cache is up to date, one important point for
> FUSE_CREATE is that it works atomically even if the cache is stale.
> So if cache is negative and we send a FUSE_CREATE it may still open an
> *existing* file, and we want to do suid/caps clearing in that case
> also, no?

Yes, makes sense. This can happen in a race condition also where
fuse_lookup_name() gets a negative dentry and then another client
creates file (with setuid/setgid/caps) set. Now fuse_create_open()
is called without O_EXCL and in that case we should remove
setuid/setgid/caps as needed. 

So yes, please make modifications accordingly for FUSE_CREATE. If you
want me to do make those changes, please let me know.

Thanks
Vivek

Re: [PATCH v3 3/6] fuse: setattr should set FATTR_KILL_PRIV upon size change

[Miklos Szeredi]

On Fri, Nov 6, 2020 at 6:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:

> I think it does not hurt to start passing FATTR_KILL_PRIV for chown()
> as well. In that case, server will always clear caps on chown but
> clear suid/sgid only if FATTR_KILL_PRIV is set. (Which will always
> be set).

Okay.

More thoughts for FUSE_HANDLE_KILLPRIV_V2:

 - clear "security.capability" on write, truncate and chown unconditionally
 - clear suid/sgid if
    o setattr has FATTR_SIZE and  FATTR_KILL_PRIV
    o setattr has FATTR_UID or FATTR_GID
    o open has O_TRUNC and FUSE_OPEN_KILL_PRIV
    o write has FUSE_WRITE_KILL_PRIV

Kernel has:
ATTR_KILL_PRIV -> clear "security.capability"
ATTR_KILL_SUID -> clear S_ISUID
ATTR_KILL_SGID -> clear S_ISGID if executable

Fuse has:
FUSE_*KILL_PRIV -> clear S_ISUID and S_ISGID if executable

So the fuse meaning of FUSE_*KILL_PRIV has a complementary meaning to
that of ATTR_KILL_PRIV, which is somewhat confusing.  Also "PRIV"
implies all privileges, including "security.capability" but the fuse
ones relate to suid/sgid only.

How about FUSE_*KILL_SUIDGID (FUSE_WRITE_KILL_SUIDGID being an alias
for FUSE_WRITE_KILL_PRIV)?

Thanks,
Miklos




>
> So anything is fine. We just need to document it well. I think I will
> write it very clearly in qemu patch depending on what goes in kernel.
>
> Thanks
> Vivek
>

Re: [PATCH v3 3/6] fuse: setattr should set FATTR_KILL_PRIV upon size change

[Miklos Szeredi]

On Wed, Nov 11, 2020 at 2:54 PM Miklos Szeredi <miklos@szeredi.hu> wrote:

> How about FUSE_*KILL_SUIDGID (FUSE_WRITE_KILL_SUIDGID being an alias
> for FUSE_WRITE_KILL_PRIV)?

Series pushed to #for-next with these changes.  Please take a look and test.

Thanks,
Miklos

Re: [PATCH v3 3/6] fuse: setattr should set FATTR_KILL_PRIV upon size change

[Vivek Goyal]

On Wed, Nov 11, 2020 at 02:54:43PM +0100, Miklos Szeredi wrote:
> On Fri, Nov 6, 2020 at 6:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > I think it does not hurt to start passing FATTR_KILL_PRIV for chown()
> > as well. In that case, server will always clear caps on chown but
> > clear suid/sgid only if FATTR_KILL_PRIV is set. (Which will always
> > be set).
> 
> Okay.
> 
> More thoughts for FUSE_HANDLE_KILLPRIV_V2:
> 
>  - clear "security.capability" on write, truncate and chown unconditionally
>  - clear suid/sgid if
>     o setattr has FATTR_SIZE and  FATTR_KILL_PRIV
>     o setattr has FATTR_UID or FATTR_GID
>     o open has O_TRUNC and FUSE_OPEN_KILL_PRIV
>     o write has FUSE_WRITE_KILL_PRIV
> 
> Kernel has:
> ATTR_KILL_PRIV -> clear "security.capability"
> ATTR_KILL_SUID -> clear S_ISUID
> ATTR_KILL_SGID -> clear S_ISGID if executable
> 
> Fuse has:
> FUSE_*KILL_PRIV -> clear S_ISUID and S_ISGID if executable
> 
> So the fuse meaning of FUSE_*KILL_PRIV has a complementary meaning to
> that of ATTR_KILL_PRIV, which is somewhat confusing.  Also "PRIV"
> implies all privileges, including "security.capability" but the fuse
> ones relate to suid/sgid only.
> 
> How about FUSE_*KILL_SUIDGID (FUSE_WRITE_KILL_SUIDGID being an alias
> for FUSE_WRITE_KILL_PRIV)?

Hi Miklos,

Renaming FUSE_*KILL_PRIV to FUSE_*KILL_SUIDSGID sounds good. For a
breif moment I was also thinking that these FUSE_*KILL_PRIV and
and ATTR_KILL_PRIV are not exactly mapping. Glad you caught it
and made the situation better.

Thanks
Vivek

Re: [PATCH v3 3/6] fuse: setattr should set FATTR_KILL_PRIV upon size change

[Vivek Goyal]

On Wed, Nov 11, 2020 at 05:24:22PM +0100, Miklos Szeredi wrote:
> On Wed, Nov 11, 2020 at 2:54 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> 
> > How about FUSE_*KILL_SUIDGID (FUSE_WRITE_KILL_SUIDGID being an alias
> > for FUSE_WRITE_KILL_PRIV)?
> 
> Series pushed to #for-next with these changes.  Please take a look and test.

Thanks Miklos. I looked at the patches quickly and they look good. I am
now fixing qemu virtiofsd side and will test.

Thanks
Vivek