Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state

From: Ondrej Mosnacek <omosnace@redhat.com>
To: Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc: trond.myklebust@hammerspace.com, anna.schumaker@netapp.com,
	Paul Moore <paul@paul-moore.com>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	linux-nfs@vger.kernel.org,
	Linux Security Module list 
	<linux-security-module@vger.kernel.org>,
	SElinux list <selinux@vger.kernel.org>
Subject: Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
Date: Thu, 5 Nov 2020 19:55:23 +0100	[thread overview]
Message-ID: <CAFqZXNtjMEF0LO4vtEmcgwydbWfUS36d8g24J6C-NDXORYbEJg@mail.gmail.com> (raw)
In-Reply-To: <20201105173328.2539-2-olga.kornievskaia@gmail.com>

On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
>
> Currently, the client will always ask for security_labels if the server
> returns that it supports that feature regardless of any LSM modules
> (such as Selinux) enforcing security policy. This adds performance
> penalty to the READDIR operation.
>
> Instead, query the LSM module to find if anything is enabled and
> if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.

Having spent some time staring at some of the NFS code very recently,
I can't help but suggest: Would it perhaps be enough to decide whether
to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some LSM
confirms via the security_sb_*_mnt_opts() hook that it wants the
filesystem to give it labels (or at least that's how I interpret the
cryptic name) [1]. It's just a shot in the dark, but it seems to fit
this use case.

[1] https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148

>
> Suggested-by: Scott Mayhew <smayhew@redhat.com>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4proc.c       | 5 +++++
>  fs/nfs/nfs4xdr.c        | 3 ++-
>  include/linux/nfs_xdr.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9e0ca9b2b210..774bc5e63ca7 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -55,6 +55,7 @@
>  #include <linux/utsname.h>
>  #include <linux/freezer.h>
>  #include <linux/iversion.h>
> +#include <linux/security.h>
>
>  #include "nfs4_fs.h"
>  #include "delegation.h"
> @@ -4968,6 +4969,7 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
>                 .count = count,
>                 .bitmask = NFS_SERVER(d_inode(dentry))->attr_bitmask,
>                 .plus = plus,
> +               .labels = true,
>         };
>         struct nfs4_readdir_res res;
>         struct rpc_message msg = {
> @@ -4977,10 +4979,13 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
>                 .rpc_cred = cred,
>         };
>         int                     status;
> +       int sec_flags = LSM_FQUERY_VFS_XATTRS;
>
>         dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
>                         dentry,
>                         (unsigned long long)cookie);
> +       if (!security_func_query_vfs(sec_flags))
> +               args.labels = false;
>         nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
>         res.pgbase = args.pgbase;
>         status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index c6dbfcae7517..585d5b5cc3dc 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -1605,7 +1605,8 @@ static void encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg
>                         FATTR4_WORD1_OWNER_GROUP|FATTR4_WORD1_RAWDEV|
>                         FATTR4_WORD1_SPACE_USED|FATTR4_WORD1_TIME_ACCESS|
>                         FATTR4_WORD1_TIME_METADATA|FATTR4_WORD1_TIME_MODIFY;
> -               attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
> +               if (readdir->labels)
> +                       attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
>                 dircount >>= 1;
>         }
>         /* Use mounted_on_fileid only if the server supports it */
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index d63cb862d58e..95f648b26525 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1119,6 +1119,7 @@ struct nfs4_readdir_arg {
>         unsigned int                    pgbase; /* zero-copy data */
>         const u32 *                     bitmask;
>         bool                            plus;
> +       bool                            labels;
>  };
>
>  struct nfs4_readdir_res {
> --
> 2.18.2
>

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.