Received-SPF: pass (google.com: domain of linux-nfs+bounces-1812-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Christian Brauner" <brauner@kernel.org>
Cc: "Jeff Layton" <jlayton@kernel.org>,
 "Alexander Viro" <viro@zeniv.linux.org.uk>, "Jan Kara" <jack@suse.cz>,
 "Chuck Lever" <chuck.lever@oracle.com>,
 "Olga Kornievskaia" <kolga@netapp.com>, "Dai Ngo" <Dai.Ngo@oracle.com>,
 "Tom Talpey" <tom@talpey.com>, linux-fsdevel@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-nfs@vger.kernel.org,
 Ondrej =?utf-8?q?Mosn=C3=A1=C4=8Dek?= <omosnacek@gmail.com>,
 "Zdenek Pytela" <zpytela@redhat.com>
Subject: Re: [PATCH] filelock: don't do security checks on nfsd setlease calls
In-reply-to: <20240206-gewaschen-bauen-f7932047a1be@brauner>
References: <20240205-bz2248830-v1-1-d0ec0daecba1@kernel.org>,
 <170716318935.13976.13465352731929804157@noble.neil.brown.name>,
 <20240206-gewaschen-bauen-f7932047a1be@brauner>
Date: Wed, 07 Feb 2024 08:16:57 +1100
Message-id: <170725421733.13976.232570371514420308@noble.neil.brown.name>

On Wed, 07 Feb 2024, Christian Brauner wrote:
> On Tue, Feb 06, 2024 at 06:59:49AM +1100, NeilBrown wrote:
> > On Mon, 05 Feb 2024, Jeff Layton wrote:
> > > Zdenek reported seeing some AVC denials due to nfsd trying to set
> > > delegations:
> > >=20
> > >     type=3DAVC msg=3Daudit(09.11.2023 09:03:46.411:496) : avc:  denied =
 { lease } for  pid=3D5127 comm=3Drpc.nfsd capability=3Dlease  scontext=3Dsys=
tem_u:system_r:nfsd_t:s0 tcontext=3Dsystem_u:system_r:nfsd_t:s0 tclass=3Dcapa=
bility permissive=3D0
> > >=20
> > > When setting delegations on behalf of nfsd, we don't want to do all of
> > > the normal capabilty and LSM checks. nfsd is a kernel thread and runs
> > > with CAP_LEASE set, so the uid checks end up being a no-op in most cases
> > > anyway.
> > >=20
> > > Some nfsd functions can end up running in normal process context when
> > > tearing down the server. At that point, the CAP_LEASE check can fail and
> > > cause the client to not tear down delegations when expected.
> > >=20
> > > Also, the way the per-fs ->setlease handlers work today is a little
> > > convoluted. The non-trivial ones are wrappers around generic_setlease,
> > > so when they fail due to permission problems they usually they end up
> > > doing a little extra work only to determine that they can't set the
> > > lease anyway. It would be more efficient to do those checks earlier.
> > >=20
> > > Transplant the permission checking from generic_setlease to
> > > vfs_setlease, which will make the permission checking happen earlier on
> > > filesystems that have a ->setlease operation. Add a new kernel_setlease
> > > function that bypasses these checks, and switch nfsd to use that instead
> > > of vfs_setlease.
> > >=20
> > > There is one behavioral change here: prior this patch the
> > > setlease_notifier would fire even if the lease attempt was going to fail
> > > the security checks later. With this change, it doesn't fire until the
> > > caller has passed them. I think this is a desirable change overall. nfsd
> > > is the only user of the setlease_notifier and it doesn't benefit from
> > > being notified about failed attempts.
> > >=20
> > > Cc: Ondrej Mosn=C3=A1=C4=8Dek <omosnacek@gmail.com>
> > > Reported-by: Zdenek Pytela <zpytela@redhat.com>
> > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=3D2248830
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >=20
> > Reviewed-by: NeilBrown <neilb@suse.de>
> >=20
> > It definitely nice to move all the security and sanity check early.
> > This patch allows a minor clean-up in cifs which could possibly be
> > included:
> > diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> > index 2a4a4e3a8751..0f142d1ec64f 100644
> >=20
> > --- a/fs/smb/client/cifsfs.c
> > +++ b/fs/smb/client/cifsfs.c
> > @@ -1094,9 +1094,6 @@ cifs_setlease(struct file *file, int arg, struct fi=
le_lock **lease, void **priv)
> >  	struct inode *inode =3D file_inode(file);
> >  	struct cifsFileInfo *cfile =3D file->private_data;
> > =20
> > -	if (!(S_ISREG(inode->i_mode)))
> > -		return -EINVAL;
> > -
> >  	/* Check if file is oplocked if this is request for new lease */
> >  	if (arg =3D=3D F_UNLCK ||
> >  	    ((arg =3D=3D F_RDLCK) && CIFS_CACHE_READ(CIFS_I(inode))) ||
> >=20
> >=20
> > as ->setlease() is now never called for non-ISREG files.
>=20
> I've added the following on top. I've made you author and added your
> SoB. Please tell me if you have any problems with this:

No problems at all - thanks for doing this!

NeilBrown


>=20
> From d30e52329760873bf0d7984a442cace3a4b5f39d Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@suse.de>
> Date: Tue, 6 Feb 2024 14:08:57 +0100
> Subject: [PATCH] smb: remove redundant check
>=20
> ->setlease() is never called on non-regular files now. So remove the
> check from cifs_setlease().
>=20
> Link: https://lore.kernel.org/r/170716318935.13976.13465352731929804157@nob=
le.neil.brown.name
> Signed-off-by: NeilBrown <neilb@suse.de>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/smb/client/cifsfs.c | 3 ---
>  1 file changed, 3 deletions(-)
>=20
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 5eee5b00547f..cbcb98d5f2d7 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -1094,9 +1094,6 @@ cifs_setlease(struct file *file, int arg, struct file=
_lease **lease, void **priv
>  	struct inode *inode =3D file_inode(file);
>  	struct cifsFileInfo *cfile =3D file->private_data;
> =20
> -	if (!(S_ISREG(inode->i_mode)))
> -		return -EINVAL;
> -
>  	/* Check if file is oplocked if this is request for new lease */
>  	if (arg =3D=3D F_UNLCK ||
>  	    ((arg =3D=3D F_RDLCK) && CIFS_CACHE_READ(CIFS_I(inode))) ||
> --=20
> 2.43.0
>=20
>=20