[PATCH v2 1/2] [security] Add new hook to compare new mount to an existing mount

[PATCH v2 1/2] [security] Add new hook to compare new mount to an existing mount

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Add a new hook that takes an existing super block and a new mount
with new options and determines if new options confict with an
existing mount or not.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
`
---
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/lsm_hooks.h     |  6 ++++
 include/linux/security.h      |  8 ++++++
 security/security.c           |  7 +++++
 security/selinux/hooks.c      | 54 +++++++++++++++++++++++++++++++++++
 5 files changed, 76 insertions(+)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 7aaa753b8608..1b12a5266a51 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -62,6 +62,7 @@ LSM_HOOK(int, 0, sb_alloc_security, struct super_block *sb)
 LSM_HOOK(void, LSM_RET_VOID, sb_free_security, struct super_block *sb)
 LSM_HOOK(void, LSM_RET_VOID, sb_free_mnt_opts, void *mnt_opts)
 LSM_HOOK(int, 0, sb_eat_lsm_opts, char *orig, void **mnt_opts)
+LSM_HOOK(int, 0, sb_mnt_opts_compat, struct super_block *sb, void *mnt_opts)
 LSM_HOOK(int, 0, sb_remount, struct super_block *sb, void *mnt_opts)
 LSM_HOOK(int, 0, sb_kern_mount, struct super_block *sb)
 LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index a19adef1f088..77c1e9cdeaca 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -142,6 +142,12 @@
  *	@orig the original mount data copied from userspace.
  *	@copy copied data which will be passed to the security module.
  *	Returns 0 if the copy was successful.
+ * @sb_mnt_opts_compat:
+ *	Determine if the existing mount options are compatible with the new
+ *	mount options being used.
+ *	@sb superblock being compared
+ *	@mnt_opts new mount options
+ *	Return 0 if options are the same.
  * @sb_remount:
  *	Extracts security system specific mount options and verifies no changes
  *	are being made to those options.
diff --git a/include/linux/security.h b/include/linux/security.h
index c35ea0ffccd9..50db3d5d1608 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -291,6 +291,7 @@ int security_sb_alloc(struct super_block *sb);
 void security_sb_free(struct super_block *sb);
 void security_free_mnt_opts(void **mnt_opts);
 int security_sb_eat_lsm_opts(char *options, void **mnt_opts);
+int security_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts);
 int security_sb_remount(struct super_block *sb, void *mnt_opts);
 int security_sb_kern_mount(struct super_block *sb);
 int security_sb_show_options(struct seq_file *m, struct super_block *sb);
@@ -635,6 +636,13 @@ static inline int security_sb_remount(struct super_block *sb,
 	return 0;
 }
 
+static inline int security_sb_mnt_opts_compat(struct super_block *sb,
+					      void *mnt_opts)
+{
+	return 0;
+}
+
+
 static inline int security_sb_kern_mount(struct super_block *sb)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 7b09cfbae94f..56cf5563efde 100644
--- a/security/security.c
+++ b/security/security.c
@@ -890,6 +890,13 @@ int security_sb_eat_lsm_opts(char *options, void **mnt_opts)
 }
 EXPORT_SYMBOL(security_sb_eat_lsm_opts);
 
+int security_sb_mnt_opts_compat(struct super_block *sb,
+				void *mnt_opts)
+{
+	return call_int_hook(sb_mnt_opts_compat, 0, sb, mnt_opts);
+}
+EXPORT_SYMBOL(security_sb_mnt_opts_compat);
+
 int security_sb_remount(struct super_block *sb,
 			void *mnt_opts)
 {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 644b17ec9e63..f0b8ebc1e2c2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2656,6 +2656,59 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
 	return rc;
 }
 
+static int selinux_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts)
+{
+	struct selinux_mnt_opts *opts = mnt_opts;
+	struct superblock_security_struct *sbsec = sb->s_security;
+	u32 sid;
+	int rc;
+
+	/* superblock not initialized (i.e. no options) - reject if any
+	 * options specified, otherwise accept
+	 */
+	if (!(sbsec->flags & SE_SBINITIALIZED))
+		return opts ? 1 : 0;
+
+	/* superblock initialized and no options specified - reject if
+	 * superblock has any options set, otherwise accept
+	 */
+	if (!opts)
+		return (sbsec->flags & SE_MNTMASK) ? 1 : 0;
+
+	if (opts->fscontext) {
+		rc = parse_sid(sb, opts->fscontext, &sid);
+		if (rc)
+			return 1;
+		if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid))
+			return 1;
+	}
+	if (opts->context) {
+		rc = parse_sid(sb, opts->context, &sid);
+		if (rc)
+			return 1;
+		if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid))
+			return 1;
+	}
+	if (opts->rootcontext) {
+		struct inode_security_struct *root_isec;
+
+		root_isec = backing_inode_security(sb->s_root);
+		rc = parse_sid(sb, opts->rootcontext, &sid);
+		if (rc)
+			return 1;
+		if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
+			return 1;
+	}
+	if (opts->defcontext) {
+		rc = parse_sid(sb, opts->defcontext, &sid);
+		if (rc)
+			return 1;
+		if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
+			return 1;
+	}
+	return 0;
+}
+
 static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
 {
 	struct selinux_mnt_opts *opts = mnt_opts;
@@ -6984,6 +7037,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 
 	LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
 	LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
+	LSM_HOOK_INIT(sb_mnt_opts_compat, selinux_sb_mnt_opts_compat),
 	LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
 	LSM_HOOK_INIT(sb_kern_mount, selinux_sb_kern_mount),
 	LSM_HOOK_INIT(sb_show_options, selinux_sb_show_options),
-- 
2.27.0

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

[Olga Kornievskaia]

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(+)

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 */
-- 
2.27.0

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

[Casey Schaufler]

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

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

[Olga Kornievskaia]

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

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

[Casey Schaufler]

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

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

[Olga Kornievskaia]

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

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 4628 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38051 bytes --]

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 4703 bytes --]

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(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38051 bytes --]

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 4703 bytes --]

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(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38051 bytes --]

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

[Olga Kornievskaia]

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

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

[Trond Myklebust]

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().

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v2 1/2] [security] Add new hook to compare new mount to an existing mount

[Casey Schaufler]

On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
>
> Add a new hook that takes an existing super block and a new mount
> with new options and determines if new options confict with an
> existing mount or not.
>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> `
> ---
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/lsm_hooks.h     |  6 ++++
>  include/linux/security.h      |  8 ++++++
>  security/security.c           |  7 +++++
>  security/selinux/hooks.c      | 54 +++++++++++++++++++++++++++++++++++
>  5 files changed, 76 insertions(+)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 7aaa753b8608..1b12a5266a51 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -62,6 +62,7 @@ LSM_HOOK(int, 0, sb_alloc_security, struct super_block *sb)
>  LSM_HOOK(void, LSM_RET_VOID, sb_free_security, struct super_block *sb)
>  LSM_HOOK(void, LSM_RET_VOID, sb_free_mnt_opts, void *mnt_opts)
>  LSM_HOOK(int, 0, sb_eat_lsm_opts, char *orig, void **mnt_opts)
> +LSM_HOOK(int, 0, sb_mnt_opts_compat, struct super_block *sb, void *mnt_opts)
>  LSM_HOOK(int, 0, sb_remount, struct super_block *sb, void *mnt_opts)
>  LSM_HOOK(int, 0, sb_kern_mount, struct super_block *sb)
>  LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index a19adef1f088..77c1e9cdeaca 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -142,6 +142,12 @@
>   *	@orig the original mount data copied from userspace.
>   *	@copy copied data which will be passed to the security module.
>   *	Returns 0 if the copy was successful.
> + * @sb_mnt_opts_compat:
> + *	Determine if the existing mount options are compatible with the new
> + *	mount options being used.
> + *	@sb superblock being compared
> + *	@mnt_opts new mount options
> + *	Return 0 if options are the same.

