Re: [PATCH v6] vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing

[Paul Moore]

On Aug  2, 2023 Jeff Layton <jlayton@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)
> 
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Acked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> Acked-by: "Christian Brauner (Microsoft)" <brauner@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/165962680944.3334508.6610023900349142034.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v1
> Link: https://lore.kernel.org/r/165962729225.3357250.14350728846471527137.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v2
> Link: https://lore.kernel.org/r/165970659095.2812394.6868894171102318796.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v3
> Link: https://lore.kernel.org/r/166133579016.3678898.6283195019480567275.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v4
> Link: https://lore.kernel.org/r/217595.1662033775@xxxxxxxxxxxxxxxxxxxxxx/ # 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 ..."?

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.

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.

> 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.

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

--
paul-moore.com