From: Paul Moore <paul@paul-moore.com>
To: linux-btrfs@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org,
David Howells <dhowells@redhat.com>,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
Stephen Smalley <stephen.smalley.work@gmail.com>,
Richard Haines <richard_c_haines@btinternet.com>,
Ondrej Mosnacek <omosnace@redhat.com>
Subject: Re: [PATCH v2] vfs: fix fsconfig(2) LSM mount option handling for btrfs
Date: Tue, 16 Mar 2021 14:21:45 -0400 [thread overview]
Message-ID: <CAHC9VhRoTjimpKrrQ5f04SE7AOcGv6p5iBgSnoSRgtiUP47rRg@mail.gmail.com> (raw)
In-Reply-To: <20210316144823.2188946-1-omosnace@redhat.com>
On Tue, Mar 16, 2021 at 10:48 AM Ondrej Mosnacek <omosnace@redhat.com> 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 <richard_c_haines@btinternet.com>
> Tested-by: Richard Haines <richard_c_haines@btinternet.com>
> 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.
> 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
--
paul moore
www.paul-moore.com
next prev parent reply other threads:[~2021-03-16 18:22 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-16 14:48 [PATCH v2] vfs: fix fsconfig(2) LSM mount option handling for btrfs Ondrej Mosnacek
2021-03-16 18:21 ` Paul Moore [this message]
2021-03-16 18:59 ` Al Viro
2021-03-18 9:42 ` Ondrej Mosnacek
2021-03-29 9:00 ` Ondrej Mosnacek
2021-04-01 6:54 ` Christoph Hellwig
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=CAHC9VhRoTjimpKrrQ5f04SE7AOcGv6p5iBgSnoSRgtiUP47rRg@mail.gmail.com \
--to=paul@paul-moore.com \
--cc=dhowells@redhat.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=omosnace@redhat.com \
--cc=richard_c_haines@btinternet.com \
--cc=selinux@vger.kernel.org \
--cc=stephen.smalley.work@gmail.com \
--cc=viro@zeniv.linux.org.uk \
/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).