diff mbox series

[v2,2/2] NFSv4 account for selinux security context when deciding to share superblock

Message ID 20210218195046.19280-2-olga.kornievskaia@gmail.com (mailing list archive)
State New
Headers show
Series [v2,1/2,security] Add new hook to compare new mount to an existing mount | expand

Commit Message

Olga Kornievskaia Feb. 18, 2021, 7:50 p.m. UTC
From: Olga Kornievskaia <kolga@netapp.com>

Keep track of whether or not there was an selinux context mount
options during the mount. While deciding if the superblock can be
shared for the new mount, check for if we had selinux context on
the existing mount and call into selinux to tell if new passed
in selinux context is compatible with the existing mount's options.

Previously, NFS wasn't able to do the following 2mounts:
mount -o vers=4.2,sec=sys,context=system_u:object_r:root_t:s0
<serverip>:/ /mnt
mount -o vers=4.2,sec=sys,context=system_u:object_r:swapfile_t:s0
<serverip>:/scratch /scratch

2nd mount would fail with "mount.nfs: an incorrect mount option was
specified" and var log messages would have:
"SElinux: mount invalid. Same superblock, different security
settings for.."

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/fs_context.c       | 3 +++
 fs/nfs/internal.h         | 1 +
 fs/nfs/super.c            | 4 ++++
 include/linux/nfs_fs_sb.h | 1 +
 4 files changed, 9 insertions(+)

Comments

Casey Schaufler Feb. 18, 2021, 10:07 p.m. UTC | #1
On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
>
> Keep track of whether or not there was an selinux context mount
> options during the mount.

This may be the intention, but it's not what the change you're
introducing here does.


>  While deciding if the superblock can be
> shared for the new mount, check for if we had selinux context on
> the existing mount and call into selinux to tell if new passed
> in selinux context is compatible with the existing mount's options.

You're describing how you expect the change to be used, not
what it does. If I am the author of a security module other
than SELinux (which, it turns out, I am) what would I use this
for? How might this change interact with my security module?
Is this something I might exploit? If I am the author of a
filesystem other than NFS (which I am not) should I be doing
the same thing?

