On Thu, Aug 11, 2022 at 8:28 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Fri, Aug 5, 2022 at 3:36 PM David Howells <dhowells@xxxxxxxxxx> wrote:
> > 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@xxxxxxxxxx>
> > 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 <trond.myklebust@xxxxxxxxxxxxxxx>
> > cc: Anna Schumaker <anna@xxxxxxxxxx>
> > cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
> > cc: Scott Mayhew <smayhew@xxxxxxxxxx>
> > cc: Jeff Layton <jlayton@xxxxxxxxxx>
> > cc: Paul Moore <paul@xxxxxxxxxxxxxx>
> > cc: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> > cc: linux-nfs@xxxxxxxxxxxxxxx
> > cc: selinux@xxxxxxxxxxxxxxx
> > cc: linux-security-module@xxxxxxxxxxxxxxx
> > cc: linux-fsdevel@xxxxxxxxxxxxxxx
> > ---
> >
> > 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>
>
> > 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
