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

[Ondrej Mosnacek <omosnace@redhat.com>]

On Tue, Mar 16, 2021 at 8:25 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Mar 16, 2021 at 02:21:45PM -0400, Paul Moore wrote:
> > 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]
>
> 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.

>
> > 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.