Message-ID: <ca156cecbc070c3b7c68626572274806079a6e04.camel@kernel.org>
Date:   Wed, 02 Aug 2023 15:34:27 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Paul Moore <paul@...l-moore.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna@...nel.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>,
        Casey Schaufler <casey@...aufler-ca.com>,
        David Howells <dhowells@...hat.com>,
        Scott Mayhew <smayhew@...hat.com>
Cc:     Stephen Smalley <sds@...ho.nsa.gov>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: [PATCH v6] vfs, security: Fix automount superblock LSM init 
 problem, preventing NFS sb sharing

On Wed, 2023-08-02 at 14:16 -0400, Paul Moore wrote:
> On Aug  2, 2023 Jeff Layton <jlayton@...nel.org> 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)
> > 
> > Signed-off-by: David Howells <dhowells@...hat.com>
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > Fixes: 9bc61ab18b1d ("vfs: Introduce fs_context, switch vfs_kern_mount() to it.")
> > Fixes: 779df6a5480f ("NFS: Ensure security label is set for root inode)
> > Tested-by: Jeff Layton <jlayton@...nel.org>
> > Reviewed-by: Jeff Layton <jlayton@...nel.org>
> > Acked-by: Casey Schaufler <casey@...aufler-ca.com>
> > Acked-by: "Christian Brauner (Microsoft)" <brauner@...nel.org>
> > Link: https://lore.kernel.org/r/165962680944.3334508.6610023900349142034.stgit@warthog.procyon.org.uk/ # v1
> > Link: https://lore.kernel.org/r/165962729225.3357250.14350728846471527137.stgit@warthog.procyon.org.uk/ # v2
> > Link: https://lore.kernel.org/r/165970659095.2812394.6868894171102318796.stgit@warthog.procyon.org.uk/ # v3
> > Link: https://lore.kernel.org/r/166133579016.3678898.6283195019480567275.stgit@warthog.procyon.org.uk/ # v4
> > Link: https://lore.kernel.org/r/217595.1662033775@warthog.procyon.org.uk/ # v5
> > ---
> > This patch was originally sent by David several months ago, but it
> > never got merged. I'm resending to resurrect the discussion. Can we
> > get this fixed?
> 
> Sorry, I sorta lost track of this after the ROOTCONTEXT_MNT discussion
> back in v3.  Looking at it a bit closer now I have one nitpicky
> request and one larger concern (see below).
> 
> > diff --git a/fs/super.c b/fs/super.c
> > index e781226e2880..13adf43e2e5d 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -1541,10 +1541,12 @@ int vfs_get_tree(struct fs_context *fc)
> >  	smp_wmb();
> >  	sb->s_flags |= SB_BORN;
> >  
> > -	error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL);
> > -	if (unlikely(error)) {
> > -		fc_drop_locked(fc);
> > -		return error;
> > +	if (!(fc->lsm_set)) {
> > +		error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL);
> > +		if (unlikely(error)) {
> > +			fc_drop_locked(fc);
> > +			return error;
> > +		}
> >  	}
> 
> I generally dislike core kernel code which makes LSM calls conditional
> on some kernel state maintained outside the LSM.  Sometimes it has to
> be done as there is no other good options, but I would like us to try
> and avoid it if possible.  The commit description mentioned that this
> was put here to avoid a SELinux complaint, can you provide an example
> of the complain?  Does it complain about a double/invalid mount, e.g.
> "SELinux: mount invalid.  Same superblock, different security ..."?
> 

The problem I had was not so much SELinux warnings, but rather that in a
situation where I would expect to share superblocks between two
filesystems, it didn't.

Basically if you do something like this:

# mount nfsserver:/export/foo /mnt/foo -o context=system_u:object_r:root_t:s0
# mount nfsserver:/export/bar /mnt/bar -o context=system_u:object_r:root_t:s0

...when "foo" and "bar" are directories on the same filesystem on the
server, you should get two vfsmounts that share a superblock. That's
what you get if selinux is disabled, but not when it's enabled (even
when it's in permissive mode).

The problems that David hit with the automounter have a similar root
cause though, I believe.

> I'd like to understand why the sb_set_mnt_opts() call fails when it
> comes after the fs_context_init() call.  I'm particulary curious to
> know if the failure is due to conflicting SELinux state in the
> fs_context, or if it is simply an issue of sb_set_mnt_opts() not
> properly handling existing values.  Perhaps I'm being overly naive,
> but I'm hopeful that we can address both of these within the SELinux
> code itself.
> 

The problem I hit was that nfs_compare_super is called with a fs_context
that has a NULL ->security pointer. That caused it to call
selinux_sb_mnt_opts_compat with mnt_opts set to NULL, and at that point
it returns 1 and decides not to share sb's.

Filling out fc->security with this new operation seems to fix that, but
if you see a better way to do this, then I'm certainly open to the idea.

> In a worst case situation, we could always implement a flag *inside*
> the SELinux code, similar to what has been done with 'lsm_set' here.
> 

I'm fine with a different solution, if you see a better one. You'll have
to handhold me through this one though. LSM stuff is not really my
forte'. Let me know what you'd like to see here.


> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index d06e350fedee..29cce0fadbeb 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2745,6 +2745,30 @@ 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;
> > +	struct selinux_mnt_opts *opts;
> > +
> > +	if (fc->purpose == FS_CONTEXT_FOR_SUBMOUNT) {
> > +		opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> > +		if (!opts)
> > +			return -ENOMEM;
> > +
> > +		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 & DEFCONTEXT_MNT)
> > +			opts->defcontext_sid	= sbsec->def_sid;
> 
> I acknowledge this is very nitpicky, but we're starting to make a
> greater effort towards using consistent style within the SELinux
> code.  With that in mind, please remove the alignment whitespace in
> the assignments above.  Thank you.
> 

Will do. Thanks for having a look!

> > +		fc->security = opts;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int selinux_fs_context_dup(struct fs_context *fc,
> >  				  struct fs_context *src_fc)
> >  {
> 
> --
> paul-moore.com

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists