From: Olga Kornievskaia <[email protected]>
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 <[email protected]>
`
---
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
From: Olga Kornievskaia <[email protected]>
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 <[email protected]>
---
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
On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> 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 <[email protected]>
> `
> ---
> 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),
On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 */
On Thu, Feb 18, 2021 at 5:07 PM Casey Schaufler <[email protected]> wrote:
>
> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 */
>
On 2/18/2021 2:39 PM, Olga Kornievskaia wrote:
> On Thu, Feb 18, 2021 at 5:07 PM Casey Schaufler <[email protected]> wrote:
>> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
>>> From: Olga Kornievskaia <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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 */
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 <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
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/[email protected]
On Thu, Feb 18, 2021 at 4:57 PM Casey Schaufler <[email protected]> wrote:
>
> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <[email protected]>
> >
> > 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 <[email protected]>
> > `
> > ---
> > 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),
On 2/19/2021 8:25 AM, Olga Kornievskaia wrote:
> On Thu, Feb 18, 2021 at 4:57 PM Casey Schaufler <[email protected]> wrote:
>> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
>>> From: Olga Kornievskaia <[email protected]>
>>>
>>> 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 <[email protected]>
>>> `
>>> ---
>>> 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),
On Thu, Feb 18, 2021 at 6:17 PM Casey Schaufler <[email protected]> wrote:
>
> On 2/18/2021 2:39 PM, Olga Kornievskaia wrote:
> > On Thu, Feb 18, 2021 at 5:07 PM Casey Schaufler <[email protected]> wrote:
> >> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> >>> From: Olga Kornievskaia <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>> 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 */
>
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 <[email protected]> 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 <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> 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/[email protected]
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
[email protected]