Re: [PATCH v3] nfs: Fix automount superblock LSM init problem, preventing sb sharing

[Paul Moore]

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