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

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 [off-list ref] wrote:

quoted

On 2/12/2021 1:19 PM, Olga Kornievskaia wrote:

quoted

From: Olga Kornievskaia <redacted>

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 <redacted>
---
 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.

quoted

quoted

 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.

quoted

Also, "options are the same" isn't the right description, even for
SELinux.

Again compatible is better, I can change it.

quoted

quoted

  * @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),