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

[Richard Haines]

On Wed, 2020-11-18 at 11:23 +0100, Ondrej Mosnacek 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 at btinternet.com>
> Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock
> creation/configuration context")
> Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
> ---
>  fs/fs_context.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 

Tested-by: Richard Haines <richard_c_haines at btinternet.com>

Ran the selinux-testsuite with the 'Add btrfs support for filesystem
tests' patch [1] installed, plus the NFS './tools/nfs.sh' tests. All
passed okay.

[1]
https://lore.kernel.org/selinux/20201103110121.53919-1-richard_c_haines@btinternet.com/


> diff --git a/fs/fs_context.c b/fs/fs_context.c
> index 2834d1afa6e80..cfc5ee2e381ef 100644
> --- a/fs/fs_context.c
> +++ b/fs/fs_context.c
> @@ -106,12 +106,28 @@ 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);