[PATCH v2] vfs: fix fsconfig(2) LSM mount option handling for btrfs

When SELinux security options are passed to btrfs via fsconfig(2) rather
than via mount(2), the operation aborts with an error. What happens is
roughly this sequence:

1. vfs_parse_fs_param() eats away the LSM options and parses them into
   fc->security.
2. legacy_get_tree() finds nothing in ctx->legacy_data, passes this
   nothing to btrfs.
[here btrfs calls another layer of vfs_kern_mount(), but let's ignore
 that for simplicity]
3. btrfs calls security_sb_set_mnt_opts() with empty options.
4. vfs_get_tree() then calls its own security_sb_set_mnt_opts() with the
   options stashed in fc->security.
5. SELinux doesn't like that different options were used for the same
   superblock and returns -EINVAL.

In the case of mount(2), the options are parsed by
legacy_parse_monolithic(), which skips the eating away of security
opts because of the FS_BINARY_MOUNTDATA flag, so they are passed to the
FS via ctx->legacy_data. The second call to security_sb_set_mnt_opts()
(from vfs_get_tree()) now passes empty opts, but the non-empty -> empty
sequence is allowed by SELinux for the FS_BINARY_MOUNTDATA case.

It is a total mess, but the only sane fix for now seems to be to skip
processing the security opts in vfs_parse_fs_param() if the fc has
legacy opts set AND the fs specfies the FS_BINARY_MOUNTDATA flag. This
combination currently matches only btrfs and coda. For btrfs this fixes
the fsconfig(2) behavior, and for coda it makes setting security opts
via fsconfig(2) fail the same way as it would with mount(2) (because
FS_BINARY_MOUNTDATA filesystems are expected to call the mount opts LSM
hooks themselves, but coda never cared enough to do that). I believe
that is an acceptable state until both filesystems (or at least btrfs)
are converted to the new mount API (at which point btrfs won't need to
pretend it takes binary mount data any more and also won't need to call
the LSM hooks itself, assuming it will pass the fc->security information
properly).

Note that we can't skip LSM opts handling in vfs_parse_fs_param() solely
based on FS_BINARY_MOUNTDATA because that would break NFS.

See here for the original report and reproducer:
https://lore.kernel.org/selinux/c02674c970fa292610402aa866c4068772d9ad4e.camel@btinternet.com/

Reported-by: Richard Haines <redacted>
Tested-by: Richard Haines <redacted>
Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

Trying to revive this patch... Sending v2 with style tweaks as suggested
by David Sterba.

v2:
- split the if condition over two lines (David Sterba)
- fix comment style in the comment being reindented (David Sterba)

 fs/fs_context.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index 2834d1afa6e8..e6575102bbbd 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c

@@ -106,12 +106,30 @@ int vfs_parse_fs_param(struct fs_context *fc, struct fs_parameter *param)
 	if (ret != -ENOPARAM)
 		return ret;
 
