On Tue, Mar 16, 2021 at 8:25 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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.
