| Message ID | 20210316160147.289193-2-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: acl: Send file mode updates using SETATTR | expand |
On Tue, Mar 16, 2021 at 5:02 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > If ACL changes, it is possible that file mode permission bits change. As of > now fuse client relies on file server to make those changes. But it does > not send enough information to server so that it can decide where SGID > bit should be cleared or not. Server does not know if caller has CAP_FSETID > or not. It also does not know what are caller's group memberships and if any > of the groups match file owner group. Right. So what about performing the capability and group membership check in the client and sending the result of this check to the server? Yes, need to extend fuse_setxattr_in. There's still a race with uid and gid changing on the underlying filesystem, so the attributes need to be refreshed, but I don't think that's a big worry. Thanks, Miklos
On Wed, Mar 17, 2021 at 04:43:35PM +0100, Miklos Szeredi wrote: > On Tue, Mar 16, 2021 at 5:02 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > If ACL changes, it is possible that file mode permission bits change. As of > > now fuse client relies on file server to make those changes. But it does > > not send enough information to server so that it can decide where SGID > > bit should be cleared or not. Server does not know if caller has CAP_FSETID > > or not. It also does not know what are caller's group memberships and if any > > of the groups match file owner group. > > Right. So what about performing the capability and group membership > check in the client and sending the result of this check to the > server? Hi Miklos, But that will still be non-atomic, right? I mean server probably will do setxattr first, then check if SGID was cleared or not, and if it has not been cleared, then it needs to set the mode. IOW, we still have two operations (setxattr followed by mode setting). I had thought about that option. But could not understand what does it buy us as opposed to guest sending a SETATTR. > > Yes, need to extend fuse_setxattr_in. Ok. > > There's still a race with uid and gid changing on the underlying > filesystem, so the attributes need to be refreshed, but I don't think > that's a big worry. Yes, attributes will need to be refreshed. Thanks Vivek
On Wed, Mar 17, 2021 at 6:01 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > On Wed, Mar 17, 2021 at 04:43:35PM +0100, Miklos Szeredi wrote: > > On Tue, Mar 16, 2021 at 5:02 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > If ACL changes, it is possible that file mode permission bits change. As of > > > now fuse client relies on file server to make those changes. But it does > > > not send enough information to server so that it can decide where SGID > > > bit should be cleared or not. Server does not know if caller has CAP_FSETID > > > or not. It also does not know what are caller's group memberships and if any > > > of the groups match file owner group. > > > > Right. So what about performing the capability and group membership > > check in the client and sending the result of this check to the > > server? > > Hi Miklos, > > But that will still be non-atomic, right? I mean server probably will > do setxattr first, then check if SGID was cleared or not, and if it > has not been cleared, then it needs to set the mode. > > IOW, we still have two operations (setxattr followed by mode setting). > > I had thought about that option. But could not understand what does > it buy us as opposed to guest sending a SETATTR. If the non-atomic SETXATTR + SETATTR is done in the client, then the server has no chance of ever operating correctly. If the responsibility for clearing sgid is in the server, then it's up to the server to decide how to best deal with this. That may be the racy way, but at least it's not the only possibility. Not sure if virtiofsd can do this atomically or not. setgid()/setgroups() require CAP_SETGID, but that's relative to the user namespace of the daemon, so this might be possible to do, I haven't put a lot of thought into this. Thanks, Miklos
On Wed, Mar 17, 2021 at 08:25:51PM +0100, Miklos Szeredi wrote: > On Wed, Mar 17, 2021 at 6:01 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > On Wed, Mar 17, 2021 at 04:43:35PM +0100, Miklos Szeredi wrote: > > > On Tue, Mar 16, 2021 at 5:02 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > If ACL changes, it is possible that file mode permission bits change. As of > > > > now fuse client relies on file server to make those changes. But it does > > > > not send enough information to server so that it can decide where SGID > > > > bit should be cleared or not. Server does not know if caller has CAP_FSETID > > > > or not. It also does not know what are caller's group memberships and if any > > > > of the groups match file owner group. > > > > > > Right. So what about performing the capability and group membership > > > check in the client and sending the result of this check to the > > > server? > > > > Hi Miklos, > > > > But that will still be non-atomic, right? I mean server probably will > > do setxattr first, then check if SGID was cleared or not, and if it > > has not been cleared, then it needs to set the mode. > > > > IOW, we still have two operations (setxattr followed by mode setting). > > > > I had thought about that option. But could not understand what does > > it buy us as opposed to guest sending a SETATTR. > > If the non-atomic SETXATTR + SETATTR is done in the client, then the > server has no chance of ever operating correctly. > > If the responsibility for clearing sgid is in the server, then it's up > to the server to decide how to best deal with this. That may be the > racy way, but at least it's not the only possibility. > > Not sure if virtiofsd can do this atomically or not. > setgid()/setgroups() require CAP_SETGID, but that's relative to the > user namespace of the daemon, so this might be possible to do, I > haven't put a lot of thought into this. I guess you are right. virtiofsd can do it atomically. If client says to clear SGID, then virtiofsd can do following. A. Query who is file owner group. B. setgid(non-file-owner-group) C. drop CAP_FSETID D. setxattr(system.posix_acl_access). If file owner group is not root, then we don't have to do any setgid at all. If file owner group is root, then we have to find a gid which is valid in mount namespace of virtiofsd and switch to it. I guess at virtiofsd start time we can look at /proc/$$/gid_map and store on non-root gid which is valid. Only race here will be if another client changes file owner group between A and D and sets to same non-file-onwer-group we changed to. In that case it will not be cleared. Hmm.., may be post setxattr we can do another stat() and make sure SGID got cleared. If not, do a chmod. IOW, if atomic approach races, fall back to non-atomic approach. Litle twisted but should probably work. Will try. Thanks Vivek
diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c index e9c0f916349d..38920dcfb710 100644 --- a/fs/fuse/acl.c +++ b/fs/fuse/acl.c @@ -50,10 +50,31 @@ struct posix_acl *fuse_get_acl(struct inode *inode, int type) return acl; } +static int fuse_acl_mode_setattr(struct inode *inode, umode_t mode) +{ + struct fuse_mount *fm = get_fuse_mount(inode); + FUSE_ARGS(args); + struct fuse_setattr_in inarg; + struct fuse_attr_out outarg; + + memset(&inarg, 0, sizeof(inarg)); + memset(&outarg, 0, sizeof(outarg)); + + inarg.valid = FATTR_MODE; + inarg.mode = mode; + fuse_setattr_fill(fm->fc, &args, inode, &inarg, &outarg); + + return fuse_simple_request(fm, &args); +} + int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode, struct posix_acl *acl, int type) { struct fuse_conn *fc = get_fuse_conn(inode); + umode_t new_mode; + bool update_mode = false; + size_t size = 0; + void *value = NULL; const char *name; int ret; @@ -63,9 +84,24 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode, if (!fc->posix_acl || fc->no_setxattr) return -EOPNOTSUPP; - if (type == ACL_TYPE_ACCESS) + if (type == ACL_TYPE_ACCESS) { name = XATTR_NAME_POSIX_ACL_ACCESS; - else if (type == ACL_TYPE_DEFAULT) + if (acl && fc->posix_acl_update_mode) { + /* + * Setting access ACL might clear SGID. + * Refresh inode->i_mode before making a decision. + */ + ret = fuse_do_getattr(inode, NULL, NULL); + if (ret) + return ret; + ret = posix_acl_update_mode(&init_user_ns, inode, + &new_mode, &acl); + if (ret) + return ret; + if (new_mode != inode->i_mode) + update_mode = true; + } + } else if (type == ACL_TYPE_DEFAULT) name = XATTR_NAME_POSIX_ACL_DEFAULT; else return -EINVAL; @@ -78,8 +114,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode, * them to be refreshed the next time they are used, * and it also updates i_ctime. */ - size_t size = posix_acl_xattr_size(acl->a_count); - void *value; + size = posix_acl_xattr_size(acl->a_count); if (size > PAGE_SIZE) return -E2BIG; @@ -93,8 +128,19 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode, kfree(value); return ret; } + } + + if (update_mode) { + ret = fuse_acl_mode_setattr(inode, new_mode); + if (ret < 0) { + kfree(value); + return ret; + } + } + if (acl) { ret = fuse_setxattr(inode, name, value, size, 0); + /* TODO: If setxattr failed, should we restore mode ? */ kfree(value); } else { ret = fuse_removexattr(inode, name); diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 06a18700a845..1d5c5aafb82d 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -1022,8 +1022,7 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr, stat->blksize = 1 << blkbits; } -static int fuse_do_getattr(struct inode *inode, struct kstat *stat, - struct file *file) +int fuse_do_getattr(struct inode *inode, struct kstat *stat, struct file *file) { int err; struct fuse_getattr_in inarg; @@ -1541,10 +1540,10 @@ void fuse_release_nowrite(struct inode *inode) spin_unlock(&fi->lock); } -static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_args *args, - struct inode *inode, - struct fuse_setattr_in *inarg_p, - struct fuse_attr_out *outarg_p) +void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_args *args, + struct inode *inode, + struct fuse_setattr_in *inarg_p, + struct fuse_attr_out *outarg_p) { args->opcode = FUSE_SETATTR; args->nodeid = get_node_id(inode); diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 68cca8d4db6e..ba836daecd08 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -731,6 +731,9 @@ struct fuse_conn { /** Does the filesystem support posix acls? */ unsigned posix_acl:1; + /** If posix acl results in file mode change, send update to fs */ + unsigned posix_acl_update_mode:1; + /** Check permissions based on the file mode or not? */ unsigned default_permissions:1; @@ -1097,6 +1100,7 @@ u64 fuse_lock_owner_id(struct fuse_conn *fc, fl_owner_t id); void fuse_update_ctime(struct inode *inode); +int fuse_do_getattr(struct inode *inode, struct kstat *stat, struct file *file); int fuse_update_attributes(struct inode *inode, struct file *file); void fuse_flush_writepages(struct inode *inode); @@ -1162,7 +1166,10 @@ int fuse_write_inode(struct inode *inode, struct writeback_control *wbc); int fuse_do_setattr(struct dentry *dentry, struct iattr *attr, struct file *file); - +void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_args *args, + struct inode *inode, + struct fuse_setattr_in *inarg_p, + struct fuse_attr_out *outarg_p); void fuse_set_initialized(struct fuse_conn *fc); void fuse_unlock_inode(struct inode *inode, bool locked); diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index b0e18b470e91..678145f987d1 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -1052,6 +1052,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args, fc->handle_killpriv_v2 = 1; fm->sb->s_flags |= SB_NOSEC; } + if (arg->flags & FUSE_POSIX_ACL_UPDATE_MODE) + fc->posix_acl_update_mode = 1; } else { ra_pages = fc->max_read / PAGE_SIZE; fc->no_lock = 1; @@ -1095,7 +1097,7 @@ void fuse_send_init(struct fuse_mount *fm) 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_HANDLE_KILLPRIV_V2; + FUSE_HANDLE_KILLPRIV_V2 | FUSE_POSIX_ACL_UPDATE_MODE; #ifdef CONFIG_FUSE_DAX if (fm->fc->dax) ia->in.flags |= FUSE_MAP_ALIGNMENT; diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index 98ca64d1beb6..89c3a88354f4 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -179,6 +179,7 @@ * 7.33 * - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID * - add FUSE_OPEN_KILL_SUIDGID + * - add FUSE_POSIX_ACL_UPDATE_MODE */ #ifndef _LINUX_FUSE_H @@ -330,6 +331,9 @@ struct fuse_file_lock { * does not have CAP_FSETID. Additionally upon * write/truncate sgid is killed only if file has group * execute permission. (Same as Linux VFS behavior). + * FUSE_POSIX_ACL_UPDATE_MODE: When posix acl setting results in a mode change, + * client should send explicit setattr message to + * update mode. */ #define FUSE_ASYNC_READ (1 << 0) #define FUSE_POSIX_LOCKS (1 << 1) @@ -360,6 +364,7 @@ struct fuse_file_lock { #define FUSE_MAP_ALIGNMENT (1 << 26) #define FUSE_SUBMOUNTS (1 << 27) #define FUSE_HANDLE_KILLPRIV_V2 (1 << 28) +#define FUSE_POSIX_ACL_UPDATE_MODE (1 << 29) /** * CUSE INIT request/reply flags
If ACL changes, it is possible that file mode permission bits change. As of now fuse client relies on file server to make those changes. But it does not send enough information to server so that it can decide where SGID bit should be cleared or not. Server does not know if caller has CAP_FSETID or not. It also does not know what are caller's group memberships and if any of the groups match file owner group. So add a flag FUSE_POSIX_ACL_UPDATE_MODE where server can specify that if mode changes due to ACL change, then client needs to send explicit SETATTR to update mode. Reported-by: Luis Henriques <lhenriques@suse.de> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/fuse/acl.c | 54 ++++++++++++++++++++++++++++++++++++--- fs/fuse/dir.c | 11 ++++---- fs/fuse/fuse_i.h | 9 ++++++- fs/fuse/inode.c | 4 ++- include/uapi/linux/fuse.h | 5 ++++ 5 files changed, 71 insertions(+), 12 deletions(-)