-	ret = security_fs_context_parse_param(fc, param);
-	if (ret != -ENOPARAM)
-		/* Param belongs to the LSM or is disallowed by the LSM; so
-		 * don't pass to the FS.
-		 */
-		return ret;
+	/*
+	 * In the legacy+binary mode, skip the security_fs_context_parse_param()
+	 * call and let the legacy handler process also the security options.
+	 * It will format them into the monolithic string, where the FS can
+	 * process them (with FS_BINARY_MOUNTDATA it is expected to do it).
+	 *
+	 * Currently, this matches only btrfs and coda. Coda is broken with
+	 * fsconfig(2) anyway, because it does actually take binary data. Btrfs
+	 * only *pretends* to take binary data to work around the SELinux's
+	 * no-remount-with-different-options check, so this allows it to work
+	 * with fsconfig(2) properly.
+	 *
+	 * Once btrfs is ported to the new mount API, this hack can be reverted.
+	 */
+	if (fc->ops != &legacy_fs_context_ops ||
+	    !(fc->fs_type->fs_flags & FS_BINARY_MOUNTDATA)) {
+		ret = security_fs_context_parse_param(fc, param);
+		if (ret != -ENOPARAM)
+			/*
+			 * Param belongs to the LSM or is disallowed by the LSM;
+			 * so don't pass to the FS.
+			 */
+			return ret;
+	}
 
 	if (fc->ops->parse_param) {
 		ret = fc->ops->parse_param(fc, param);

-- 
2.30.2

On Tue, Mar 16, 2021 at 10:48 AM Ondrej Mosnacek [off-list ref] wrote:

When SELinux security options are passed to btrfs via fsconfig(2) rather
than via mount(2), the operation aborts with an error. What happens is
roughly this sequence:

1. vfs_parse_fs_param() eats away the LSM options and parses them into
   fc->security.
2. legacy_get_tree() finds nothing in ctx->legacy_data, passes this
   nothing to btrfs.
[here btrfs calls another layer of vfs_kern_mount(), but let's ignore
 that for simplicity]
3. btrfs calls security_sb_set_mnt_opts() with empty options.
4. vfs_get_tree() then calls its own security_sb_set_mnt_opts() with the
   options stashed in fc->security.
5. SELinux doesn't like that different options were used for the same
   superblock and returns -EINVAL.

In the case of mount(2), the options are parsed by
legacy_parse_monolithic(), which skips the eating away of security
opts because of the FS_BINARY_MOUNTDATA flag, so they are passed to the
FS via ctx->legacy_data. The second call to security_sb_set_mnt_opts()
(from vfs_get_tree()) now passes empty opts, but the non-empty -> empty
sequence is allowed by SELinux for the FS_BINARY_MOUNTDATA case.

It is a total mess, but the only sane fix for now seems to be to skip
processing the security opts in vfs_parse_fs_param() if the fc has
legacy opts set AND the fs specfies the FS_BINARY_MOUNTDATA flag. This
combination currently matches only btrfs and coda. For btrfs this fixes
the fsconfig(2) behavior, and for coda it makes setting security opts
via fsconfig(2) fail the same way as it would with mount(2) (because
FS_BINARY_MOUNTDATA filesystems are expected to call the mount opts LSM
hooks themselves, but coda never cared enough to do that). I believe
that is an acceptable state until both filesystems (or at least btrfs)
are converted to the new mount API (at which point btrfs won't need to
pretend it takes binary mount data any more and also won't need to call
the LSM hooks itself, assuming it will pass the fc->security information
properly).

Note that we can't skip LSM opts handling in vfs_parse_fs_param() solely
based on FS_BINARY_MOUNTDATA because that would break NFS.

See here for the original report and reproducer:
https://lore.kernel.org/selinux/c02674c970fa292610402aa866c4068772d9ad4e.camel@btinternet.com/

Reported-by: Richard Haines <redacted>
Tested-by: Richard Haines <redacted>
Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

Trying to revive this patch... Sending v2 with style tweaks as suggested
by David Sterba.

v2:
- split the if condition over two lines (David Sterba)
- fix comment style in the comment being reindented (David Sterba)

 fs/fs_context.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

VFS folks, can we get a verdict/feedback on this patch?  The v1 draft
of this patch was posted almost four months ago with no serious
comments/feedback.  It's a bit ugly, but it does appear to work and at
the very least SELinux needs this to handle btrfs properly, other LSMs
may need this too.

-- 
paul moore
www.paul-moore.com

On Tue, Mar 16, 2021 at 02:21:45PM -0400, Paul Moore wrote:

On Tue, Mar 16, 2021 at 10:48 AM Ondrej Mosnacek [off-list ref] wrote:

quoted

When SELinux security options are passed to btrfs via fsconfig(2) rather
than via mount(2), the operation aborts with an error. What happens is
roughly this sequence:

1. vfs_parse_fs_param() eats away the LSM options and parses them into
   fc->security.
2. legacy_get_tree() finds nothing in ctx->legacy_data, passes this
   nothing to btrfs.
[here btrfs calls another layer of vfs_kern_mount(), but let's ignore
 that for simplicity]

Let's not.  This is where the root of the problem actually lies.  Take a look
at that sucker:

        struct fs_context *fc;
        struct vfsmount *mnt;
        int ret = 0;

        if (!type)
                return ERR_PTR(-EINVAL);

        fc = fs_context_for_mount(type, flags);
        if (IS_ERR(fc))
                return ERR_CAST(fc);

        if (name)
                ret = vfs_parse_fs_string(fc, "source",
                                          name, strlen(name));
        if (!ret)
                ret = parse_monolithic_mount_data(fc, data);  
        if (!ret)
                mnt = fc_mount(fc);
        else   
                mnt = ERR_PTR(ret);

        put_fs_context(fc);
        return mnt;

That's where the problem comes - you've lost the original context's ->security.
Note that there's such thing as security_fs_context_dup(), so you can bloody
well either
	* provide a variant of vfs_kern_mount() that would take 'base' fc to
pick security options from or
	* do all options parsing on btrfs fc and then do fs_context_for_mount +
security_fs_context_dup + copy (parsed) options to whatever private data you
use for btrfs_root context + fc_mount + put_fs_context yourself. 

My preference would be the latter, but I have *not* looked at btrfs mount options
handling in any details.

VFS folks, can we get a verdict/feedback on this patch?  The v1 draft
of this patch was posted almost four months ago with no serious
comments/feedback.  It's a bit ugly, but it does appear to work and at
the very least SELinux needs this to handle btrfs properly, other LSMs
may need this too.

It's more than a bit ugly; it perpetuates the use of FS_BINARY_MOUNTDATA,
and the semantics it gets is quite counterintuitive at that.

On Tue, Mar 16, 2021 at 8:25 PM Al Viro [off-list ref] wrote:

On Tue, Mar 16, 2021 at 02:21:45PM -0400, Paul Moore wrote:

quoted

On Tue, Mar 16, 2021 at 10:48 AM Ondrej Mosnacek [off-list ref] wrote:

quoted

When SELinux security options are passed to btrfs via fsconfig(2) rather
than via mount(2), the operation aborts with an error. What happens is
roughly this sequence:

1. vfs_parse_fs_param() eats away the LSM options and parses them into
   fc->security.
2. legacy_get_tree() finds nothing in ctx->legacy_data, passes this
   nothing to btrfs.
[here btrfs calls another layer of vfs_kern_mount(), but let's ignore
 that for simplicity]

Let's not.  This is where the root of the problem actually lies.  Take a look
at that sucker:

        struct fs_context *fc;
        struct vfsmount *mnt;
        int ret = 0;

        if (!type)
                return ERR_PTR(-EINVAL);

        fc = fs_context_for_mount(type, flags);
        if (IS_ERR(fc))
                return ERR_CAST(fc);

        if (name)
                ret = vfs_parse_fs_string(fc, "source",
                                          name, strlen(name));
        if (!ret)
                ret = parse_monolithic_mount_data(fc, data);
        if (!ret)
                mnt = fc_mount(fc);
        else
                mnt = ERR_PTR(ret);

        put_fs_context(fc);
        return mnt;

That's where the problem comes - you've lost the original context's ->security.
Note that there's such thing as security_fs_context_dup(), so you can bloody
well either
        * provide a variant of vfs_kern_mount() that would take 'base' fc to
pick security options from or
        * do all options parsing on btrfs fc and then do fs_context_for_mount +
security_fs_context_dup + copy (parsed) options to whatever private data you
use for btrfs_root context + fc_mount + put_fs_context yourself.

My preference would be the latter, but I have *not* looked at btrfs mount options
handling in any details.

Ack, I'll look into that. I didn't dare to try to touch btrfs mount
handling (if it was straightforward, someone would've already done it,
right? :), but it sounds like converting just this one
vfs_kern_mount() could be relatively easy, would fix the bug, and
would be an incremental improvement.

quoted

VFS folks, can we get a verdict/feedback on this patch?  The v1 draft
of this patch was posted almost four months ago with no serious
comments/feedback.  It's a bit ugly, but it does appear to work and at
the very least SELinux needs this to handle btrfs properly, other LSMs
may need this too.

It's more than a bit ugly; it perpetuates the use of FS_BINARY_MOUNTDATA,
and the semantics it gets is quite counterintuitive at that.

Fair enough.

Thank you for the feedback and hints!

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

On Thu, Mar 18, 2021 at 10:42 AM Ondrej Mosnacek [off-list ref] wrote:

On Tue, Mar 16, 2021 at 8:25 PM Al Viro [off-list ref] wrote:

quoted

On Tue, Mar 16, 2021 at 02:21:45PM -0400, Paul Moore wrote:

quoted

On Tue, Mar 16, 2021 at 10:48 AM Ondrej Mosnacek [off-list ref] wrote:

quoted

When SELinux security options are passed to btrfs via fsconfig(2) rather
than via mount(2), the operation aborts with an error. What happens is
roughly this sequence:

1. vfs_parse_fs_param() eats away the LSM options and parses them into
   fc->security.
2. legacy_get_tree() finds nothing in ctx->legacy_data, passes this
   nothing to btrfs.
[here btrfs calls another layer of vfs_kern_mount(), but let's ignore
 that for simplicity]

Let's not.  This is where the root of the problem actually lies.  Take a look
at that sucker:

        struct fs_context *fc;
        struct vfsmount *mnt;
        int ret = 0;

        if (!type)
                return ERR_PTR(-EINVAL);

        fc = fs_context_for_mount(type, flags);
        if (IS_ERR(fc))
                return ERR_CAST(fc);

        if (name)
                ret = vfs_parse_fs_string(fc, "source",
                                          name, strlen(name));
        if (!ret)
                ret = parse_monolithic_mount_data(fc, data);
        if (!ret)
                mnt = fc_mount(fc);
        else
                mnt = ERR_PTR(ret);

        put_fs_context(fc);
        return mnt;

That's where the problem comes - you've lost the original context's ->security.
Note that there's such thing as security_fs_context_dup(), so you can bloody
well either
        * provide a variant of vfs_kern_mount() that would take 'base' fc to
pick security options from or
        * do all options parsing on btrfs fc and then do fs_context_for_mount +
security_fs_context_dup + copy (parsed) options to whatever private data you
use for btrfs_root context + fc_mount + put_fs_context yourself.

My preference would be the latter, but I have *not* looked at btrfs mount options
handling in any details.

Ack, I'll look into that. I didn't dare to try to touch btrfs mount
handling (if it was straightforward, someone would've already done it,
right? :), but it sounds like converting just this one
vfs_kern_mount() could be relatively easy, would fix the bug, and
would be an incremental improvement.

After taking a closer look, it seems this won't actually work... The
problem is that since btrfs still uses the legacy mount API, it has no
way to get to fs_context in btrfs_mount() and thus both of your
suggestions aren't really workable (again, without converting btrfs at
least partially to the new API)...

So unless you have other ideas, I'd like to put the original patch
back on the table. Note that the change can be reverted as soon as
btrfs is ported to the fs_context API properly, it's not something
that would need to stick around forever.

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

On Mon, Mar 29, 2021 at 11:00:39AM +0200, Ondrej Mosnacek wrote:

After taking a closer look, it seems this won't actually work... The
problem is that since btrfs still uses the legacy mount API, it has no
way to get to fs_context in btrfs_mount() and thus both of your
suggestions aren't really workable (again, without converting btrfs at
least partially to the new API)...

.. and that conversion is long overdue anyway ..