[PATCH 0/2] virtiofsd: Add capability to block xattrs

[PATCH 0/2] virtiofsd: Add capability to block xattrs

[Vivek Goyal]

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

[PATCH 1/2] virtiofsd: Add an array to keep track of blocked xattrs

[Vivek Goyal]

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

[PATCH 2/2] virtiofsd: Add option "block_xattr=" to block certain xattrs

[Vivek Goyal]

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

Re: [PATCH 0/2] virtiofsd: Add capability to block xattrs

[Dr. David Alan Gilbert]

* 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

Re: [PATCH 0/2] virtiofsd: Add capability to block xattrs

[Vivek Goyal]

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