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

From: Ondrej Mosnacek <omosnace@redhat.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Paul Moore <paul@paul-moore.com>,
	linux-btrfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	David Howells <dhowells@redhat.com>,
	Linux Security Module list 
	<linux-security-module@vger.kernel.org>,
	SElinux list <selinux@vger.kernel.org>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	Richard Haines <richard_c_haines@btinternet.com>
Subject: Re: [PATCH v2] vfs: fix fsconfig(2) LSM mount option handling for btrfs
Date: Mon, 29 Mar 2021 11:00:39 +0200	[thread overview]
Message-ID: <CAFqZXNtBrGVrjXAbrn30QSMFP4Gc99fRK23hMujxYu_otzu0yA@mail.gmail.com> (raw)
In-Reply-To: <CAFqZXNukusUPp+kO7vxPZBt5ehkpH6EUZ5e8XwUq9adOQHdMkQ@mail.gmail.com>

On Thu, Mar 18, 2021 at 10:42 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> 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.

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.