s/the same/compatible/

>   * @sb_remount:
>   *	Extracts security system specific mount options and verifies no changes
>   *	are being made to those options.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c35ea0ffccd9..50db3d5d1608 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -291,6 +291,7 @@ int security_sb_alloc(struct super_block *sb);
>  void security_sb_free(struct super_block *sb);
>  void security_free_mnt_opts(void **mnt_opts);
>  int security_sb_eat_lsm_opts(char *options, void **mnt_opts);
> +int security_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts);
>  int security_sb_remount(struct super_block *sb, void *mnt_opts);
>  int security_sb_kern_mount(struct super_block *sb);
>  int security_sb_show_options(struct seq_file *m, struct super_block *sb);
> @@ -635,6 +636,13 @@ static inline int security_sb_remount(struct super_block *sb,
>  	return 0;
>  }
>  
> +static inline int security_sb_mnt_opts_compat(struct super_block *sb,
> +					      void *mnt_opts)
> +{
> +	return 0;
> +}
> +
> +
>  static inline int security_sb_kern_mount(struct super_block *sb)
>  {
>  	return 0;
> diff --git a/security/security.c b/security/security.c
> index 7b09cfbae94f..56cf5563efde 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -890,6 +890,13 @@ int security_sb_eat_lsm_opts(char *options, void **mnt_opts)
>  }
>  EXPORT_SYMBOL(security_sb_eat_lsm_opts);
>  
> +int security_sb_mnt_opts_compat(struct super_block *sb,
> +				void *mnt_opts)
> +{
> +	return call_int_hook(sb_mnt_opts_compat, 0, sb, mnt_opts);
> +}
> +EXPORT_SYMBOL(security_sb_mnt_opts_compat);
> +
>  int security_sb_remount(struct super_block *sb,
>  			void *mnt_opts)
>  {
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 644b17ec9e63..f0b8ebc1e2c2 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2656,6 +2656,59 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>  	return rc;
>  }
>  
> +static int selinux_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts)
> +{
> +	struct selinux_mnt_opts *opts = mnt_opts;
> +	struct superblock_security_struct *sbsec = sb->s_security;
> +	u32 sid;
> +	int rc;
> +
> +	/* superblock not initialized (i.e. no options) - reject if any

Please maintain the existing comment style used in this file.

	/*
	 * superblock not initialized (i.e. no options) - reject if any

> +	 * options specified, otherwise accept
> +	 */
> +	if (!(sbsec->flags & SE_SBINITIALIZED))
> +		return opts ? 1 : 0;
> +
> +	/* superblock initialized and no options specified - reject if
> +	 * superblock has any options set, otherwise accept
> +	 */
> +	if (!opts)
> +		return (sbsec->flags & SE_MNTMASK) ? 1 : 0;
> +
> +	if (opts->fscontext) {
> +		rc = parse_sid(sb, opts->fscontext, &sid);
> +		if (rc)
> +			return 1;
> +		if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid))
> +			return 1;
> +	}
> +	if (opts->context) {
> +		rc = parse_sid(sb, opts->context, &sid);
> +		if (rc)
> +			return 1;
> +		if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid))
> +			return 1;
> +	}
> +	if (opts->rootcontext) {
> +		struct inode_security_struct *root_isec;
> +
> +		root_isec = backing_inode_security(sb->s_root);
> +		rc = parse_sid(sb, opts->rootcontext, &sid);
> +		if (rc)
> +			return 1;
> +		if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
> +			return 1;
> +	}
> +	if (opts->defcontext) {
> +		rc = parse_sid(sb, opts->defcontext, &sid);
> +		if (rc)
> +			return 1;
> +		if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
> +			return 1;
> +	}
> +	return 0;
> +}
> +
>  static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
>  {
>  	struct selinux_mnt_opts *opts = mnt_opts;
> @@ -6984,6 +7037,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>  
>  	LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
>  	LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
> +	LSM_HOOK_INIT(sb_mnt_opts_compat, selinux_sb_mnt_opts_compat),
>  	LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
>  	LSM_HOOK_INIT(sb_kern_mount, selinux_sb_kern_mount),
>  	LSM_HOOK_INIT(sb_show_options, selinux_sb_show_options),

Re: [PATCH v2 1/2] [security] Add new hook to compare new mount to an existing mount

[Olga Kornievskaia]

On Thu, Feb 18, 2021 at 4:57 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Add a new hook that takes an existing super block and a new mount
> > with new options and determines if new options confict with an
> > existing mount or not.
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > `
> > ---
> >  include/linux/lsm_hook_defs.h |  1 +
> >  include/linux/lsm_hooks.h     |  6 ++++
> >  include/linux/security.h      |  8 ++++++
> >  security/security.c           |  7 +++++
> >  security/selinux/hooks.c      | 54 +++++++++++++++++++++++++++++++++++
> >  5 files changed, 76 insertions(+)
> >
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 7aaa753b8608..1b12a5266a51 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -62,6 +62,7 @@ LSM_HOOK(int, 0, sb_alloc_security, struct super_block *sb)
> >  LSM_HOOK(void, LSM_RET_VOID, sb_free_security, struct super_block *sb)
> >  LSM_HOOK(void, LSM_RET_VOID, sb_free_mnt_opts, void *mnt_opts)
> >  LSM_HOOK(int, 0, sb_eat_lsm_opts, char *orig, void **mnt_opts)
> > +LSM_HOOK(int, 0, sb_mnt_opts_compat, struct super_block *sb, void *mnt_opts)
> >  LSM_HOOK(int, 0, sb_remount, struct super_block *sb, void *mnt_opts)
> >  LSM_HOOK(int, 0, sb_kern_mount, struct super_block *sb)
> >  LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index a19adef1f088..77c1e9cdeaca 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -142,6 +142,12 @@
> >   *   @orig the original mount data copied from userspace.
> >   *   @copy copied data which will be passed to the security module.
> >   *   Returns 0 if the copy was successful.
> > + * @sb_mnt_opts_compat:
> > + *   Determine if the existing mount options are compatible with the new
> > + *   mount options being used.
> > + *   @sb superblock being compared
> > + *   @mnt_opts new mount options
> > + *   Return 0 if options are the same.
>
> s/the same/compatible/
>
> >   * @sb_remount:
> >   *   Extracts security system specific mount options and verifies no changes
> >   *   are being made to those options.
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index c35ea0ffccd9..50db3d5d1608 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -291,6 +291,7 @@ int security_sb_alloc(struct super_block *sb);
> >  void security_sb_free(struct super_block *sb);
> >  void security_free_mnt_opts(void **mnt_opts);
> >  int security_sb_eat_lsm_opts(char *options, void **mnt_opts);
> > +int security_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts);
> >  int security_sb_remount(struct super_block *sb, void *mnt_opts);
> >  int security_sb_kern_mount(struct super_block *sb);
> >  int security_sb_show_options(struct seq_file *m, struct super_block *sb);
> > @@ -635,6 +636,13 @@ static inline int security_sb_remount(struct super_block *sb,
> >       return 0;
> >  }
> >
> > +static inline int security_sb_mnt_opts_compat(struct super_block *sb,
> > +                                           void *mnt_opts)
> > +{
> > +     return 0;
> > +}
> > +
> > +
> >  static inline int security_sb_kern_mount(struct super_block *sb)
> >  {
> >       return 0;
> > diff --git a/security/security.c b/security/security.c
> > index 7b09cfbae94f..56cf5563efde 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -890,6 +890,13 @@ int security_sb_eat_lsm_opts(char *options, void **mnt_opts)
> >  }
> >  EXPORT_SYMBOL(security_sb_eat_lsm_opts);
> >
> > +int security_sb_mnt_opts_compat(struct super_block *sb,
> > +                             void *mnt_opts)
> > +{
> > +     return call_int_hook(sb_mnt_opts_compat, 0, sb, mnt_opts);
> > +}
> > +EXPORT_SYMBOL(security_sb_mnt_opts_compat);
> > +
> >  int security_sb_remount(struct super_block *sb,
> >                       void *mnt_opts)
> >  {
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 644b17ec9e63..f0b8ebc1e2c2 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2656,6 +2656,59 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
> >       return rc;
> >  }
> >
> > +static int selinux_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts)
> > +{
> > +     struct selinux_mnt_opts *opts = mnt_opts;
> > +     struct superblock_security_struct *sbsec = sb->s_security;
> > +     u32 sid;
> > +     int rc;
> > +
> > +     /* superblock not initialized (i.e. no options) - reject if any
>
> Please maintain the existing comment style used in this file.

I'm again not sure what exactly is the style used in this file and how
is this inconsistent?

Here's one example from this file:
        /* Wake up the parent if it is waiting so that it can recheck
         * wait permission to the new task SID. */
this is another example from this file:
        /* Check whether the new SID can inherit signal state from the old SID.
         * If not, clear itimers to avoid subsequent signal generation and
         * flush and unblock signals.
         *
         * This must occur _after_ the task SID has been updated so that any
         * kill done after the flush will be checked against the new SID.
         */
Here's another:
                       /* strip quotes */

What exactly is not correct about the new comment?
        /* superblock not initialized (i.e. no options) - reject if any
         * options specified, otherwise accept
         */
It uses "/*" and has a space after. It starts each new line with "*"
and a space. And ends with a new line. This is consistent with the
general kernel style. Actually example 1 is not following kernel style
by not ending on the new line.

Is the objection that it's perhaps not a sentence that starts with a
capital letter and ends with a period? Not all comments are sentences.
If so please specify. Otherwise, I'm just guessing what you are
expecting.


>         /*
>          * superblock not initialized (i.e. no options) - reject if any
>
> > +      * options specified, otherwise accept
> > +      */
> > +     if (!(sbsec->flags & SE_SBINITIALIZED))
> > +             return opts ? 1 : 0;
> > +
> > +     /* superblock initialized and no options specified - reject if
> > +      * superblock has any options set, otherwise accept
> > +      */
> > +     if (!opts)
> > +             return (sbsec->flags & SE_MNTMASK) ? 1 : 0;
> > +
> > +     if (opts->fscontext) {
> > +             rc = parse_sid(sb, opts->fscontext, &sid);
> > +             if (rc)
> > +                     return 1;
> > +             if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid))
> > +                     return 1;
> > +     }
> > +     if (opts->context) {
> > +             rc = parse_sid(sb, opts->context, &sid);
> > +             if (rc)
> > +                     return 1;
> > +             if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid))
> > +                     return 1;
> > +     }
> > +     if (opts->rootcontext) {
> > +             struct inode_security_struct *root_isec;
> > +
> > +             root_isec = backing_inode_security(sb->s_root);
> > +             rc = parse_sid(sb, opts->rootcontext, &sid);
> > +             if (rc)
> > +                     return 1;
> > +             if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
> > +                     return 1;
> > +     }
> > +     if (opts->defcontext) {
> > +             rc = parse_sid(sb, opts->defcontext, &sid);
> > +             if (rc)
> > +                     return 1;
> > +             if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
> > +                     return 1;
> > +     }
> > +     return 0;
> > +}
> > +
> >  static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
> >  {
> >       struct selinux_mnt_opts *opts = mnt_opts;
> > @@ -6984,6 +7037,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> >
> >       LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
> >       LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
> > +     LSM_HOOK_INIT(sb_mnt_opts_compat, selinux_sb_mnt_opts_compat),
> >       LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
> >       LSM_HOOK_INIT(sb_kern_mount, selinux_sb_kern_mount),
> >       LSM_HOOK_INIT(sb_show_options, selinux_sb_show_options),

Re: [PATCH v2 1/2] [security] Add new hook to compare new mount to an existing mount

[Casey Schaufler]

On 2/19/2021 8:25 AM, Olga Kornievskaia wrote:
> On Thu, Feb 18, 2021 at 4:57 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>
>>> Add a new hook that takes an existing super block and a new mount
>>> with new options and determines if new options confict with an
>>> existing mount or not.
>>>
>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>> `
>>> ---
>>>  include/linux/lsm_hook_defs.h |  1 +
>>>  include/linux/lsm_hooks.h     |  6 ++++
>>>  include/linux/security.h      |  8 ++++++
>>>  security/security.c           |  7 +++++
>>>  security/selinux/hooks.c      | 54 +++++++++++++++++++++++++++++++++++
>>>  5 files changed, 76 insertions(+)
>>>
>>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>>> index 7aaa753b8608..1b12a5266a51 100644
>>> --- a/include/linux/lsm_hook_defs.h
>>> +++ b/include/linux/lsm_hook_defs.h
>>> @@ -62,6 +62,7 @@ LSM_HOOK(int, 0, sb_alloc_security, struct super_block *sb)
>>>  LSM_HOOK(void, LSM_RET_VOID, sb_free_security, struct super_block *sb)
>>>  LSM_HOOK(void, LSM_RET_VOID, sb_free_mnt_opts, void *mnt_opts)
>>>  LSM_HOOK(int, 0, sb_eat_lsm_opts, char *orig, void **mnt_opts)
>>> +LSM_HOOK(int, 0, sb_mnt_opts_compat, struct super_block *sb, void *mnt_opts)
>>>  LSM_HOOK(int, 0, sb_remount, struct super_block *sb, void *mnt_opts)
>>>  LSM_HOOK(int, 0, sb_kern_mount, struct super_block *sb)
>>>  LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index a19adef1f088..77c1e9cdeaca 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -142,6 +142,12 @@
>>>   *   @orig the original mount data copied from userspace.
>>>   *   @copy copied data which will be passed to the security module.
>>>   *   Returns 0 if the copy was successful.
>>> + * @sb_mnt_opts_compat:
>>> + *   Determine if the existing mount options are compatible with the new
>>> + *   mount options being used.
>>> + *   @sb superblock being compared
>>> + *   @mnt_opts new mount options
>>> + *   Return 0 if options are the same.
>> s/the same/compatible/
>>
>>>   * @sb_remount:
>>>   *   Extracts security system specific mount options and verifies no changes
>>>   *   are being made to those options.
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index c35ea0ffccd9..50db3d5d1608 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -291,6 +291,7 @@ int security_sb_alloc(struct super_block *sb);
>>>  void security_sb_free(struct super_block *sb);
>>>  void security_free_mnt_opts(void **mnt_opts);
>>>  int security_sb_eat_lsm_opts(char *options, void **mnt_opts);
>>> +int security_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts);
>>>  int security_sb_remount(struct super_block *sb, void *mnt_opts);
>>>  int security_sb_kern_mount(struct super_block *sb);
>>>  int security_sb_show_options(struct seq_file *m, struct super_block *sb);
>>> @@ -635,6 +636,13 @@ static inline int security_sb_remount(struct super_block *sb,
>>>       return 0;
>>>  }
>>>
>>> +static inline int security_sb_mnt_opts_compat(struct super_block *sb,
>>> +                                           void *mnt_opts)
>>> +{
>>> +     return 0;
>>> +}
>>> +
>>> +
>>>  static inline int security_sb_kern_mount(struct super_block *sb)
>>>  {
>>>       return 0;
>>> diff --git a/security/security.c b/security/security.c
>>> index 7b09cfbae94f..56cf5563efde 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -890,6 +890,13 @@ int security_sb_eat_lsm_opts(char *options, void **mnt_opts)
>>>  }
>>>  EXPORT_SYMBOL(security_sb_eat_lsm_opts);
>>>
>>> +int security_sb_mnt_opts_compat(struct super_block *sb,
>>> +                             void *mnt_opts)
>>> +{
>>> +     return call_int_hook(sb_mnt_opts_compat, 0, sb, mnt_opts);
>>> +}
>>> +EXPORT_SYMBOL(security_sb_mnt_opts_compat);
>>> +
>>>  int security_sb_remount(struct super_block *sb,
>>>                       void *mnt_opts)
>>>  {
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 644b17ec9e63..f0b8ebc1e2c2 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -2656,6 +2656,59 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>>>       return rc;
>>>  }
>>>
>>> +static int selinux_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts)
>>> +{
>>> +     struct selinux_mnt_opts *opts = mnt_opts;
>>> +     struct superblock_security_struct *sbsec = sb->s_security;
>>> +     u32 sid;
>>> +     int rc;
>>> +
>>> +     /* superblock not initialized (i.e. no options) - reject if any
>> Please maintain the existing comment style used in this file.
> I'm again not sure what exactly is the style used in this file and how
> is this inconsistent?

While not 100% consistent, the style used here has the "/*" alone on the first line
with the text starting on the second line.

	/*
	 * Like this
	 * is done
	 */

not

	/* Like this
	 * is done
	 */

>
> Here's one example from this file:
>         /* Wake up the parent if it is waiting so that it can recheck
>          * wait permission to the new task SID. */
> this is another example from this file:
>         /* Check whether the new SID can inherit signal state from the old SID.
>          * If not, clear itimers to avoid subsequent signal generation and
>          * flush and unblock signals.
>          *
>          * This must occur _after_ the task SID has been updated so that any
>          * kill done after the flush will be checked against the new SID.
>          */
> Here's another:
>                        /* strip quotes */
>
> What exactly is not correct about the new comment?
>         /* superblock not initialized (i.e. no options) - reject if any
>          * options specified, otherwise accept
>          */
> It uses "/*" and has a space after. It starts each new line with "*"
> and a space. And ends with a new line. This is consistent with the
> general kernel style. Actually example 1 is not following kernel style
> by not ending on the new line.
>
> Is the objection that it's perhaps not a sentence that starts with a
> capital letter and ends with a period? Not all comments are sentences.
> If so please specify. Otherwise, I'm just guessing what you are
> expecting.
>
>
>>         /*
>>          * superblock not initialized (i.e. no options) - reject if any
>>
>>> +      * options specified, otherwise accept
>>> +      */
>>> +     if (!(sbsec->flags & SE_SBINITIALIZED))
>>> +             return opts ? 1 : 0;
>>> +
>>> +     /* superblock initialized and no options specified - reject if
>>> +      * superblock has any options set, otherwise accept
>>> +      */
>>> +     if (!opts)
>>> +             return (sbsec->flags & SE_MNTMASK) ? 1 : 0;
>>> +
>>> +     if (opts->fscontext) {
>>> +             rc = parse_sid(sb, opts->fscontext, &sid);
>>> +             if (rc)
>>> +                     return 1;
>>> +             if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid))
>>> +                     return 1;
>>> +     }
>>> +     if (opts->context) {
>>> +             rc = parse_sid(sb, opts->context, &sid);
>>> +             if (rc)
>>> +                     return 1;
>>> +             if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid))
>>> +                     return 1;
>>> +     }
>>> +     if (opts->rootcontext) {
>>> +             struct inode_security_struct *root_isec;
>>> +
>>> +             root_isec = backing_inode_security(sb->s_root);
>>> +             rc = parse_sid(sb, opts->rootcontext, &sid);
>>> +             if (rc)
>>> +                     return 1;
>>> +             if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
>>> +                     return 1;
>>> +     }
>>> +     if (opts->defcontext) {
>>> +             rc = parse_sid(sb, opts->defcontext, &sid);
>>> +             if (rc)
>>> +                     return 1;
>>> +             if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
>>> +                     return 1;
>>> +     }
>>> +     return 0;
>>> +}
>>> +
>>>  static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
>>>  {
>>>       struct selinux_mnt_opts *opts = mnt_opts;
>>> @@ -6984,6 +7037,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>>
>>>       LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
>>>       LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
>>> +     LSM_HOOK_INIT(sb_mnt_opts_compat, selinux_sb_mnt_opts_compat),
>>>       LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
>>>       LSM_HOOK_INIT(sb_kern_mount, selinux_sb_kern_mount),
>>>       LSM_HOOK_INIT(sb_show_options, selinux_sb_show_options),

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5343 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210218195046.19280-2-olga.kornievskaia@gmail.com>
References: <20210218195046.19280-2-olga.kornievskaia@gmail.com>
TO: Olga Kornievskaia <olga.kornievskaia@gmail.com>
TO: trond.myklebust(a)hammerspace.com
TO: anna.schumaker(a)netapp.com
CC: linux-nfs(a)vger.kernel.org
CC: linux-security-module(a)vger.kernel.org
CC: selinux(a)vger.kernel.org

Hi Olga,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on nfs/linux-next]
[also build test WARNING on pcmoore-selinux/next linus/master security/next-testing v5.11 next-20210218]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

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
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago
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

48b605f83c920d Jeff Layton       2008-06-10  1017  
f7b422b17ee5ee David Howells     2006-06-09  1018  /*
ab88dca311a372 Al Viro           2019-12-10  1019   * Finish setting up an NFS superblock
f7b422b17ee5ee David Howells     2006-06-09  1020   */
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)
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;
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(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38051 bytes --]