* [PATCH 0/2] virtiofsd: Add capability to block xattrs @ 2021-08-26 21:19 ` Vivek Goyal 0 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-08-26 21:19 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal As of now we have a knob "-o xattr/no_xattr" which either enables all xattrs or disables all xattrs. We need something more fine grained where we can selectively disable only certain xattrs (and not all). For example, in some cases we want to disable "security.selinux" xattr. This is equivalent to virtiofs not supporting security.selinux and guest kernel will fallback to a single label for whole fs (virtiofs_t). So add an option "-o block_xattr=<list-of-xattrs>" which will allow specifying a list of xattrs to block. Vivek Goyal (2): virtiofsd: Add an array to keep track of blocked xattrs virtiofsd: Add option "block_xattr=" to block certain xattrs docs/tools/virtiofsd.rst | 17 ++++ tools/virtiofsd/helper.c | 3 + tools/virtiofsd/passthrough_ll.c | 166 ++++++++++++++++++++++++++++--- 3 files changed, 171 insertions(+), 15 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Virtio-fs] [PATCH 0/2] virtiofsd: Add capability to block xattrs @ 2021-08-26 21:19 ` Vivek Goyal 0 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-08-26 21:19 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: vgoyal As of now we have a knob "-o xattr/no_xattr" which either enables all xattrs or disables all xattrs. We need something more fine grained where we can selectively disable only certain xattrs (and not all). For example, in some cases we want to disable "security.selinux" xattr. This is equivalent to virtiofs not supporting security.selinux and guest kernel will fallback to a single label for whole fs (virtiofs_t). So add an option "-o block_xattr=<list-of-xattrs>" which will allow specifying a list of xattrs to block. Vivek Goyal (2): virtiofsd: Add an array to keep track of blocked xattrs virtiofsd: Add option "block_xattr=" to block certain xattrs docs/tools/virtiofsd.rst | 17 ++++ tools/virtiofsd/helper.c | 3 + tools/virtiofsd/passthrough_ll.c | 166 ++++++++++++++++++++++++++++--- 3 files changed, 171 insertions(+), 15 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] virtiofsd: Add an array to keep track of blocked xattrs 2021-08-26 21:19 ` [Virtio-fs] " Vivek Goyal @ 2021-08-26 21:19 ` Vivek Goyal -1 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-08-26 21:19 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal Right now we have capability to block "system.posix_acl_access" and "system.posix_acl_default" xattrs. But we have sort of hardcoded these two values and its not generic. Now we want to support blocking of arbitrary xattr as passed in by user. So let us keep an array of blocked xattrs and consult that array when deciding whether an attribute is blocked or not. This should not result any functional change. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- tools/virtiofsd/passthrough_ll.c | 77 ++++++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 14 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 38b2af8599..9e93bcdbb3 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -176,6 +176,8 @@ struct lo_data { /* If set, virtiofsd is responsible for setting umask during creation */ bool change_umask; int user_posix_acl, posix_acl; + char **blocked_xattrs; + size_t num_blocked_xattrs; }; static const struct fuse_opt lo_opts[] = { @@ -2811,19 +2813,57 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, assert(fchdir_res == 0); \ } while (0) +/* Returns 0 on success, 1 on failure */ +static int add_blocked_xattr(struct lo_data *lo, const char *name) +{ + size_t nr_elems = lo->num_blocked_xattrs + 1; + + lo->blocked_xattrs = reallocarray(lo->blocked_xattrs, nr_elems, + sizeof(char *)); + if (!lo->blocked_xattrs) { + fuse_log(FUSE_LOG_ERR, "failed to grow blocked xattrs array: %m\n"); + return 1; + } + + lo->blocked_xattrs[nr_elems - 1] = strdup(name); + if (!lo->blocked_xattrs[nr_elems - 1]) { + fuse_log(FUSE_LOG_ERR, "strdup(%s) failed: %m\n", name); + return 1; + } + lo->num_blocked_xattrs++; + return 0; +} + +static void free_blocked_xattrs(struct lo_data *lo) +{ + size_t i; + + if (!lo->num_blocked_xattrs) { + return; + } + + for (i = 0; i < lo->num_blocked_xattrs; i++) { + free(lo->blocked_xattrs[i]); + } + + free(lo->blocked_xattrs); + lo->num_blocked_xattrs = 0; + lo->blocked_xattrs = NULL; +} + static bool block_xattr(struct lo_data *lo, const char *name) { - /* - * If user explicitly enabled posix_acl or did not provide any option, - * do not block acl. Otherwise block system.posix_acl_access and - * system.posix_acl_default xattrs. - */ - if (lo->user_posix_acl) { + size_t i; + + if (!lo->num_blocked_xattrs) { return false; } - if (!strcmp(name, "system.posix_acl_access") || - !strcmp(name, "system.posix_acl_default")) + + for (i = 0; i < lo->num_blocked_xattrs; i++) { + if (!strcmp(name, lo->blocked_xattrs[i])) { return true; + } + } return false; } @@ -2840,12 +2880,7 @@ static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list, { size_t out_index, in_index; - /* - * As of now we only filter out acl xattrs. If acls are enabled or - * they have not been explicitly disabled, there is nothing to - * filter. - */ - if (lo->user_posix_acl) { + if (!lo->num_blocked_xattrs) { return in_size; } @@ -3880,6 +3915,7 @@ static void fuse_lo_data_cleanup(struct lo_data *lo) free(lo->xattrmap); free_xattrmap(lo); free(lo->xattr_security_capability); + free_blocked_xattrs(lo); free(lo->source); } @@ -3920,6 +3956,8 @@ int main(int argc, char *argv[]) lo.root.fd = -1; lo.root.fuse_ino = FUSE_ROOT_ID; lo.cache = CACHE_AUTO; + lo.num_blocked_xattrs = 0; + lo.blocked_xattrs = NULL; /* * Set up the ino map like this: @@ -4036,6 +4074,17 @@ int main(int argc, char *argv[]) exit(1); } + if (!lo.user_posix_acl) { + /* User disabled posix acl explicitly. Block acl xattrs */ + if (add_blocked_xattr(&lo, "system.posix_acl_access")) { + exit(1); + } + + if (add_blocked_xattr(&lo, "system.posix_acl_default")) { + exit(1); + } + } + lo.use_statx = true; se = fuse_session_new(&args, &lo_oper, sizeof(lo_oper), &lo); -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Virtio-fs] [PATCH 1/2] virtiofsd: Add an array to keep track of blocked xattrs @ 2021-08-26 21:19 ` Vivek Goyal 0 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-08-26 21:19 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: vgoyal Right now we have capability to block "system.posix_acl_access" and "system.posix_acl_default" xattrs. But we have sort of hardcoded these two values and its not generic. Now we want to support blocking of arbitrary xattr as passed in by user. So let us keep an array of blocked xattrs and consult that array when deciding whether an attribute is blocked or not. This should not result any functional change. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- tools/virtiofsd/passthrough_ll.c | 77 ++++++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 14 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 38b2af8599..9e93bcdbb3 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -176,6 +176,8 @@ struct lo_data { /* If set, virtiofsd is responsible for setting umask during creation */ bool change_umask; int user_posix_acl, posix_acl; + char **blocked_xattrs; + size_t num_blocked_xattrs; }; static const struct fuse_opt lo_opts[] = { @@ -2811,19 +2813,57 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, assert(fchdir_res == 0); \ } while (0) +/* Returns 0 on success, 1 on failure */ +static int add_blocked_xattr(struct lo_data *lo, const char *name) +{ + size_t nr_elems = lo->num_blocked_xattrs + 1; + + lo->blocked_xattrs = reallocarray(lo->blocked_xattrs, nr_elems, + sizeof(char *)); + if (!lo->blocked_xattrs) { + fuse_log(FUSE_LOG_ERR, "failed to grow blocked xattrs array: %m\n"); + return 1; + } + + lo->blocked_xattrs[nr_elems - 1] = strdup(name); + if (!lo->blocked_xattrs[nr_elems - 1]) { + fuse_log(FUSE_LOG_ERR, "strdup(%s) failed: %m\n", name); + return 1; + } + lo->num_blocked_xattrs++; + return 0; +} + +static void free_blocked_xattrs(struct lo_data *lo) +{ + size_t i; + + if (!lo->num_blocked_xattrs) { + return; + } + + for (i = 0; i < lo->num_blocked_xattrs; i++) { + free(lo->blocked_xattrs[i]); + } + + free(lo->blocked_xattrs); + lo->num_blocked_xattrs = 0; + lo->blocked_xattrs = NULL; +} + static bool block_xattr(struct lo_data *lo, const char *name) { - /* - * If user explicitly enabled posix_acl or did not provide any option, - * do not block acl. Otherwise block system.posix_acl_access and - * system.posix_acl_default xattrs. - */ - if (lo->user_posix_acl) { + size_t i; + + if (!lo->num_blocked_xattrs) { return false; } - if (!strcmp(name, "system.posix_acl_access") || - !strcmp(name, "system.posix_acl_default")) + + for (i = 0; i < lo->num_blocked_xattrs; i++) { + if (!strcmp(name, lo->blocked_xattrs[i])) { return true; + } + } return false; } @@ -2840,12 +2880,7 @@ static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list, { size_t out_index, in_index; - /* - * As of now we only filter out acl xattrs. If acls are enabled or - * they have not been explicitly disabled, there is nothing to - * filter. - */ - if (lo->user_posix_acl) { + if (!lo->num_blocked_xattrs) { return in_size; } @@ -3880,6 +3915,7 @@ static void fuse_lo_data_cleanup(struct lo_data *lo) free(lo->xattrmap); free_xattrmap(lo); free(lo->xattr_security_capability); + free_blocked_xattrs(lo); free(lo->source); } @@ -3920,6 +3956,8 @@ int main(int argc, char *argv[]) lo.root.fd = -1; lo.root.fuse_ino = FUSE_ROOT_ID; lo.cache = CACHE_AUTO; + lo.num_blocked_xattrs = 0; + lo.blocked_xattrs = NULL; /* * Set up the ino map like this: @@ -4036,6 +4074,17 @@ int main(int argc, char *argv[]) exit(1); } + if (!lo.user_posix_acl) { + /* User disabled posix acl explicitly. Block acl xattrs */ + if (add_blocked_xattr(&lo, "system.posix_acl_access")) { + exit(1); + } + + if (add_blocked_xattr(&lo, "system.posix_acl_default")) { + exit(1); + } + } + lo.use_statx = true; se = fuse_session_new(&args, &lo_oper, sizeof(lo_oper), &lo); -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] virtiofsd: Add option "block_xattr=" to block certain xattrs 2021-08-26 21:19 ` [Virtio-fs] " Vivek Goyal @ 2021-08-26 21:19 ` Vivek Goyal -1 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-08-26 21:19 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal We need capability to block security.selinux xattr and return EOPNOTSUPP. That way guest SELinux thinks filesystem does not support selinux xattr and falls back to some default label (virtiofs_t) for the virtiofs filesystem instance. So add a generic option "-o block_xattr=", which can allow user to specify a list of xattrs to block. Xattrs should be ":" separated. For example, "-o block_xattr=security.selinux:user.foo". Valid xattrs to block should belong to one of of the "security", "system", "trusted" or "user" xattr namespace. Ex. -o block_xattr="security.selinux:user.foo" One can also specify prefix which should be matched against xattr name and if prefix matches, that xattr will be blocked. Requirement of xattr belonging to one of the 4 namepsaces still remain in place. For example -o block_xattr="user.virtiofs*" should block any xattr name starting with prefix "user.virtiofs". Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- docs/tools/virtiofsd.rst | 17 ++++++ tools/virtiofsd/helper.c | 3 + tools/virtiofsd/passthrough_ll.c | 101 ++++++++++++++++++++++++++++--- 3 files changed, 114 insertions(+), 7 deletions(-) diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst index b208f2a6f0..406c1ab721 100644 --- a/docs/tools/virtiofsd.rst +++ b/docs/tools/virtiofsd.rst @@ -101,6 +101,23 @@ Options Enable/disable extended attributes (xattr) on files and directories. The default is ``no_xattr``. + * block_xattr=<list-of-xattrs> - + Block xattrs specified in the colon separated list. When an xattr + is blocked getxattr/setxattr/removexattr return error code + EOPNOTSUPP, and listxattr removes the xattr from list if there is one. + + xattr name should belong to one of the four namespsaces, namely + security, system, trusted and user. + + e.g. -o block_xattr=security.selinux:user.foo + + One could also specify just a xattr name prefix followed by "*" to + signify any xattr name matching prefix will be blocked. + + e.g -o block_xattr=user.foo* + + This will block any xattr name starting with "user.foo" + * posix_acl|no_posix_acl - Enable/disable posix acl support. Posix ACLs are disabled by default. diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c index a8295d975a..da674ff70a 100644 --- a/tools/virtiofsd/helper.c +++ b/tools/virtiofsd/helper.c @@ -175,6 +175,9 @@ void fuse_cmdline_help(void) " -o xattrmap=<mapping> Enable xattr mapping (enables xattr)\n" " <mapping> is a string consists of a series of rules\n" " e.g. -o xattrmap=:map::user.virtiofs.:\n" + " -o block_xattr=<xattrs> Block xattrs specified in list\n" + " <xattrs> is colon separated list of xattrs to block\n" + " e.g. -o block_xattr=security.selinux:user.*\n" " -o modcaps=CAPLIST Modify the list of capabilities\n" " e.g. -o modcaps=+sys_admin:-chown\n" " --rlimit-nofile=<num> set maximum number of file descriptors\n" diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 9e93bcdbb3..2008e6be55 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -142,6 +142,12 @@ typedef struct xattr_map_entry { unsigned int flags; } XattrMapEntry; +struct xattr_block_entry { + /* true if name is prefix otherwise false */ + bool prefix; + char *name; +}; + struct lo_data { pthread_mutex_t mutex; int sandbox; @@ -176,8 +182,9 @@ struct lo_data { /* If set, virtiofsd is responsible for setting umask during creation */ bool change_umask; int user_posix_acl, posix_acl; - char **blocked_xattrs; + struct xattr_block_entry *blocked_xattrs; size_t num_blocked_xattrs; + char *block_xattr_str; }; static const struct fuse_opt lo_opts[] = { @@ -212,6 +219,7 @@ static const struct fuse_opt lo_opts[] = { { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 }, { "posix_acl", offsetof(struct lo_data, user_posix_acl), 1 }, { "no_posix_acl", offsetof(struct lo_data, user_posix_acl), 0 }, + { "block_xattr=%s", offsetof(struct lo_data, block_xattr_str), 0 }, FUSE_OPT_END }; static bool use_syslog = false; @@ -2817,23 +2825,88 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, static int add_blocked_xattr(struct lo_data *lo, const char *name) { size_t nr_elems = lo->num_blocked_xattrs + 1; + struct xattr_block_entry *xbe; + char *ptr; lo->blocked_xattrs = reallocarray(lo->blocked_xattrs, nr_elems, - sizeof(char *)); + sizeof(struct xattr_block_entry)); if (!lo->blocked_xattrs) { fuse_log(FUSE_LOG_ERR, "failed to grow blocked xattrs array: %m\n"); return 1; } - lo->blocked_xattrs[nr_elems - 1] = strdup(name); - if (!lo->blocked_xattrs[nr_elems - 1]) { + xbe = &lo->blocked_xattrs[nr_elems - 1]; + xbe->prefix = false; + + ptr = strchr(name, '*'); + if (ptr) { + xbe->prefix = true; + *ptr = '\0'; + } + + xbe->name = strdup(name); + if (!xbe->name) { fuse_log(FUSE_LOG_ERR, "strdup(%s) failed: %m\n", name); return 1; } + lo->num_blocked_xattrs++; return 0; } +/* Returns true on success, false on error */ +static bool valid_block_xattr(char *name) +{ + char *ptr; + + if (!g_str_has_prefix(name, "user.") && + !g_str_has_prefix(name, "system.") && + !g_str_has_prefix(name, "security.") && + !g_str_has_prefix(name, "trusted.")) { + return false; + } + + ptr = strchr(name, '*'); + if (!ptr) { + return true; + } + + /* if there is a '*' in name, it should be last char */ + if (*++ptr != '\0') { + return false; + } + return true; +} + +/* Returns 0 on success, 1 on error */ +static int parse_block_xattr(struct lo_data *lo, char *block_xattr_str) +{ + char *token, *parse_str; + + /* strtok() modifies the string passed. So work on the copy */ + parse_str = strdup(block_xattr_str); + if (!parse_str) { + fuse_log(FUSE_LOG_ERR, "Failed strdup(%s):%m\n", block_xattr_str); + return 1; + } + + while ((token = strtok(parse_str, ":"))) { + parse_str = NULL; + if (!valid_block_xattr(token)) { + fuse_log(FUSE_LOG_ERR, "Invalid xattr to block: %s\n", token); + return 1; + } + if (add_blocked_xattr(lo, token)) { + fuse_log(FUSE_LOG_ERR, "Failed to add blocked xattr %s\n", + token); + free(parse_str); + return 1; + } + } + free(parse_str); + return 0; +} + static void free_blocked_xattrs(struct lo_data *lo) { size_t i; @@ -2843,7 +2916,7 @@ static void free_blocked_xattrs(struct lo_data *lo) } for (i = 0; i < lo->num_blocked_xattrs; i++) { - free(lo->blocked_xattrs[i]); + free(lo->blocked_xattrs[i].name); } free(lo->blocked_xattrs); @@ -2854,14 +2927,22 @@ static void free_blocked_xattrs(struct lo_data *lo) static bool block_xattr(struct lo_data *lo, const char *name) { size_t i; + struct xattr_block_entry *xbe; if (!lo->num_blocked_xattrs) { return false; } for (i = 0; i < lo->num_blocked_xattrs; i++) { - if (!strcmp(name, lo->blocked_xattrs[i])) { - return true; + xbe = &lo->blocked_xattrs[i]; + if (xbe->prefix) { + if (g_str_has_prefix(name, xbe->name)) { + return true; + } + } else { + if (!strcmp(name, xbe->name)) { + return true; + } } } @@ -4068,6 +4149,12 @@ int main(int argc, char *argv[]) exit(1); } + if (lo.block_xattr_str) { + if (parse_block_xattr(&lo, lo.block_xattr_str)) { + exit(1); + } + } + if (lo.user_posix_acl == 1 && !lo.xattr) { fuse_log(FUSE_LOG_ERR, "Can't enable posix ACLs. xattrs are disabled." "\n"); -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Virtio-fs] [PATCH 2/2] virtiofsd: Add option "block_xattr=" to block certain xattrs @ 2021-08-26 21:19 ` Vivek Goyal 0 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-08-26 21:19 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: vgoyal We need capability to block security.selinux xattr and return EOPNOTSUPP. That way guest SELinux thinks filesystem does not support selinux xattr and falls back to some default label (virtiofs_t) for the virtiofs filesystem instance. So add a generic option "-o block_xattr=", which can allow user to specify a list of xattrs to block. Xattrs should be ":" separated. For example, "-o block_xattr=security.selinux:user.foo". Valid xattrs to block should belong to one of of the "security", "system", "trusted" or "user" xattr namespace. Ex. -o block_xattr="security.selinux:user.foo" One can also specify prefix which should be matched against xattr name and if prefix matches, that xattr will be blocked. Requirement of xattr belonging to one of the 4 namepsaces still remain in place. For example -o block_xattr="user.virtiofs*" should block any xattr name starting with prefix "user.virtiofs". Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- docs/tools/virtiofsd.rst | 17 ++++++ tools/virtiofsd/helper.c | 3 + tools/virtiofsd/passthrough_ll.c | 101 ++++++++++++++++++++++++++++--- 3 files changed, 114 insertions(+), 7 deletions(-) diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst index b208f2a6f0..406c1ab721 100644 --- a/docs/tools/virtiofsd.rst +++ b/docs/tools/virtiofsd.rst @@ -101,6 +101,23 @@ Options Enable/disable extended attributes (xattr) on files and directories. The default is ``no_xattr``. + * block_xattr=<list-of-xattrs> - + Block xattrs specified in the colon separated list. When an xattr + is blocked getxattr/setxattr/removexattr return error code + EOPNOTSUPP, and listxattr removes the xattr from list if there is one. + + xattr name should belong to one of the four namespsaces, namely + security, system, trusted and user. + + e.g. -o block_xattr=security.selinux:user.foo + + One could also specify just a xattr name prefix followed by "*" to + signify any xattr name matching prefix will be blocked. + + e.g -o block_xattr=user.foo* + + This will block any xattr name starting with "user.foo" + * posix_acl|no_posix_acl - Enable/disable posix acl support. Posix ACLs are disabled by default. diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c index a8295d975a..da674ff70a 100644 --- a/tools/virtiofsd/helper.c +++ b/tools/virtiofsd/helper.c @@ -175,6 +175,9 @@ void fuse_cmdline_help(void) " -o xattrmap=<mapping> Enable xattr mapping (enables xattr)\n" " <mapping> is a string consists of a series of rules\n" " e.g. -o xattrmap=:map::user.virtiofs.:\n" + " -o block_xattr=<xattrs> Block xattrs specified in list\n" + " <xattrs> is colon separated list of xattrs to block\n" + " e.g. -o block_xattr=security.selinux:user.*\n" " -o modcaps=CAPLIST Modify the list of capabilities\n" " e.g. -o modcaps=+sys_admin:-chown\n" " --rlimit-nofile=<num> set maximum number of file descriptors\n" diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 9e93bcdbb3..2008e6be55 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -142,6 +142,12 @@ typedef struct xattr_map_entry { unsigned int flags; } XattrMapEntry; +struct xattr_block_entry { + /* true if name is prefix otherwise false */ + bool prefix; + char *name; +}; + struct lo_data { pthread_mutex_t mutex; int sandbox; @@ -176,8 +182,9 @@ struct lo_data { /* If set, virtiofsd is responsible for setting umask during creation */ bool change_umask; int user_posix_acl, posix_acl; - char **blocked_xattrs; + struct xattr_block_entry *blocked_xattrs; size_t num_blocked_xattrs; + char *block_xattr_str; }; static const struct fuse_opt lo_opts[] = { @@ -212,6 +219,7 @@ static const struct fuse_opt lo_opts[] = { { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 }, { "posix_acl", offsetof(struct lo_data, user_posix_acl), 1 }, { "no_posix_acl", offsetof(struct lo_data, user_posix_acl), 0 }, + { "block_xattr=%s", offsetof(struct lo_data, block_xattr_str), 0 }, FUSE_OPT_END }; static bool use_syslog = false; @@ -2817,23 +2825,88 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, static int add_blocked_xattr(struct lo_data *lo, const char *name) { size_t nr_elems = lo->num_blocked_xattrs + 1; + struct xattr_block_entry *xbe; + char *ptr; lo->blocked_xattrs = reallocarray(lo->blocked_xattrs, nr_elems, - sizeof(char *)); + sizeof(struct xattr_block_entry)); if (!lo->blocked_xattrs) { fuse_log(FUSE_LOG_ERR, "failed to grow blocked xattrs array: %m\n"); return 1; } - lo->blocked_xattrs[nr_elems - 1] = strdup(name); - if (!lo->blocked_xattrs[nr_elems - 1]) { + xbe = &lo->blocked_xattrs[nr_elems - 1]; + xbe->prefix = false; + + ptr = strchr(name, '*'); + if (ptr) { + xbe->prefix = true; + *ptr = '\0'; + } + + xbe->name = strdup(name); + if (!xbe->name) { fuse_log(FUSE_LOG_ERR, "strdup(%s) failed: %m\n", name); return 1; } + lo->num_blocked_xattrs++; return 0; } +/* Returns true on success, false on error */ +static bool valid_block_xattr(char *name) +{ + char *ptr; + + if (!g_str_has_prefix(name, "user.") && + !g_str_has_prefix(name, "system.") && + !g_str_has_prefix(name, "security.") && + !g_str_has_prefix(name, "trusted.")) { + return false; + } + + ptr = strchr(name, '*'); + if (!ptr) { + return true; + } + + /* if there is a '*' in name, it should be last char */ + if (*++ptr != '\0') { + return false; + } + return true; +} + +/* Returns 0 on success, 1 on error */ +static int parse_block_xattr(struct lo_data *lo, char *block_xattr_str) +{ + char *token, *parse_str; + + /* strtok() modifies the string passed. So work on the copy */ + parse_str = strdup(block_xattr_str); + if (!parse_str) { + fuse_log(FUSE_LOG_ERR, "Failed strdup(%s):%m\n", block_xattr_str); + return 1; + } + + while ((token = strtok(parse_str, ":"))) { + parse_str = NULL; + if (!valid_block_xattr(token)) { + fuse_log(FUSE_LOG_ERR, "Invalid xattr to block: %s\n", token); + return 1; + } + if (add_blocked_xattr(lo, token)) { + fuse_log(FUSE_LOG_ERR, "Failed to add blocked xattr %s\n", + token); + free(parse_str); + return 1; + } + } + free(parse_str); + return 0; +} + static void free_blocked_xattrs(struct lo_data *lo) { size_t i; @@ -2843,7 +2916,7 @@ static void free_blocked_xattrs(struct lo_data *lo) } for (i = 0; i < lo->num_blocked_xattrs; i++) { - free(lo->blocked_xattrs[i]); + free(lo->blocked_xattrs[i].name); } free(lo->blocked_xattrs); @@ -2854,14 +2927,22 @@ static void free_blocked_xattrs(struct lo_data *lo) static bool block_xattr(struct lo_data *lo, const char *name) { size_t i; + struct xattr_block_entry *xbe; if (!lo->num_blocked_xattrs) { return false; } for (i = 0; i < lo->num_blocked_xattrs; i++) { - if (!strcmp(name, lo->blocked_xattrs[i])) { - return true; + xbe = &lo->blocked_xattrs[i]; + if (xbe->prefix) { + if (g_str_has_prefix(name, xbe->name)) { + return true; + } + } else { + if (!strcmp(name, xbe->name)) { + return true; + } } } @@ -4068,6 +4149,12 @@ int main(int argc, char *argv[]) exit(1); } + if (lo.block_xattr_str) { + if (parse_block_xattr(&lo, lo.block_xattr_str)) { + exit(1); + } + } + if (lo.user_posix_acl == 1 && !lo.xattr) { fuse_log(FUSE_LOG_ERR, "Can't enable posix ACLs. xattrs are disabled." "\n"); -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] virtiofsd: Add capability to block xattrs 2021-08-26 21:19 ` [Virtio-fs] " Vivek Goyal @ 2021-09-22 11:00 ` Dr. David Alan Gilbert -1 siblings, 0 replies; 10+ messages in thread From: Dr. David Alan Gilbert @ 2021-09-22 11:00 UTC (permalink / raw) To: Vivek Goyal; +Cc: virtio-fs, qemu-devel * Vivek Goyal (vgoyal@redhat.com) wrote: > As of now we have a knob "-o xattr/no_xattr" which either enables > all xattrs or disables all xattrs. Hi Vivek, Thanks for this. > We need something more fine grained where we can selectively disable > only certain xattrs (and not all). > > For example, in some cases we want to disable "security.selinux" > xattr. This is equivalent to virtiofs not supporting security.selinux > and guest kernel will fallback to a single label for whole fs > (virtiofs_t). > > So add an option "-o block_xattr=<list-of-xattrs>" which will allow > specifying a list of xattrs to block. This is quite interesting; I'd not noticed you had the exisitng blocking mechanism, however, as discussed, I think my preference is if this could be done as a modification of the xattrmap it would avoid another set of options. The mapping code already has 'type's of: prefix, ok, bad I think you just need to add a 'reject' type that produces the error code you need. Dave > Vivek Goyal (2): > virtiofsd: Add an array to keep track of blocked xattrs > virtiofsd: Add option "block_xattr=" to block certain xattrs > > docs/tools/virtiofsd.rst | 17 ++++ > tools/virtiofsd/helper.c | 3 + > tools/virtiofsd/passthrough_ll.c | 166 ++++++++++++++++++++++++++++--- > 3 files changed, 171 insertions(+), 15 deletions(-) > > -- > 2.31.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Add capability to block xattrs @ 2021-09-22 11:00 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 10+ messages in thread From: Dr. David Alan Gilbert @ 2021-09-22 11:00 UTC (permalink / raw) To: Vivek Goyal; +Cc: virtio-fs, qemu-devel * Vivek Goyal (vgoyal@redhat.com) wrote: > As of now we have a knob "-o xattr/no_xattr" which either enables > all xattrs or disables all xattrs. Hi Vivek, Thanks for this. > We need something more fine grained where we can selectively disable > only certain xattrs (and not all). > > For example, in some cases we want to disable "security.selinux" > xattr. This is equivalent to virtiofs not supporting security.selinux > and guest kernel will fallback to a single label for whole fs > (virtiofs_t). > > So add an option "-o block_xattr=<list-of-xattrs>" which will allow > specifying a list of xattrs to block. This is quite interesting; I'd not noticed you had the exisitng blocking mechanism, however, as discussed, I think my preference is if this could be done as a modification of the xattrmap it would avoid another set of options. The mapping code already has 'type's of: prefix, ok, bad I think you just need to add a 'reject' type that produces the error code you need. Dave > Vivek Goyal (2): > virtiofsd: Add an array to keep track of blocked xattrs > virtiofsd: Add option "block_xattr=" to block certain xattrs > > docs/tools/virtiofsd.rst | 17 ++++ > tools/virtiofsd/helper.c | 3 + > tools/virtiofsd/passthrough_ll.c | 166 ++++++++++++++++++++++++++++--- > 3 files changed, 171 insertions(+), 15 deletions(-) > > -- > 2.31.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] virtiofsd: Add capability to block xattrs 2021-09-22 11:00 ` [Virtio-fs] " Dr. David Alan Gilbert @ 2021-09-22 12:30 ` Vivek Goyal -1 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-09-22 12:30 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: virtio-fs, qemu-devel On Wed, Sep 22, 2021 at 12:00:17PM +0100, Dr. David Alan Gilbert wrote: > * Vivek Goyal (vgoyal@redhat.com) wrote: > > As of now we have a knob "-o xattr/no_xattr" which either enables > > all xattrs or disables all xattrs. > > Hi Vivek, > Thanks for this. > > > We need something more fine grained where we can selectively disable > > only certain xattrs (and not all). > > > > For example, in some cases we want to disable "security.selinux" > > xattr. This is equivalent to virtiofs not supporting security.selinux > > and guest kernel will fallback to a single label for whole fs > > (virtiofs_t). > > > > So add an option "-o block_xattr=<list-of-xattrs>" which will allow > > specifying a list of xattrs to block. > > This is quite interesting; I'd not noticed you had the exisitng blocking > mechanism, Yes, that's for blocking posix acl xattrs if needed. If xattr map support blocking, then we could probably insert an internal rule to block posix acl xattrs. But that's more of a cleanup exercise I will take up some other time. > however, as discussed, I think my preference is if this could > be done as a modification of the xattrmap it would avoid another set of > options. > > The mapping code already has 'type's of: > > prefix, ok, bad > > I think you just need to add a 'reject' type > that produces the error code you need. How about "unsupported" and then return -EOPNOTSUPP? I am looking at selinux kernel code and it expect -EOPNOTSUPP to decide that selinux xattr is not supported and looks into fallback options. static int sb_check_xattr_support(struct super_block *sb) { rc = __vfs_getxattr(root, root_inode, XATTR_NAME_SELINUX, NULL, 0); if (rc < 0 && rc != -ENODATA) { if (rc == -EOPNOTSUPP) { pr_warn("SELinux: (dev %s, type %s) has no security xattr handler\n", sb->s_id, sb->s_type->name); goto fallback; ... ... } Vivek ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Add capability to block xattrs @ 2021-09-22 12:30 ` Vivek Goyal 0 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-09-22 12:30 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: virtio-fs, qemu-devel On Wed, Sep 22, 2021 at 12:00:17PM +0100, Dr. David Alan Gilbert wrote: > * Vivek Goyal (vgoyal@redhat.com) wrote: > > As of now we have a knob "-o xattr/no_xattr" which either enables > > all xattrs or disables all xattrs. > > Hi Vivek, > Thanks for this. > > > We need something more fine grained where we can selectively disable > > only certain xattrs (and not all). > > > > For example, in some cases we want to disable "security.selinux" > > xattr. This is equivalent to virtiofs not supporting security.selinux > > and guest kernel will fallback to a single label for whole fs > > (virtiofs_t). > > > > So add an option "-o block_xattr=<list-of-xattrs>" which will allow > > specifying a list of xattrs to block. > > This is quite interesting; I'd not noticed you had the exisitng blocking > mechanism, Yes, that's for blocking posix acl xattrs if needed. If xattr map support blocking, then we could probably insert an internal rule to block posix acl xattrs. But that's more of a cleanup exercise I will take up some other time. > however, as discussed, I think my preference is if this could > be done as a modification of the xattrmap it would avoid another set of > options. > > The mapping code already has 'type's of: > > prefix, ok, bad > > I think you just need to add a 'reject' type > that produces the error code you need. How about "unsupported" and then return -EOPNOTSUPP? I am looking at selinux kernel code and it expect -EOPNOTSUPP to decide that selinux xattr is not supported and looks into fallback options. static int sb_check_xattr_support(struct super_block *sb) { rc = __vfs_getxattr(root, root_inode, XATTR_NAME_SELINUX, NULL, 0); if (rc < 0 && rc != -ENODATA) { if (rc == -EOPNOTSUPP) { pr_warn("SELinux: (dev %s, type %s) has no security xattr handler\n", sb->s_id, sb->s_type->name); goto fallback; ... ... } Vivek ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-09-22 12:38 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-26 21:19 [PATCH 0/2] virtiofsd: Add capability to block xattrs Vivek Goyal 2021-08-26 21:19 ` [Virtio-fs] " Vivek Goyal 2021-08-26 21:19 ` [PATCH 1/2] virtiofsd: Add an array to keep track of blocked xattrs Vivek Goyal 2021-08-26 21:19 ` [Virtio-fs] " Vivek Goyal 2021-08-26 21:19 ` [PATCH 2/2] virtiofsd: Add option "block_xattr=" to block certain xattrs Vivek Goyal 2021-08-26 21:19 ` [Virtio-fs] " Vivek Goyal 2021-09-22 11:00 ` [PATCH 0/2] virtiofsd: Add capability to block xattrs Dr. David Alan Gilbert 2021-09-22 11:00 ` [Virtio-fs] " Dr. David Alan Gilbert 2021-09-22 12:30 ` Vivek Goyal 2021-09-22 12:30 ` [Virtio-fs] " Vivek Goyal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.