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 | 1 +
security/security.c | 7 +++++
security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++++++++
5 files changed, 69 insertions(+)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 7aaa753b8608..fbfc07d0b3d5 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_do_mnt_opts_match, 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..a11b062c1847 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_do_mnt_opts_match:
+ * 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 1 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..07026db7304d 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_do_mnt_opts_match(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);
diff --git a/security/security.c b/security/security.c
index 7b09cfbae94f..dae380916c6a 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_do_mnt_opts_match(struct super_block *sb,
+ void *mnt_opts)
+{
+ return call_int_hook(sb_do_mnt_opts_match, 0, sb, mnt_opts);
+}
+EXPORT_SYMBOL(security_sb_do_mnt_opts_match);
+
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..aaa3a725da94 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_do_mnt_opts_match(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 ? 0 : 1;
+
+ /* superblock initialized and no options specified - reject if
+ * superblock has any options set, otherwise accept
+ */
+ if (!opts)
+ return (sbsec->flags & SE_MNTMASK) ? 0 : 1;
+
+ if (opts->fscontext) {
+ rc = parse_sid(sb, opts->fscontext, &sid);
+ if (rc)
+ return 0;
+ if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid))
+ return 0;
+ }
+ if (opts->context) {
+ rc = parse_sid(sb, opts->context, &sid);
+ if (rc)
+ return 0;
+ if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid))
+ return 0;
+ }
+ 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 0;
+ if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
+ return 0;
+ }
+ if (opts->defcontext) {
+ rc = parse_sid(sb, opts->defcontext, &sid);
+ if (rc)
+ return 0;
+ if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
+ return 0;
+ }
+ return 1;
+}
+
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_do_mnt_opts_match, selinux_sb_do_mnt_opts_match),
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..ea4e5252a1f0 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_do_mnt_opts_match(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/12/2021 1:19 PM, 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 | 1 +
> security/security.c | 7 +++++
> security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++++++++
> 5 files changed, 69 insertions(+)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 7aaa753b8608..fbfc07d0b3d5 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_do_mnt_opts_match, struct super_block *sb, void *mnt_opts)
Don't you want this to be sb_mnt_opts_compatible ?
They may not have to match. They just need to be acceptable
to the security module. SELinux doesn't appear to require
that they "match" in all respects.
> 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..a11b062c1847 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_do_mnt_opts_match:
> + * 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 1 if options are the same.
This is inconsistent with the convention for returns from LSM interfaces.
Return 0 on success and -ESOMETHINGOROTHER if the operation would be
denied. Your implementation of security_sb_do_mnt_opts_match() will
always return 0 if there's no module supplying the hook. I seriously
doubt that you want the mounts to fail 100% of the time if there isn't
an LSM.
Also, "options are the same" isn't the right description, even for
SELinux.
> * @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..07026db7304d 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_do_mnt_opts_match(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);
> diff --git a/security/security.c b/security/security.c
> index 7b09cfbae94f..dae380916c6a 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_do_mnt_opts_match(struct super_block *sb,
> + void *mnt_opts)
> +{
> + return call_int_hook(sb_do_mnt_opts_match, 0, sb, mnt_opts);
> +}
> +EXPORT_SYMBOL(security_sb_do_mnt_opts_match);
> +
> 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..aaa3a725da94 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_do_mnt_opts_match(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 ? 0 : 1;
> +
> + /* superblock initialized and no options specified - reject if
> + * superblock has any options set, otherwise accept
> + */
> + if (!opts)
> + return (sbsec->flags & SE_MNTMASK) ? 0 : 1;
> +
> + if (opts->fscontext) {
> + rc = parse_sid(sb, opts->fscontext, &sid);
> + if (rc)
> + return 0;
> + if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid))
> + return 0;
> + }
> + if (opts->context) {
> + rc = parse_sid(sb, opts->context, &sid);
> + if (rc)
> + return 0;
> + if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid))
> + return 0;
> + }
> + 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 0;
> + if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
> + return 0;
> + }
> + if (opts->defcontext) {
> + rc = parse_sid(sb, opts->defcontext, &sid);
> + if (rc)
> + return 0;
> + if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
> + return 0;
> + }
> + return 1;
> +}
> +
> 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_do_mnt_opts_match, selinux_sb_do_mnt_opts_match),
> 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),
Thank you for comment Casey. Comments are in line.
On Fri, Feb 12, 2021 at 5:37 PM Casey Schaufler <[email protected]> wrote:
>
> On 2/12/2021 1:19 PM, 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 | 1 +
> > security/security.c | 7 +++++
> > security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++++++++
> > 5 files changed, 69 insertions(+)
> >
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 7aaa753b8608..fbfc07d0b3d5 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_do_mnt_opts_match, struct super_block *sb, void *mnt_opts)
>
> Don't you want this to be sb_mnt_opts_compatible ?
> They may not have to match. They just need to be acceptable
> to the security module. SELinux doesn't appear to require
> that they "match" in all respects.
You are right, it was a poor naming choice. Compatible is better. I
can change it.
>
>
> > 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..a11b062c1847 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_do_mnt_opts_match:
> > + * 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 1 if options are the same.
>
> This is inconsistent with the convention for returns from LSM interfaces.
> Return 0 on success and -ESOMETHINGOROTHER if the operation would be
> denied. Your implementation of security_sb_do_mnt_opts_match() will
> always return 0 if there's no module supplying the hook. I seriously
> doubt that you want the mounts to fail 100% of the time if there isn't
> an LSM.
The mounts are not failing. This allows a file system to decide
whether or not to share the superblocks or not. Ok a good point, I
haven't tested building a kernel where there is LSM but SElinux is not
compiled into the kernel (ie, the only user of that hook). I'm not
sure that's a possible or an interesting option.
Ok, I can flip the returns, I wasn't aware that SElinux is so
restrictive in its return function meaning. Returning 0 on a success
when you are looking for a match/capability seems backwards.
> Also, "options are the same" isn't the right description, even for
> SELinux.
Again compatible is better, I can change it.
> > * @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..07026db7304d 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_do_mnt_opts_match(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);
> > diff --git a/security/security.c b/security/security.c
> > index 7b09cfbae94f..dae380916c6a 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_do_mnt_opts_match(struct super_block *sb,
> > + void *mnt_opts)
> > +{
> > + return call_int_hook(sb_do_mnt_opts_match, 0, sb, mnt_opts);
> > +}
> > +EXPORT_SYMBOL(security_sb_do_mnt_opts_match);
> > +
> > 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..aaa3a725da94 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_do_mnt_opts_match(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 ? 0 : 1;
> > +
> > + /* superblock initialized and no options specified - reject if
> > + * superblock has any options set, otherwise accept
> > + */
> > + if (!opts)
> > + return (sbsec->flags & SE_MNTMASK) ? 0 : 1;
> > +
> > + if (opts->fscontext) {
> > + rc = parse_sid(sb, opts->fscontext, &sid);
> > + if (rc)
> > + return 0;
> > + if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid))
> > + return 0;
> > + }
> > + if (opts->context) {
> > + rc = parse_sid(sb, opts->context, &sid);
> > + if (rc)
> > + return 0;
> > + if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid))
> > + return 0;
> > + }
> > + 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 0;
> > + if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
> > + return 0;
> > + }
> > + if (opts->defcontext) {
> > + rc = parse_sid(sb, opts->defcontext, &sid);
> > + if (rc)
> > + return 0;
> > + if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
> > + return 0;
> > + }
> > + return 1;
> > +}
> > +
> > 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_do_mnt_opts_match, selinux_sb_do_mnt_opts_match),
> > 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),
>
Hi Olga,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on nfs/linux-next]
[also build test ERROR on pcmoore-selinux/next linus/master security/next-testing v5.11-rc7 next-20210211]
[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/20210213-052321
base: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: x86_64-randconfig-r011-20210212 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/ff69e0bcc99716695e11ed2741b2e01d6014f960
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Olga-Kornievskaia/Add-new-hook-to-compare-new-mount-to-an-existing-mount/20210213-052321
git checkout ff69e0bcc99716695e11ed2741b2e01d6014f960
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> fs/nfs/super.c:1179:5: error: implicit declaration of function 'security_sb_do_mnt_opts_match' [-Werror,-Wimplicit-function-declaration]
!security_sb_do_mnt_opts_match(sb, fc->security))
^
fs/nfs/super.c:1179:5: note: did you mean 'security_sb_set_mnt_opts'?
include/linux/security.h:673:19: note: 'security_sb_set_mnt_opts' declared here
static inline int security_sb_set_mnt_opts(struct super_block *sb,
^
1 error generated.
vim +/security_sb_do_mnt_opts_match +1179 fs/nfs/super.c
1164
1165 static int nfs_compare_super(struct super_block *sb, struct fs_context *fc)
1166 {
1167 struct nfs_server *server = fc->s_fs_info, *old = NFS_SB(sb);
1168
1169 if (!nfs_compare_super_address(old, server))
1170 return 0;
1171 /* Note: NFS_MOUNT_UNSHARED == NFS4_MOUNT_UNSHARED */
1172 if (old->flags & NFS_MOUNT_UNSHARED)
1173 return 0;
1174 if (memcmp(&old->fsid, &server->fsid, sizeof(old->fsid)) != 0)
1175 return 0;
1176 if (!nfs_compare_userns(old, server))
1177 return 0;
1178 if ((old->has_sec_mnt_opts || fc->security) &&
> 1179 !security_sb_do_mnt_opts_match(sb, fc->security))
1180 return 0;
1181 return nfs_compare_mount_options(sb, server, fc);
1182 }
1183
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Olga,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on nfs/linux-next]
[also build test ERROR on pcmoore-selinux/next linus/master security/next-testing v5.11-rc7 next-20210211]
[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/20210213-052321
base: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/ff69e0bcc99716695e11ed2741b2e01d6014f960
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Olga-Kornievskaia/Add-new-hook-to-compare-new-mount-to-an-existing-mount/20210213-052321
git checkout ff69e0bcc99716695e11ed2741b2e01d6014f960
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
fs/nfs/super.c: In function 'nfs_compare_super':
>> fs/nfs/super.c:1179:5: error: implicit declaration of function 'security_sb_do_mnt_opts_match'; did you mean 'security_sb_set_mnt_opts'? [-Werror=implicit-function-declaration]
1179 | !security_sb_do_mnt_opts_match(sb, fc->security))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| security_sb_set_mnt_opts
cc1: some warnings being treated as errors
vim +1179 fs/nfs/super.c
1164
1165 static int nfs_compare_super(struct super_block *sb, struct fs_context *fc)
1166 {
1167 struct nfs_server *server = fc->s_fs_info, *old = NFS_SB(sb);
1168
1169 if (!nfs_compare_super_address(old, server))
1170 return 0;
1171 /* Note: NFS_MOUNT_UNSHARED == NFS4_MOUNT_UNSHARED */
1172 if (old->flags & NFS_MOUNT_UNSHARED)
1173 return 0;
1174 if (memcmp(&old->fsid, &server->fsid, sizeof(old->fsid)) != 0)
1175 return 0;
1176 if (!nfs_compare_userns(old, server))
1177 return 0;
1178 if ((old->has_sec_mnt_opts || fc->security) &&
> 1179 !security_sb_do_mnt_opts_match(sb, fc->security))
1180 return 0;
1181 return nfs_compare_mount_options(sb, server, fc);
1182 }
1183
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 2/12/2021 2:50 PM, Olga Kornievskaia wrote:
> Thank you for comment Casey. Comments are in line.
>
> On Fri, Feb 12, 2021 at 5:37 PM Casey Schaufler <[email protected]> wrote:
>> On 2/12/2021 1:19 PM, 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 | 1 +
>>> security/security.c | 7 +++++
>>> security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++++++++
>>> 5 files changed, 69 insertions(+)
>>>
>>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>>> index 7aaa753b8608..fbfc07d0b3d5 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_do_mnt_opts_match, struct super_block *sb, void *mnt_opts)
>> Don't you want this to be sb_mnt_opts_compatible ?
>> They may not have to match. They just need to be acceptable
>> to the security module. SELinux doesn't appear to require
>> that they "match" in all respects.
> You are right, it was a poor naming choice. Compatible is better. I
> can change it.
>>
>>> 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..a11b062c1847 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_do_mnt_opts_match:
>>> + * 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 1 if options are the same.
>> This is inconsistent with the convention for returns from LSM interfaces.
>> Return 0 on success and -ESOMETHINGOROTHER if the operation would be
>> denied. Your implementation of security_sb_do_mnt_opts_match() will
>> always return 0 if there's no module supplying the hook. I seriously
>> doubt that you want the mounts to fail 100% of the time if there isn't
>> an LSM.
> The mounts are not failing. This allows a file system to decide
> whether or not to share the superblocks or not.
OK. I hadn't gotten that far.
> Ok a good point, I
> haven't tested building a kernel where there is LSM but SElinux is not
> compiled into the kernel (ie, the only user of that hook).
That's a pretty important configuration.
> I'm not
> sure that's a possible or an interesting option.
Yeah, it's both possible and interesting. Every Ubuntu, Tizen
and most Yocto Project based systems will be affected. There
are many use cases for which SELinux is not the best choice,
at least in this reviewer's experience.
> Ok, I can flip the returns, I wasn't aware that SElinux is so
> restrictive in its return function meaning.
You need to be aware that the LSM infrastructure, while
heavily influenced by SELinux, is not defined by SELinux.
> Returning 0 on a success
> when you are looking for a match/capability seems backwards.
It is an artifact of the system internal convention of
returning 0 on success and an error code if something goes
wrong. There is one way to succeed, but many ways to fail.
>> Also, "options are the same" isn't the right description, even for
>> SELinux.
> Again compatible is better, I can change it.
>
>>> * @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..07026db7304d 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_do_mnt_opts_match(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);
>>> diff --git a/security/security.c b/security/security.c
>>> index 7b09cfbae94f..dae380916c6a 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_do_mnt_opts_match(struct super_block *sb,
>>> + void *mnt_opts)
>>> +{
>>> + return call_int_hook(sb_do_mnt_opts_match, 0, sb, mnt_opts);
>>> +}
>>> +EXPORT_SYMBOL(security_sb_do_mnt_opts_match);
>>> +
>>> 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..aaa3a725da94 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_do_mnt_opts_match(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 ? 0 : 1;
>>> +
>>> + /* superblock initialized and no options specified - reject if
>>> + * superblock has any options set, otherwise accept
>>> + */
>>> + if (!opts)
>>> + return (sbsec->flags & SE_MNTMASK) ? 0 : 1;
>>> +
>>> + if (opts->fscontext) {
>>> + rc = parse_sid(sb, opts->fscontext, &sid);
>>> + if (rc)
>>> + return 0;
>>> + if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid))
>>> + return 0;
>>> + }
>>> + if (opts->context) {
>>> + rc = parse_sid(sb, opts->context, &sid);
>>> + if (rc)
>>> + return 0;
>>> + if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid))
>>> + return 0;
>>> + }
>>> + 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 0;
>>> + if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
>>> + return 0;
>>> + }
>>> + if (opts->defcontext) {
>>> + rc = parse_sid(sb, opts->defcontext, &sid);
>>> + if (rc)
>>> + return 0;
>>> + if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
>>> + return 0;
>>> + }
>>> + return 1;
>>> +}
>>> +
>>> 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_do_mnt_opts_match, selinux_sb_do_mnt_opts_match),
>>> 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),