Received-SPF: pass (google.com: domain of linux-kernel+bounces-53881-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Jeff Layton" <jlayton@kernel.org>
Cc: "Alexander Viro" <viro@zeniv.linux.org.uk>,
 "Christian Brauner" <brauner@kernel.org>, "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>, "Jeff Layton" <jlayton@kernel.org>
Subject: Re: [PATCH] filelock: don't do security checks on nfsd setlease calls
In-reply-to: <20240205-bz2248830-v1-1-d0ec0daecba1@kernel.org>
References: <20240205-bz2248830-v1-1-d0ec0daecba1@kernel.org>
Date: Tue, 06 Feb 2024 06:59:49 +1100
Message-id: <170716318935.13976.13465352731929804157@noble.neil.brown.name>

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  { l=
ease } for  pid=3D5127 comm=3Drpc.nfsd capability=3Dlease  scontext=3Dsystem_=
u:system_r:nfsd_t:s0 tcontext=3Dsystem_u:system_r:nfsd_t:s0 tclass=3Dcapabili=
ty 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>

Reviewed-by: NeilBrown <neilb@suse.de>

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

--- 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_l=
ock **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))) ||


as ->setlease() is now never called for non-ISREG files.

NeilBrown


> ---
> This patch is based on top of a merge of Christian's vfs.file branch
> (which has the file_lock/lease split). There is a small merge confict
> with Chuck's nfsd-next patch, but it should be fairly simple to resolve.
> ---
>  fs/locks.c               | 43 +++++++++++++++++++++++++------------------
>  fs/nfsd/nfs4layouts.c    |  5 ++---
>  fs/nfsd/nfs4state.c      |  8 ++++----
>  include/linux/filelock.h |  7 +++++++
>  4 files changed, 38 insertions(+), 25 deletions(-)
>=20
> diff --git a/fs/locks.c b/fs/locks.c
> index 33c7f4a8c729..26d52ef5314a 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1925,18 +1925,6 @@ static int generic_delete_lease(struct file *filp, v=
oid *owner)
>  int generic_setlease(struct file *filp, int arg, struct file_lease **flp,
>  			void **priv)
>  {
> -	struct inode *inode =3D file_inode(filp);
> -	vfsuid_t vfsuid =3D i_uid_into_vfsuid(file_mnt_idmap(filp), inode);
> -	int error;
> -
> -	if ((!vfsuid_eq_kuid(vfsuid, current_fsuid())) && !capable(CAP_LEASE))
> -		return -EACCES;
> -	if (!S_ISREG(inode->i_mode))
> -		return -EINVAL;
> -	error =3D security_file_lock(filp, arg);
> -	if (error)
> -		return error;
> -
>  	switch (arg) {
>  	case F_UNLCK:
>  		return generic_delete_lease(filp, *priv);
> @@ -1987,6 +1975,19 @@ void lease_unregister_notifier(struct notifier_block=
 *nb)
>  }
>  EXPORT_SYMBOL_GPL(lease_unregister_notifier);
> =20
> +
> +int
> +kernel_setlease(struct file *filp, int arg, struct file_lease **lease, voi=
d **priv)
> +{
> +	if (lease)
> +		setlease_notifier(arg, *lease);
> +	if (filp->f_op->setlease)
> +		return filp->f_op->setlease(filp, arg, lease, priv);
> +	else
> +		return generic_setlease(filp, arg, lease, priv);
> +}
> +EXPORT_SYMBOL_GPL(kernel_setlease);
> +
>  /**
>   * vfs_setlease        -       sets a lease on an open file
>   * @filp:	file pointer
> @@ -2007,12 +2008,18 @@ EXPORT_SYMBOL_GPL(lease_unregister_notifier);
>  int
>  vfs_setlease(struct file *filp, int arg, struct file_lease **lease, void *=
*priv)
>  {
> -	if (lease)
> -		setlease_notifier(arg, *lease);
> -	if (filp->f_op->setlease)
> -		return filp->f_op->setlease(filp, arg, lease, priv);
> -	else
> -		return generic_setlease(filp, arg, lease, priv);
> +	struct inode *inode =3D file_inode(filp);
> +	vfsuid_t vfsuid =3D i_uid_into_vfsuid(file_mnt_idmap(filp), inode);
> +	int error;
> +
> +	if ((!vfsuid_eq_kuid(vfsuid, current_fsuid())) && !capable(CAP_LEASE))
> +		return -EACCES;
> +	if (!S_ISREG(inode->i_mode))
> +		return -EINVAL;
> +	error =3D security_file_lock(filp, arg);
> +	if (error)
> +		return error;
> +	return kernel_setlease(filp, arg, lease, priv);
>  }
>  EXPORT_SYMBOL_GPL(vfs_setlease);
> =20
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 4fa21b74a981..4c0d00bdfbb1 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -170,7 +170,7 @@ nfsd4_free_layout_stateid(struct nfs4_stid *stid)
>  	spin_unlock(&fp->fi_lock);
> =20
>  	if (!nfsd4_layout_ops[ls->ls_layout_type]->disable_recalls)
> -		vfs_setlease(ls->ls_file->nf_file, F_UNLCK, NULL, (void **)&ls);
> +		kernel_setlease(ls->ls_file->nf_file, F_UNLCK, NULL, (void **)&ls);
>  	nfsd_file_put(ls->ls_file);
> =20
>  	if (ls->ls_recalled)
> @@ -199,8 +199,7 @@ nfsd4_layout_setlease(struct nfs4_layout_stateid *ls)
>  	fl->c.flc_pid =3D current->tgid;
>  	fl->c.flc_file =3D ls->ls_file->nf_file;
> =20
> -	status =3D vfs_setlease(fl->c.flc_file, fl->c.flc_type, &fl,
> -			      NULL);
> +	status =3D kernel_setlease(fl->c.flc_file, fl->c.flc_type, &fl, NULL);
>  	if (status) {
>  		locks_free_lease(fl);
>  		return status;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b2c8efb5f793..6d52ecba8e9c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1249,7 +1249,7 @@ static void nfs4_unlock_deleg_lease(struct nfs4_deleg=
ation *dp)
> =20
>  	WARN_ON_ONCE(!fp->fi_delegees);
> =20
> -	vfs_setlease(nf->nf_file, F_UNLCK, NULL, (void **)&dp);
> +	kernel_setlease(nf->nf_file, F_UNLCK, NULL, (void **)&dp);
>  	put_deleg_file(fp);
>  }
> =20
> @@ -5532,8 +5532,8 @@ nfs4_set_delegation(struct nfsd4_open *open, struct n=
fs4_ol_stateid *stp,
>  	if (!fl)
>  		goto out_clnt_odstate;
> =20
> -	status =3D vfs_setlease(fp->fi_deleg_file->nf_file,
> -			      fl->c.flc_type, &fl, NULL);
> +	status =3D kernel_setlease(fp->fi_deleg_file->nf_file,
> +				      fl->c.flc_type, &fl, NULL);
>  	if (fl)
>  		locks_free_lease(fl);
>  	if (status)
> @@ -5571,7 +5571,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct n=
fs4_ol_stateid *stp,
> =20
>  	return dp;
>  out_unlock:
> -	vfs_setlease(fp->fi_deleg_file->nf_file, F_UNLCK, NULL, (void **)&dp);
> +	kernel_setlease(fp->fi_deleg_file->nf_file, F_UNLCK, NULL, (void **)&dp);
>  out_clnt_odstate:
>  	put_clnt_odstate(dp->dl_clnt_odstate);
>  	nfs4_put_stid(&dp->dl_stid);
> diff --git a/include/linux/filelock.h b/include/linux/filelock.h
> index 4a5ad26962c1..cd6c1c291de9 100644
> --- a/include/linux/filelock.h
> +++ b/include/linux/filelock.h
> @@ -208,6 +208,7 @@ struct file_lease *locks_alloc_lease(void);
>  int __break_lease(struct inode *inode, unsigned int flags, unsigned int ty=
pe);
>  void lease_get_mtime(struct inode *, struct timespec64 *time);
>  int generic_setlease(struct file *, int, struct file_lease **, void **priv=
);
> +int kernel_setlease(struct file *, int, struct file_lease **, void **);
>  int vfs_setlease(struct file *, int, struct file_lease **, void **);
>  int lease_modify(struct file_lease *, int, struct list_head *);
> =20
> @@ -357,6 +358,12 @@ static inline int generic_setlease(struct file *filp, =
int arg,
>  	return -EINVAL;
>  }
> =20
> +static inline int kernel_setlease(struct file *filp, int arg,
> +			       struct file_lease **lease, void **priv)
> +{
> +	return -EINVAL;
> +}
> +
>  static inline int vfs_setlease(struct file *filp, int arg,
>  			       struct file_lease **lease, void **priv)
>  {
>=20
> ---
> base-commit: 1499e59af376949b062cdc039257f811f6c1697f
> change-id: 20240202-bz2248830-03e6c7506705
>=20
> Best regards,
> --=20
> Jeff Layton <jlayton@kernel.org>
>=20
>=20
>=20