diff mbox series

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

Message ID 20201118102342.154277-1-omosnace@redhat.com (mailing list archive)
State New, archived
Headers show
Series vfs: fix fsconfig(2) LSM mount option handling for btrfs | expand

Commit Message

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

Comments

Richard Haines Nov. 18, 2020, 12:13 p.m. UTC | #1
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@btinternet.com>
> Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock
> creation/configuration context")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  fs/fs_context.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 

Tested-by: Richard Haines <richard_c_haines@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);
Casey Schaufler Nov. 18, 2020, 4:39 p.m. UTC | #2
On 11/18/2020 2:23 AM, 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@btinternet.com>
> Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  fs/fs_context.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> 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 the real fix is to port btrfs to the new mount API why not do that instead?

> +	 */
> +	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);
David Sterba Nov. 18, 2020, 4:48 p.m. UTC | #3
On Wed, Nov 18, 2020 at 08:39:14AM -0800, Casey Schaufler wrote:
> On 11/18/2020 2:23 AM, 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@btinternet.com>
> > Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  fs/fs_context.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> >
> > 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 the real fix is to port btrfs to the new mount API why not do that instead?

Porting to the new API is much more work compared to this fix, which can
be also backported to older stable trees if needed. The port will
happen eventually but nobody is working on it right now.
David Sterba Dec. 16, 2020, 4:37 p.m. UTC | #4
On Wed, Nov 18, 2020 at 11:23:42AM +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@btinternet.com>
> Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Can we get this merged via the vfs tree, please? Possibly with

CC: stable@vger.kernel.org # 5.4+

> +	/*
> +	 * 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)) {

Line is way over 80, it could be split like

	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.
> +			 */

The multi line comment should have the /* on a separate line (yes it's
in the original code too but such things could be fixed when the code is
moved).

> +			return ret;
> +	}
>  
>  	if (fc->ops->parse_param) {
>  		ret = fc->ops->parse_param(fc, param);
Ondrej Mosnacek Dec. 16, 2020, 6:04 p.m. UTC | #5
On Wed, Dec 16, 2020 at 5:40 PM David Sterba <dsterba@suse.cz> wrote:
> On Wed, Nov 18, 2020 at 11:23:42AM +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@btinternet.com>
> > Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> Can we get this merged via the vfs tree, please? Possibly with
>
> CC: stable@vger.kernel.org # 5.4+
>
> > +     /*
> > +      * 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)) {
>
> Line is way over 80, it could be split like
>
>         if (fc->ops != &legacy_fs_context_ops ||
>             !(fc->fs_type->fs_flags & FS_BINARY_MOUNTDATA)) {

The chackpatch.pl limit is now 100 chars, so I hoped I would get away
with it :) Splitting conditionals always looks kinda awkward... But I
have no problem with changing it, if the VFS maintainers prefer that.
I would like to get at least *some* feedback from them before I respin
with just style changes...

>
> > +             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.
> > +                      */
>
> The multi line comment should have the /* on a separate line (yes it's
> in the original code too but such things could be fixed when the code is
> moved).

Okay. I prefer the "Linus" format as well, but since different
subsystems still have their own opinions, I figured I'd just leave it
be... But again, I'll be happy to change it if VFS maintainers don't
object.

>
> > +                     return ret;
> > +     }
> >
> >       if (fc->ops->parse_param) {
> >               ret = fc->ops->parse_param(fc, param);
>
Paul Moore Jan. 12, 2021, 1:51 p.m. UTC | #6
On Wed, Nov 18, 2020 at 5:23 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>
> Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  fs/fs_context.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)

What do the VFS folks think about this patch?  It has been sitting for
a couple of months now without any real comment and it would be nice
to get some initial feedback on this as it fixes a real problem.

> 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);
> --
> 2.26.2
diff mbox series

Patch

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);