>
> Previously, NFS wasn't able to do the following 2mounts:
> mount -o vers=4.2,sec=sys,context=system_u:object_r:root_t:s0
> <serverip>:/ /mnt
> mount -o vers=4.2,sec=sys,context=system_u:object_r:swapfile_t:s0
> <serverip>:/scratch /scratch
>
> 2nd mount would fail with "mount.nfs: an incorrect mount option was
> specified" and var log messages would have:
> "SElinux: mount invalid. Same superblock, different security
> settings for.."
>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/fs_context.c       | 3 +++
>  fs/nfs/internal.h         | 1 +
>  fs/nfs/super.c            | 4 ++++
>  include/linux/nfs_fs_sb.h | 1 +
>  4 files changed, 9 insertions(+)
>
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 06894bcdea2d..8067f055d842 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -448,6 +448,9 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  	if (opt < 0)
>  		return ctx->sloppy ? 1 : opt;
>  
> +	if (fc->security)
> +		ctx->has_sec_mnt_opts = 1;
> +
>  	switch (opt) {
>  	case Opt_source:
>  		if (fc->source)
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 62d3189745cd..08f4f34e8cf5 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -96,6 +96,7 @@ struct nfs_fs_context {
>  	char			*fscache_uniq;
>  	unsigned short		protofamily;
>  	unsigned short		mountfamily;
> +	bool			has_sec_mnt_opts;
>  
>  	struct {
>  		union {
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 4034102010f0..0a2d252cf90f 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1058,6 +1058,7 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
>  						 &sb->s_blocksize_bits);
>  
>  	nfs_super_set_maxbytes(sb, server->maxfilesize);
> +	server->has_sec_mnt_opts = ctx->has_sec_mnt_opts;
>  }
>  
>  static int nfs_compare_mount_options(const struct super_block *s, const struct nfs_server *b,
> @@ -1174,6 +1175,9 @@ static int nfs_compare_super(struct super_block *sb, struct fs_context *fc)
>  		return 0;
>  	if (!nfs_compare_userns(old, server))
>  		return 0;
> +	if ((old->has_sec_mnt_opts || fc->security) &&
> +			security_sb_mnt_opts_compat(sb, fc->security))
> +		return 0;
>  	return nfs_compare_mount_options(sb, server, fc);
>  }
>  
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index 38e60ec742df..3f0acada5794 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -254,6 +254,7 @@ struct nfs_server {
>  
>  	/* User namespace info */
>  	const struct cred	*cred;
> +	bool			has_sec_mnt_opts;
>  };
>  
>  /* Server capabilities */
Olga Kornievskaia Feb. 18, 2021, 10:39 p.m. UTC | #2
On Thu, Feb 18, 2021 at 5:07 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Keep track of whether or not there was an selinux context mount
> > options during the mount.
>
> This may be the intention, but it's not what the change you're
> introducing here does.

Can you explain why, as I must be doing something wrong? I'm familiar
with NFS but not with Selinux and I thought I was doing the correct
changes to the Selinux. If that's not the case would you share what
has been done incorrectly.

> >  While deciding if the superblock can be
> > shared for the new mount, check for if we had selinux context on
> > the existing mount and call into selinux to tell if new passed
> > in selinux context is compatible with the existing mount's options.
>
> You're describing how you expect the change to be used, not
> what it does. If I am the author of a security module other
> than SELinux (which, it turns out, I am) what would I use this
> for? How might this change interact with my security module?
> Is this something I might exploit? If I am the author of a
> filesystem other than NFS (which I am not) should I be doing
> the same thing?

I'm not sure I'm understanding your questions. I'm solving a bug that
exists when NFS interacts with selinux. This is really not a feature
that I'm introducing. I thought it was somewhat descriptive with an
example that if you want to mount with different security contexts but
whatever you are mounting has a possibility of sharing the same
superblock, we need to be careful and take security contexts that are
being used by those mount into account to decide whether or not to
share the superblock. Again I thought that's exactly what the commit
states. A different security module, if it acts on security context,
would have to have the same ability to compare if one context options
are compatible with anothers. Another filesystem can decide if it
wants to share superblocks based on compatibility of existing security
context and new one. Is that what you are asking?


>
> >
> > Previously, NFS wasn't able to do the following 2mounts:
> > mount -o vers=4.2,sec=sys,context=system_u:object_r:root_t:s0
> > <serverip>:/ /mnt
> > mount -o vers=4.2,sec=sys,context=system_u:object_r:swapfile_t:s0
> > <serverip>:/scratch /scratch
> >
> > 2nd mount would fail with "mount.nfs: an incorrect mount option was
> > specified" and var log messages would have:
> > "SElinux: mount invalid. Same superblock, different security
> > settings for.."
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/fs_context.c       | 3 +++
> >  fs/nfs/internal.h         | 1 +
> >  fs/nfs/super.c            | 4 ++++
> >  include/linux/nfs_fs_sb.h | 1 +
> >  4 files changed, 9 insertions(+)
> >
> > diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> > index 06894bcdea2d..8067f055d842 100644
> > --- a/fs/nfs/fs_context.c
> > +++ b/fs/nfs/fs_context.c
> > @@ -448,6 +448,9 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
> >       if (opt < 0)
> >               return ctx->sloppy ? 1 : opt;
> >
> > +     if (fc->security)
> > +             ctx->has_sec_mnt_opts = 1;
> > +
> >       switch (opt) {
> >       case Opt_source:
> >               if (fc->source)
> > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > index 62d3189745cd..08f4f34e8cf5 100644
> > --- a/fs/nfs/internal.h
> > +++ b/fs/nfs/internal.h
> > @@ -96,6 +96,7 @@ struct nfs_fs_context {
> >       char                    *fscache_uniq;
> >       unsigned short          protofamily;
> >       unsigned short          mountfamily;
> > +     bool                    has_sec_mnt_opts;
> >
> >       struct {
> >               union {
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index 4034102010f0..0a2d252cf90f 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -1058,6 +1058,7 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
> >                                                &sb->s_blocksize_bits);
> >
> >       nfs_super_set_maxbytes(sb, server->maxfilesize);
> > +     server->has_sec_mnt_opts = ctx->has_sec_mnt_opts;
> >  }
> >
> >  static int nfs_compare_mount_options(const struct super_block *s, const struct nfs_server *b,
> > @@ -1174,6 +1175,9 @@ static int nfs_compare_super(struct super_block *sb, struct fs_context *fc)
> >               return 0;
> >       if (!nfs_compare_userns(old, server))
> >               return 0;
> > +     if ((old->has_sec_mnt_opts || fc->security) &&
> > +                     security_sb_mnt_opts_compat(sb, fc->security))
> > +             return 0;
> >       return nfs_compare_mount_options(sb, server, fc);
> >  }
> >
> > diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> > index 38e60ec742df..3f0acada5794 100644
> > --- a/include/linux/nfs_fs_sb.h
> > +++ b/include/linux/nfs_fs_sb.h
> > @@ -254,6 +254,7 @@ struct nfs_server {
> >
> >       /* User namespace info */
> >       const struct cred       *cred;
> > +     bool                    has_sec_mnt_opts;
> >  };
> >
> >  /* Server capabilities */
>
Casey Schaufler Feb. 18, 2021, 11:17 p.m. UTC | #3
On 2/18/2021 2:39 PM, Olga Kornievskaia wrote:
> On Thu, Feb 18, 2021 at 5:07 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>
>>> Keep track of whether or not there was an selinux context mount
>>> options during the mount.
>> This may be the intention, but it's not what the change you're
>> introducing here does.
> Can you explain why, as I must be doing something wrong? I'm familiar
> with NFS but not with Selinux and I thought I was doing the correct
> changes to the Selinux. If that's not the case would you share what
> has been done incorrectly.

The code in this patch is generic for any LSM that wants to provide
a security_sb_mnt_opts_compat() hook. Your 1/2 patch deals with how
the hook provided by SELinux behaves.  All the behavior that is specific
to SELinux should be in the SELinux hook. If you can state the behavior
generically you should do that here.


>>>  While deciding if the superblock can be
>>> shared for the new mount, check for if we had selinux context on
>>> the existing mount and call into selinux to tell if new passed
>>> in selinux context is compatible with the existing mount's options.
>> You're describing how you expect the change to be used, not
>> what it does. If I am the author of a security module other
>> than SELinux (which, it turns out, I am) what would I use this
>> for? How might this change interact with my security module?
>> Is this something I might exploit? If I am the author of a
>> filesystem other than NFS (which I am not) should I be doing
>> the same thing?
> I'm not sure I'm understanding your questions. I'm solving a bug that
> exists when NFS interacts with selinux.

Right, but you're introducing an LSM interface to do so. LSM interfaces
are expected to be usable by any security module. Smack ought to be able
to provide NFS support, so might be expected to provide a hook for
security_sb_mnt_opts_compat().

>  This is really not a feature
> that I'm introducing. I thought it was somewhat descriptive with an
> example that if you want to mount with different security contexts but
> whatever you are mounting has a possibility of sharing the same
> superblock, we need to be careful and take security contexts that are
> being used by those mount into account to decide whether or not to
> share the superblock. Again I thought that's exactly what the commit
> states. A different security module, if it acts on security context,
> would have to have the same ability to compare if one context options
> are compatible with anothers.

Not everyone is going to extrapolate the general behavior from
the SELinux behavior. Your description of the SELinux behavior in 1/2
is fine. I'm looking for how a module other than SELinux would use
this.

>  Another filesystem can decide if it
> wants to share superblocks based on compatibility of existing security
> context and new one. Is that what you are asking?

What I'm asking is whether this should be a concern for filesystems
in general, in which case this isn't the right place to make this change.

>
>
>>> Previously, NFS wasn't able to do the following 2mounts:
>>> mount -o vers=4.2,sec=sys,context=system_u:object_r:root_t:s0
>>> <serverip>:/ /mnt
>>> mount -o vers=4.2,sec=sys,context=system_u:object_r:swapfile_t:s0
>>> <serverip>:/scratch /scratch
>>>
>>> 2nd mount would fail with "mount.nfs: an incorrect mount option was
>>> specified" and var log messages would have:
>>> "SElinux: mount invalid. Same superblock, different security
>>> settings for.."
>>>
>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>>  fs/nfs/fs_context.c       | 3 +++
>>>  fs/nfs/internal.h         | 1 +
>>>  fs/nfs/super.c            | 4 ++++
>>>  include/linux/nfs_fs_sb.h | 1 +
>>>  4 files changed, 9 insertions(+)
>>>
>>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
>>> index 06894bcdea2d..8067f055d842 100644
>>> --- a/fs/nfs/fs_context.c
>>> +++ b/fs/nfs/fs_context.c
>>> @@ -448,6 +448,9 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>>>       if (opt < 0)
>>>               return ctx->sloppy ? 1 : opt;
>>>
>>> +     if (fc->security)
>>> +             ctx->has_sec_mnt_opts = 1;
>>> +
>>>       switch (opt) {
>>>       case Opt_source:
>>>               if (fc->source)
>>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>>> index 62d3189745cd..08f4f34e8cf5 100644
>>> --- a/fs/nfs/internal.h
>>> +++ b/fs/nfs/internal.h
>>> @@ -96,6 +96,7 @@ struct nfs_fs_context {
>>>       char                    *fscache_uniq;
>>>       unsigned short          protofamily;
>>>       unsigned short          mountfamily;
>>> +     bool                    has_sec_mnt_opts;
>>>
>>>       struct {
>>>               union {
>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>> index 4034102010f0..0a2d252cf90f 100644
>>> --- a/fs/nfs/super.c
>>> +++ b/fs/nfs/super.c
>>> @@ -1058,6 +1058,7 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
>>>                                                &sb->s_blocksize_bits);
>>>
>>>       nfs_super_set_maxbytes(sb, server->maxfilesize);
>>> +     server->has_sec_mnt_opts = ctx->has_sec_mnt_opts;
>>>  }
>>>
>>>  static int nfs_compare_mount_options(const struct super_block *s, const struct nfs_server *b,
>>> @@ -1174,6 +1175,9 @@ static int nfs_compare_super(struct super_block *sb, struct fs_context *fc)
>>>               return 0;
>>>       if (!nfs_compare_userns(old, server))
>>>               return 0;
>>> +     if ((old->has_sec_mnt_opts || fc->security) &&
>>> +                     security_sb_mnt_opts_compat(sb, fc->security))
>>> +             return 0;
>>>       return nfs_compare_mount_options(sb, server, fc);
>>>  }
>>>
>>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
>>> index 38e60ec742df..3f0acada5794 100644
>>> --- a/include/linux/nfs_fs_sb.h
>>> +++ b/include/linux/nfs_fs_sb.h
>>> @@ -254,6 +254,7 @@ struct nfs_server {
>>>
>>>       /* User namespace info */
>>>       const struct cred       *cred;
>>> +     bool                    has_sec_mnt_opts;
>>>  };
>>>
>>>  /* Server capabilities */
Dan Carpenter Feb. 19, 2021, 8:19 a.m. UTC | #4
Hi Olga,

url:    https://github.com/0day-ci/linux/commits/Olga-Kornievskaia/Add-new-hook-to-compare-new-mount-to-an-existing-mount/20210219-035957
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: i386-randconfig-m021-20210215 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
fs/nfs/super.c:1061 nfs_fill_super() error: we previously assumed 'ctx' could be null (see line 1029)

vim +/ctx +1061 fs/nfs/super.c

62a55d088cd87d Scott Mayhew      2019-12-10  1021  static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
f7b422b17ee5ee David Howells     2006-06-09  1022  {
54ceac45159860 David Howells     2006-08-22  1023  	struct nfs_server *server = NFS_SB(sb);
f7b422b17ee5ee David Howells     2006-06-09  1024  
f7b422b17ee5ee David Howells     2006-06-09  1025  	sb->s_blocksize_bits = 0;
f7b422b17ee5ee David Howells     2006-06-09  1026  	sb->s_blocksize = 0;
6a74490dca8974 Bryan Schumaker   2012-07-30  1027  	sb->s_xattr = server->nfs_client->cl_nfs_mod->xattr;
6a74490dca8974 Bryan Schumaker   2012-07-30  1028  	sb->s_op = server->nfs_client->cl_nfs_mod->sops;
5eb005caf5383d David Howells     2019-12-10 @1029  	if (ctx && ctx->bsize)
                                                            ^^^
Check for NULL

5eb005caf5383d David Howells     2019-12-10  1030  		sb->s_blocksize = nfs_block_size(ctx->bsize, &sb->s_blocksize_bits);
f7b422b17ee5ee David Howells     2006-06-09  1031  
6a74490dca8974 Bryan Schumaker   2012-07-30  1032  	if (server->nfs_client->rpc_ops->version != 2) {
54ceac45159860 David Howells     2006-08-22  1033  		/* The VFS shouldn't apply the umask to mode bits. We will do
54ceac45159860 David Howells     2006-08-22  1034  		 * so ourselves when necessary.
54ceac45159860 David Howells     2006-08-22  1035  		 */
1751e8a6cb935e Linus Torvalds    2017-11-27  1036  		sb->s_flags |= SB_POSIXACL;
54ceac45159860 David Howells     2006-08-22  1037  		sb->s_time_gran = 1;
20fa1902728698 Peng Tao          2017-06-29  1038  		sb->s_export_op = &nfs_export_ops;
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1039  	} else
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1040  		sb->s_time_gran = 1000;
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1041  
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1042  	if (server->nfs_client->rpc_ops->version != 4) {
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1043  		sb->s_time_min = 0;
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1044  		sb->s_time_max = U32_MAX;
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1045  	} else {
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1046  		sb->s_time_min = S64_MIN;
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1047  		sb->s_time_max = S64_MAX;
54ceac45159860 David Howells     2006-08-22  1048  	}
f7b422b17ee5ee David Howells     2006-06-09  1049  
ab88dca311a372 Al Viro           2019-12-10  1050  	sb->s_magic = NFS_SUPER_MAGIC;
54ceac45159860 David Howells     2006-08-22  1051  
ab88dca311a372 Al Viro           2019-12-10  1052  	/* We probably want something more informative here */
ab88dca311a372 Al Viro           2019-12-10  1053  	snprintf(sb->s_id, sizeof(sb->s_id),
ab88dca311a372 Al Viro           2019-12-10  1054  		 "%u:%u", MAJOR(sb->s_dev), MINOR(sb->s_dev));
1fcb79c1b21801 Deepa Dinamani    2019-03-26  1055  
ab88dca311a372 Al Viro           2019-12-10  1056  	if (sb->s_blocksize == 0)
ab88dca311a372 Al Viro           2019-12-10  1057  		sb->s_blocksize = nfs_block_bits(server->wsize,
ab88dca311a372 Al Viro           2019-12-10  1058  						 &sb->s_blocksize_bits);
f7b422b17ee5ee David Howells     2006-06-09  1059  
ab88dca311a372 Al Viro           2019-12-10  1060  	nfs_super_set_maxbytes(sb, server->maxfilesize);
52a2a3a4af9af7 Olga Kornievskaia 2021-02-18 @1061  	server->has_sec_mnt_opts = ctx->has_sec_mnt_opts;
                                                                                   ^^^^^^^^^^^^^^^^^^^^^
Unchecked dereference.  Is the earlier NULL check necessary?  (Actually
on my system with a built cross function DB, I see that the earlier
NULL check can be removed.  If the cross function DB were built then
Smatch would not have printed this warning about inconsistent NULL
checks).

f7b422b17ee5ee David Howells     2006-06-09  1062  }
f7b422b17ee5ee David Howells     2006-06-09  1063  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Olga Kornievskaia Feb. 19, 2021, 5:11 p.m. UTC | #5
On Thu, Feb 18, 2021 at 6:17 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 2/18/2021 2:39 PM, Olga Kornievskaia wrote:
> > On Thu, Feb 18, 2021 at 5:07 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> >>> From: Olga Kornievskaia <kolga@netapp.com>
> >>>
> >>> Keep track of whether or not there was an selinux context mount
> >>> options during the mount.
> >> This may be the intention, but it's not what the change you're
> >> introducing here does.
> > Can you explain why, as I must be doing something wrong? I'm familiar
> > with NFS but not with Selinux and I thought I was doing the correct
> > changes to the Selinux. If that's not the case would you share what
> > has been done incorrectly.
>
> The code in this patch is generic for any LSM that wants to provide
> a security_sb_mnt_opts_compat() hook. Your 1/2 patch deals with how
> the hook provided by SELinux behaves.  All the behavior that is specific
> to SELinux should be in the SELinux hook. If you can state the behavior
> generically you should do that here.

Hopefully by removing specific reference to the selinux and sticking
with LSM addresses your comment. To NFS it's a security context it
doesn't care if it's selinux or something else.

> >>>  While deciding if the superblock can be
> >>> shared for the new mount, check for if we had selinux context on
> >>> the existing mount and call into selinux to tell if new passed
> >>> in selinux context is compatible with the existing mount's options.
> >> You're describing how you expect the change to be used, not
> >> what it does. If I am the author of a security module other
> >> than SELinux (which, it turns out, I am) what would I use this
> >> for? How might this change interact with my security module?
> >> Is this something I might exploit? If I am the author of a
> >> filesystem other than NFS (which I am not) should I be doing
> >> the same thing?
> > I'm not sure I'm understanding your questions. I'm solving a bug that
> > exists when NFS interacts with selinux.
>
> Right, but you're introducing an LSM interface to do so. LSM interfaces
> are expected to be usable by any security module. Smack ought to be able
> to provide NFS support, so might be expected to provide a hook for
> security_sb_mnt_opts_compat().

So I thought to address how a filesystem uses the hooks should have
been in the first patch and I added it here. But addressing how
another LSM module (like Smack) would use it again should be coming
from the first patch. It's a new hook to compare mount options and if
Smack were to implement some security mount options it would implement
a comparison function of the two.


>
> >  This is really not a feature
> > that I'm introducing. I thought it was somewhat descriptive with an
> > example that if you want to mount with different security contexts but
> > whatever you are mounting has a possibility of sharing the same
> > superblock, we need to be careful and take security contexts that are
> > being used by those mount into account to decide whether or not to
> > share the superblock. Again I thought that's exactly what the commit
> > states. A different security module, if it acts on security context,
> > would have to have the same ability to compare if one context options
> > are compatible with anothers.
>
> Not everyone is going to extrapolate the general behavior from
> the SELinux behavior. Your description of the SELinux behavior in 1/2
> is fine. I'm looking for how a module other than SELinux would use
> this.
>
> >  Another filesystem can decide if it
> > wants to share superblocks based on compatibility of existing security
> > context and new one. Is that what you are asking?
>
> What I'm asking is whether this should be a concern for filesystems
> in general, in which case this isn't the right place to make this change.
>
> >
> >
> >>> Previously, NFS wasn't able to do the following 2mounts:
> >>> mount -o vers=4.2,sec=sys,context=system_u:object_r:root_t:s0
> >>> <serverip>:/ /mnt
> >>> mount -o vers=4.2,sec=sys,context=system_u:object_r:swapfile_t:s0
> >>> <serverip>:/scratch /scratch
> >>>
> >>> 2nd mount would fail with "mount.nfs: an incorrect mount option was
> >>> specified" and var log messages would have:
> >>> "SElinux: mount invalid. Same superblock, different security
> >>> settings for.."
> >>>
> >>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >>> ---
> >>>  fs/nfs/fs_context.c       | 3 +++
> >>>  fs/nfs/internal.h         | 1 +
> >>>  fs/nfs/super.c            | 4 ++++
> >>>  include/linux/nfs_fs_sb.h | 1 +
> >>>  4 files changed, 9 insertions(+)
> >>>
> >>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> >>> index 06894bcdea2d..8067f055d842 100644
> >>> --- a/fs/nfs/fs_context.c
> >>> +++ b/fs/nfs/fs_context.c
> >>> @@ -448,6 +448,9 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
> >>>       if (opt < 0)
> >>>               return ctx->sloppy ? 1 : opt;
> >>>
> >>> +     if (fc->security)
> >>> +             ctx->has_sec_mnt_opts = 1;
> >>> +
> >>>       switch (opt) {
> >>>       case Opt_source:
> >>>               if (fc->source)
> >>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> >>> index 62d3189745cd..08f4f34e8cf5 100644
> >>> --- a/fs/nfs/internal.h
> >>> +++ b/fs/nfs/internal.h
> >>> @@ -96,6 +96,7 @@ struct nfs_fs_context {
> >>>       char                    *fscache_uniq;
> >>>       unsigned short          protofamily;
> >>>       unsigned short          mountfamily;
> >>> +     bool                    has_sec_mnt_opts;
> >>>
> >>>       struct {
> >>>               union {
> >>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> >>> index 4034102010f0..0a2d252cf90f 100644
> >>> --- a/fs/nfs/super.c
> >>> +++ b/fs/nfs/super.c
> >>> @@ -1058,6 +1058,7 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
> >>>                                                &sb->s_blocksize_bits);
> >>>
> >>>       nfs_super_set_maxbytes(sb, server->maxfilesize);
> >>> +     server->has_sec_mnt_opts = ctx->has_sec_mnt_opts;
> >>>  }
> >>>
> >>>  static int nfs_compare_mount_options(const struct super_block *s, const struct nfs_server *b,
> >>> @@ -1174,6 +1175,9 @@ static int nfs_compare_super(struct super_block *sb, struct fs_context *fc)
> >>>               return 0;
> >>>       if (!nfs_compare_userns(old, server))
> >>>               return 0;
> >>> +     if ((old->has_sec_mnt_opts || fc->security) &&
> >>> +                     security_sb_mnt_opts_compat(sb, fc->security))
> >>> +             return 0;
> >>>       return nfs_compare_mount_options(sb, server, fc);
> >>>  }
> >>>
> >>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> >>> index 38e60ec742df..3f0acada5794 100644
> >>> --- a/include/linux/nfs_fs_sb.h
> >>> +++ b/include/linux/nfs_fs_sb.h
> >>> @@ -254,6 +254,7 @@ struct nfs_server {
> >>>
> >>>       /* User namespace info */
> >>>       const struct cred       *cred;
> >>> +     bool                    has_sec_mnt_opts;
> >>>  };
> >>>
> >>>  /* Server capabilities */
>
Olga Kornievskaia Feb. 19, 2021, 5:20 p.m. UTC | #6
Trond/Anna,

I'd like your opinion here. Some static checking flags a "ctx"
assignment in nfs_fill_super() in the new patch. In an existing code
there is a check for it is NULL before dereferencing. However, "ctx"
can never be null. nfs_get_tree_common() which calls nfs_fill_super()
and passes in "ctx" gets it from the passed in "fs_context". If the
passed in arg can be null then we are dereferencing in var assignment
so things would blow up there. So "ctx" can never be null.

Should I create another clean up patch to remove the check for null
ctx in nfs_fill_super() to make static analyzers happy?

On Fri, Feb 19, 2021 at 3:19 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hi Olga,
>
> url:    https://github.com/0day-ci/linux/commits/Olga-Kornievskaia/Add-new-hook-to-compare-new-mount-to-an-existing-mount/20210219-035957
> base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
> config: i386-randconfig-m021-20210215 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> fs/nfs/super.c:1061 nfs_fill_super() error: we previously assumed 'ctx' could be null (see line 1029)
>
> vim +/ctx +1061 fs/nfs/super.c
>
> 62a55d088cd87d Scott Mayhew      2019-12-10  1021  static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
> f7b422b17ee5ee David Howells     2006-06-09  1022  {
> 54ceac45159860 David Howells     2006-08-22  1023       struct nfs_server *server = NFS_SB(sb);
> f7b422b17ee5ee David Howells     2006-06-09  1024
> f7b422b17ee5ee David Howells     2006-06-09  1025       sb->s_blocksize_bits = 0;
> f7b422b17ee5ee David Howells     2006-06-09  1026       sb->s_blocksize = 0;
> 6a74490dca8974 Bryan Schumaker   2012-07-30  1027       sb->s_xattr = server->nfs_client->cl_nfs_mod->xattr;
> 6a74490dca8974 Bryan Schumaker   2012-07-30  1028       sb->s_op = server->nfs_client->cl_nfs_mod->sops;
> 5eb005caf5383d David Howells     2019-12-10 @1029       if (ctx && ctx->bsize)
>                                                             ^^^
> Check for NULL
>
> 5eb005caf5383d David Howells     2019-12-10  1030               sb->s_blocksize = nfs_block_size(ctx->bsize, &sb->s_blocksize_bits);
> f7b422b17ee5ee David Howells     2006-06-09  1031
> 6a74490dca8974 Bryan Schumaker   2012-07-30  1032       if (server->nfs_client->rpc_ops->version != 2) {
> 54ceac45159860 David Howells     2006-08-22  1033               /* The VFS shouldn't apply the umask to mode bits. We will do
> 54ceac45159860 David Howells     2006-08-22  1034                * so ourselves when necessary.
> 54ceac45159860 David Howells     2006-08-22  1035                */
> 1751e8a6cb935e Linus Torvalds    2017-11-27  1036               sb->s_flags |= SB_POSIXACL;
> 54ceac45159860 David Howells     2006-08-22  1037               sb->s_time_gran = 1;
> 20fa1902728698 Peng Tao          2017-06-29  1038               sb->s_export_op = &nfs_export_ops;
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1039       } else
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1040               sb->s_time_gran = 1000;
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1041
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1042       if (server->nfs_client->rpc_ops->version != 4) {
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1043               sb->s_time_min = 0;
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1044               sb->s_time_max = U32_MAX;
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1045       } else {
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1046               sb->s_time_min = S64_MIN;
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1047               sb->s_time_max = S64_MAX;
> 54ceac45159860 David Howells     2006-08-22  1048       }
> f7b422b17ee5ee David Howells     2006-06-09  1049
> ab88dca311a372 Al Viro           2019-12-10  1050       sb->s_magic = NFS_SUPER_MAGIC;
> 54ceac45159860 David Howells     2006-08-22  1051
> ab88dca311a372 Al Viro           2019-12-10  1052       /* We probably want something more informative here */
> ab88dca311a372 Al Viro           2019-12-10  1053       snprintf(sb->s_id, sizeof(sb->s_id),
> ab88dca311a372 Al Viro           2019-12-10  1054                "%u:%u", MAJOR(sb->s_dev), MINOR(sb->s_dev));
> 1fcb79c1b21801 Deepa Dinamani    2019-03-26  1055
> ab88dca311a372 Al Viro           2019-12-10  1056       if (sb->s_blocksize == 0)
> ab88dca311a372 Al Viro           2019-12-10  1057               sb->s_blocksize = nfs_block_bits(server->wsize,
> ab88dca311a372 Al Viro           2019-12-10  1058                                                &sb->s_blocksize_bits);
> f7b422b17ee5ee David Howells     2006-06-09  1059
> ab88dca311a372 Al Viro           2019-12-10  1060       nfs_super_set_maxbytes(sb, server->maxfilesize);
> 52a2a3a4af9af7 Olga Kornievskaia 2021-02-18 @1061       server->has_sec_mnt_opts = ctx->has_sec_mnt_opts;
>                                                                                    ^^^^^^^^^^^^^^^^^^^^^
> Unchecked dereference.  Is the earlier NULL check necessary?  (Actually
> on my system with a built cross function DB, I see that the earlier
> NULL check can be removed.  If the cross function DB were built then
> Smatch would not have printed this warning about inconsistent NULL
> checks).
>
> f7b422b17ee5ee David Howells     2006-06-09  1062  }
> f7b422b17ee5ee David Howells     2006-06-09  1063
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Trond Myklebust Feb. 19, 2021, 8:07 p.m. UTC | #7
On Fri, 2021-02-19 at 12:20 -0500, Olga Kornievskaia wrote:
> Trond/Anna,
> 
> I'd like your opinion here. Some static checking flags a "ctx"
> assignment in nfs_fill_super() in the new patch. In an existing code
> there is a check for it is NULL before dereferencing. However, "ctx"
> can never be null. nfs_get_tree_common() which calls nfs_fill_super()
> and passes in "ctx" gets it from the passed in "fs_context". If the
> passed in arg can be null then we are dereferencing in var assignment
> so things would blow up there. So "ctx" can never be null.
> 
> Should I create another clean up patch to remove the check for null
> ctx in nfs_fill_super() to make static analyzers happy?
> 

Yes, at this point, nfs_fill_super() is only called from
nfs_get_tree_common(), which would crash and burn well before if ctx
were an invalid pointer.

So please go ahead, and remove the check for ctx being NULL in
nfs_fill_super().
diff mbox series

Patch

diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 06894bcdea2d..8067f055d842 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -448,6 +448,9 @@  static int nfs_fs_context_parse_param(struct fs_context *fc,
 	if (opt < 0)
 		return ctx->sloppy ? 1 : opt;
 
+	if (fc->security)
+		ctx->has_sec_mnt_opts = 1;
+
 	switch (opt) {
 	case Opt_source:
 		if (fc->source)
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 62d3189745cd..08f4f34e8cf5 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -96,6 +96,7 @@  struct nfs_fs_context {
 	char			*fscache_uniq;
 	unsigned short		protofamily;
 	unsigned short		mountfamily;
+	bool			has_sec_mnt_opts;
 
 	struct {
 		union {
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 4034102010f0..0a2d252cf90f 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1058,6 +1058,7 @@  static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
 						 &sb->s_blocksize_bits);
 
 	nfs_super_set_maxbytes(sb, server->maxfilesize);
+	server->has_sec_mnt_opts = ctx->has_sec_mnt_opts;
 }
 
 static int nfs_compare_mount_options(const struct super_block *s, const struct nfs_server *b,
@@ -1174,6 +1175,9 @@  static int nfs_compare_super(struct super_block *sb, struct fs_context *fc)
 		return 0;
 	if (!nfs_compare_userns(old, server))
 		return 0;
+	if ((old->has_sec_mnt_opts || fc->security) &&
+			security_sb_mnt_opts_compat(sb, fc->security))
+		return 0;
 	return nfs_compare_mount_options(sb, server, fc);
 }
 
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index 38e60ec742df..3f0acada5794 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -254,6 +254,7 @@  struct nfs_server {
 
 	/* User namespace info */
 	const struct cred	*cred;
+	bool			has_sec_mnt_opts;
 };
 
 /* Server capabilities */