lkml.org 
[lkml]   [2022]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3] nfs: Fix automount superblock LSM init problem, preventing sb sharing
    On Thu, Aug 11, 2022 at 8:28 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
    > On Fri, Aug 5, 2022 at 3:36 PM David Howells <dhowells@redhat.com> 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@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 <trond.myklebust@hammerspace.com>
    > > cc: Anna Schumaker <anna@kernel.org>
    > > cc: Alexander Viro <viro@zeniv.linux.org.uk>
    > > cc: Scott Mayhew <smayhew@redhat.com>
    > > 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>
    >
    > > 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

    \
     
     \ /
      Last update: 2022-08-16 08:40    [W:3.508 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site