* Vivek Goyal (vgo...@redhat.com) wrote: > When posix access acls are set on a file, it can lead to adjusting file > permissions (mode) as well. If caller does not have CAP_FSETID and it > also does not have membership of owner group, this will lead to clearing > SGID bit in mode. > > Current fuse code is written in such a way that it expects file server > to take care of chaning file mode (permission), if there is a need. > Right now, host kernel does not clear SGID bit because virtiofsd is > running as root and has CAP_FSETID. For host kernel to clear SGID, > virtiofsd need to switch to gid of caller in guest and also drop > CAP_FSETID (if caller did not have it to begin with). > > If SGID needs to be cleared, client will set the flag > FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server > should kill sgid. > > Currently just switch to uid/gid of the caller and drop CAP_FSETID > and that should do it. > > This should fix the xfstest generic/375 test case. > > We don't have to switch uid for this to work. That could be one optimization > that pass a parameter to lo_change_cred() to only switch gid and not uid. > > Also this will not work whenever (if ever) we support idmapped mounts. In > that case it is possible that uid/gid in request are 0/0 but still we > need to clear SGID. So we will have to pick a non-root sgid and switch > to that instead. That's an TODO item for future when idmapped mount > support is introduced. > > This patch only adds the capability to switch creds and drop FSETID > when acl xattr is set. This does not take affect yet. It can take > affect when next patch adds the capability to enable posix_acl. > > Reported-by: Luis Henriques <lhenriq...@suse.de> > Signed-off-by: Vivek Goyal <vgo...@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 75 ++++++++++++++++++++++++++++++++ > 1 file changed, 75 insertions(+) > > diff --git a/tools/virtiofsd/passthrough_ll.c > b/tools/virtiofsd/passthrough_ll.c > index 0c9084ea15..113c725def 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -175,6 +175,7 @@ struct lo_data { > int user_killpriv_v2, killpriv_v2; > /* If set, virtiofsd is responsible for setting umask during creation */ > bool change_umask; > + int posix_acl; > }; > > static const struct fuse_opt lo_opts[] = { > @@ -1185,6 +1186,51 @@ static void lo_restore_cred(struct lo_cred *old, bool > restore_umask) > umask(old->umask); > } > > +/* > + * A helper to change cred and drop capability. Returns 0 on success and > + * errno on error > + */ > +static int lo_drop_cap_change_cred(fuse_req_t req, struct lo_cred *old, > + bool change_umask, const char *cap_name, > + bool *cap_dropped) > +{ > + int ret; > + bool __cap_dropped; > + > + assert(cap_name); > + > + ret = drop_effective_cap(cap_name, &__cap_dropped); > + if (ret) { > + return ret; > + } > + > + ret = lo_change_cred(req, old, change_umask); > + if (ret) { > + if (__cap_dropped) { > + if (gain_effective_cap(cap_name)) { > + fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name); > + } > + } > + } > + > + if (cap_dropped) { > + *cap_dropped = __cap_dropped; > + } > + return ret; > +} > + > +static void lo_restore_cred_gain_cap(struct lo_cred *old, bool restore_umask, > + const char *cap_name) > +{ > + assert(cap_name); > + > + lo_restore_cred(old, restore_umask); > + > + if (gain_effective_cap(cap_name)) { > + fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name); > + } > +} > + > static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, > const char *name, mode_t mode, dev_t rdev, > const char *link) > @@ -2976,6 +3022,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, > const char *in_name, > ssize_t ret; > int saverr; > int fd = -1; > + bool switched_creds = false; > + bool cap_fsetid_dropped = false; > + struct lo_cred old = {}; > > mapped_name = NULL; > name = in_name; > @@ -3006,6 +3055,26 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t > ino, const char *in_name, > ", name=%s value=%s size=%zd)\n", ino, name, value, size); > > sprintf(procname, "%i", inode->fd); > + /* > + * If we are setting posix access acl and if SGID needs to be > + * cleared, then switch to caller's gid and drop CAP_FSETID > + * and that should make sure host kernel clears SGID. > + * > + * This probably will not work when we support idmapped mounts. > + * In that case we will need to find a non-root gid and switch > + * to it. (Instead of gid in request). Fix it when we support > + * idmapped mounts. > + */ > + if (lo->posix_acl && !strcmp(name, "system.posix_acl_access") > + && (extra_flags & FUSE_SETXATTR_ACL_KILL_SGID)) { > + ret = lo_drop_cap_change_cred(req, &old, false, "FSETID", > + &cap_fsetid_dropped); > + if (ret) { > + saverr = ret; > + goto out; > + } > + switched_creds = true; > + } > if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) { > fd = openat(lo->proc_self_fd, procname, O_RDONLY); > if (fd < 0) { > @@ -3021,6 +3090,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t > ino, const char *in_name, > saverr = ret == -1 ? errno : 0; > FCHDIR_NOFAIL(lo->root.fd); > } > + if (switched_creds) { > + if (cap_fsetid_dropped) > + lo_restore_cred_gain_cap(&old, false, "FSETID"); > + else > + lo_restore_cred(&old, false); > + } > > out: > if (fd >= 0) { > -- > 2.25.4 > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK