From: Vivek Goyal <vgoyal@redhat.com>
To: qemu-devel@nongnu.org, virtio-fs@redhat.com
Cc: lhenriques@suse.de, dgilbert@redhat.com, vgoyal@redhat.com,
miklos@szeredi.hu
Subject: [PATCH v5 5/5] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr
Date: Thu, 25 Mar 2021 11:38:52 -0400 [thread overview]
Message-ID: <20210325153852.572927-6-vgoyal@redhat.com> (raw)
In-Reply-To: <20210325153852.572927-1-vgoyal@redhat.com>
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.
Reported-by: Luis Henriques <lhenriques@suse.de>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
include/standard-headers/linux/fuse.h | 7 +++++
tools/virtiofsd/passthrough_ll.c | 42 +++++++++++++++++++++++++--
2 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/include/standard-headers/linux/fuse.h b/include/standard-headers/linux/fuse.h
index cc87ff27d0..4eb79399d4 100644
--- a/include/standard-headers/linux/fuse.h
+++ b/include/standard-headers/linux/fuse.h
@@ -180,6 +180,7 @@
* - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
* - add FUSE_OPEN_KILL_SUIDGID
* - add FUSE_SETXATTR_V2
+ * - add FUSE_SETXATTR_ACL_KILL_SGID
*/
#ifndef _LINUX_FUSE_H
@@ -450,6 +451,12 @@ struct fuse_file_lock {
*/
#define FUSE_OPEN_KILL_SUIDGID (1 << 0)
+/**
+ * setxattr flags
+ * FUSE_SETXATTR_ACL_KILL_SGID: Clear SGID when system.posix_acl_access is set
+ */
+#define FUSE_SETXATTR_ACL_KILL_SGID (1 << 0)
+
enum fuse_opcode {
FUSE_LOOKUP = 1,
FUSE_FORGET = 2, /* no reply */
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 3f5c267604..8a48071d0b 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -175,7 +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 user_posix_acl;
+ int user_posix_acl, posix_acl;
};
static const struct fuse_opt lo_opts[] = {
@@ -716,8 +716,10 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
* in fuse_lowlevel.c
*/
fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
- conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK;
+ conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK |
+ FUSE_CAP_SETXATTR_V2;
lo->change_umask = true;
+ lo->posix_acl = true;
} else {
/* User either did not specify anything or wants it disabled */
fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
@@ -3092,12 +3094,48 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
sprintf(procname, "%i", inode->fd);
if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
+ bool switched_creds = false;
+ struct lo_cred old = {};
+
fd = openat(lo->proc_self_fd, procname, O_RDONLY);
if (fd < 0) {
saverr = errno;
goto out;
}
+
+ /*
+ * 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_change_cred(req, &old, false);
+ if (ret) {
+ saverr = ret;
+ goto out;
+ }
+ ret = drop_effective_cap("FSETID", NULL);
+ if (ret != 0) {
+ lo_restore_cred(&old, false);
+ saverr = ret;
+ goto out;
+ }
+ switched_creds = true;
+ }
+
ret = fsetxattr(fd, name, value, size, flags);
+
+ if (switched_creds) {
+ if (gain_effective_cap("FSETID"))
+ fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_FSETID\n");
+ lo_restore_cred(&old, false);
+ }
} else {
/* fchdir should not fail here */
assert(fchdir(lo->proc_self_fd) == 0);
--
2.25.4
next prev parent reply other threads:[~2021-03-25 15:57 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-25 15:38 [PATCH v5 0/5] virtiofsd: Add support to enable/disable posix acls Vivek Goyal
2021-03-25 15:38 ` [PATCH v5 1/5] virtiofsd: Add umask to seccom allow list Vivek Goyal
2021-03-25 15:38 ` [PATCH v5 2/5] virtiofsd: Add capability to change/restore umask Vivek Goyal
2021-03-25 15:38 ` [PATCH v5 3/5] virtiofsd: Add an option to enable/disable posix acls Vivek Goyal
2021-03-25 15:38 ` [PATCH v5 4/5] virtiofsd: Add support for setxattr_v2 Vivek Goyal
2021-03-25 15:38 ` Vivek Goyal [this message]
2021-03-29 15:35 ` [PATCH v5 5/5] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr Luis Henriques
2021-03-29 19:51 ` Vivek Goyal
2021-03-30 9:37 ` Luis Henriques
2021-03-25 16:03 ` [PATCH v5 0/5] virtiofsd: Add support to enable/disable posix acls no-reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210325153852.572927-6-vgoyal@redhat.com \
--to=vgoyal@redhat.com \
--cc=dgilbert@redhat.com \
--cc=lhenriques@suse.de \
--cc=miklos@szeredi.hu \
--cc=qemu-devel@nongnu.org \
--cc=virtio-fs@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).