On Thu, Aug 11, 2022 at 8:28 AM Ondrej Mosnacek [off-list ref] wrote:

On Fri, Aug 5, 2022 at 3:36 PM David Howells [off-list ref] wrote:

quoted

When NFS superblocks are created by automounting, their LSM parameters
aren't set in the fs_context struct prior to sget_fc() being called,
leading to failure to match existing superblocks.

Fix this by adding a new LSM hook to load fc->security for submount
creation when alloc_fs_context() is creating the fs_context for it.

However, this uncovers a further bug: nfs_get_root() initialises the
superblock security manually by calling security_sb_set_mnt_opts() or
security_sb_clone_mnt_opts() - but then vfs_get_tree() calls
security_sb_set_mnt_opts(), which can lead to SELinux, at least,
complaining.

Fix that by adding a flag to the fs_context that suppresses the
security_sb_set_mnt_opts() call in vfs_get_tree().  This can be set by NFS
when it sets the LSM context on the new superblock.

The first bug leads to messages like the following appearing in dmesg:

        NFS: Cache volume key already in use (nfs,4.2,2,108,106a8c0,1,,,,100000,100000,2ee,3a98,1d4c,3a98,1)

Changes
=======
ver #2)
 - Made LSM parameter extraction dependent on fc->purpose ==
   FS_CONTEXT_FOR_SUBMOUNT.  Shouldn't happen on FOR_RECONFIGURE.

ver #2)
 - Added Smack support
 - Made LSM parameter extraction dependent on reference != NULL.

Signed-off-by: David Howells <dhowells@redhat.com>
Fixes: 9bc61ab18b1d ("vfs: Introduce fs_context, switch vfs_kern_mount() to it.")
Fixes: 779df6a5480f ("NFS: Ensure security label is set for root inode)
cc: Trond Myklebust <redacted>
cc: Anna Schumaker <anna@kernel.org>
cc: Alexander Viro <viro@zeniv.linux.org.uk>
cc: Scott Mayhew <redacted>
cc: Jeff Layton <jlayton@kernel.org>
cc: Paul Moore <paul@paul-moore.com>
cc: Casey Schaufler <casey@schaufler-ca.com>
cc: linux-nfs@vger.kernel.org
cc: selinux@vger.kernel.org
cc: linux-security-module@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
---

 fs/fs_context.c               |    4 +++
 fs/nfs/getroot.c              |    1 +
 fs/super.c                    |   10 ++++---
 include/linux/fs_context.h    |    1 +
 include/linux/lsm_hook_defs.h |    1 +
 include/linux/lsm_hooks.h     |    6 +++-
 include/linux/security.h      |    6 ++++
 security/security.c           |    5 +++
 security/selinux/hooks.c      |   29 +++++++++++++++++++
 security/smack/smack_lsm.c    |   61 +++++++++++++++++++++++++++++++++++++++++
 10 files changed, 119 insertions(+), 5 deletions(-)

<snip>

quoted

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1bbd53321d13..ddeaff4f3bb1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c

@@ -2768,6 +2768,34 @@ static int selinux_umount(struct vfsmount *mnt, int flags)
                                   FILESYSTEM__UNMOUNT, NULL);
 }

+static int selinux_fs_context_init(struct fs_context *fc,
+                                  struct dentry *reference)
+{
+       const struct superblock_security_struct *sbsec;
+       const struct inode_security_struct *root_isec;
+       struct selinux_mnt_opts *opts;
+
+       if (fc->purpose == FS_CONTEXT_FOR_SUBMOUNT) {
+               opts = kzalloc(sizeof(*opts), GFP_KERNEL);
+               if (!opts)
+                       return -ENOMEM;
+
+               root_isec = backing_inode_security(reference->d_sb->s_root);
+               sbsec = selinux_superblock(reference->d_sb);
+               if (sbsec->flags & FSCONTEXT_MNT)
+                       opts->fscontext_sid     = sbsec->sid;
+               if (sbsec->flags & CONTEXT_MNT)
+                       opts->context_sid       = sbsec->mntpoint_sid;
+               if (sbsec->flags & ROOTCONTEXT_MNT)
+                       opts->rootcontext_sid   = root_isec->sid;

I wonder if this part is correct... The rootcontext=... mount option
relates to the root inode of the mount where it is specified - i.e. in
case of NFS only to the toplevel inode of the initial mount. Setting
the same context on the root inode of submounts, which AFAIK are
supposed to be transparent to the user, doesn't seem correct to me -
i.e. it should just be left unset for the automatically created
submounts.

Like Ondrej, I'm not going to say I'm very comfortable with some of
the VFS corner cases, but this is an interesting case ... as far as I
can tell, the submount has a superblock and is treated like a normal
filesystem mount with the one exception that it is mounted
automatically so that users might not be aware it is a separate mount.

I guess my question is this: for inodes inside the superblock, does
their superblock pointer point to the submount's superblock, or the
parent filesystem's superblock?

-- 
paul-moore.com