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 <trond.myklebust@hammerspace.com>,
	Anna Schumaker <anna.schumaker@netapp.com>,
	Paul Moore <paul@paul-moore.com>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	linux-nfs <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 21:24:00 +0100	[thread overview]
Message-ID: <CAFqZXNusby0F6Azkf0U_tV-1KCQT0Z1NZH6v0GXxt9RBQrR07g@mail.gmail.com> (raw)
In-Reply-To: <CAN-5tyF+cLpmT=rwAYvQQ445tjFKZtGq+Qzf6rDGg8COPpFRwA@mail.gmail.com>

On Thu, Nov 5, 2020 at 8:51 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
> On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > 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
>
> Very interesting. I was not aware of something like that nor was it
> mentioned when I asked on the selinux mailing list. I wonder if this
> is a supported feature that will always stay? In that case, NFS
> wouldn't need the extra hook that was added for this series. I will
> try this out and report back.

I wish I could have suggested it sooner, but I only learned about it
in recent days while investigating a bug that involves SELinux and NFS
:) And when I saw your patch, it immediately reminded me of that flag.

I don't think it's going away, at least as long as NFS depends on it.
If someone were to remove it, they would have to provide something
equivalent to make sure no existing users get broken, as with any
in-kernel change.

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