Re: [PATCH] filelock: don't do security checks on nfsd setlease calls

[Tom Talpey <tom@xxxxxxxxxx>]

On 2/5/2024 3:16 PM, Jeff Layton wrote:
> On Tue, 2024-02-06 at 06:59 +1100, NeilBrown wrote:
> > On Mon, 05 Feb 2024, Jeff Layton wrote:
> > > Zdenek reported seeing some AVC denials due to nfsd trying to set
> > > delegations:
> > >
> > >      type=AVC msg=audit(09.11.2023 09:03:46.411:496) : avc:  denied  { lease } for  pid=5127 comm=rpc.nfsd capability=lease  scontext=system_u:system_r:nfsd_t:s0 tcontext=system_u:system_r:nfsd_t:s0 tclass=capability permissive=0
> > >
> > > 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.
> > >
> > > 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.
> > >
> > > 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.
> > >
> > > 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.
> > >
> > > 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.
> > >
> > > Cc: Ondrej Mosnáček <omosnacek@xxxxxxxxx>
> > > Reported-by: Zdenek Pytela <zpytela@xxxxxxxxxx>
> > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> >
> > Reviewed-by: NeilBrown <neilb@xxxxxxx>
> >
> > 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_lock **lease, void **priv)
> >   	struct inode *inode = file_inode(file);
> >   	struct cifsFileInfo *cfile = file->private_data;
> >   
> > -	if (!(S_ISREG(inode->i_mode)))
> > -		return -EINVAL;
> > -
> >   	/* Check if file is oplocked if this is request for new lease */
> >   	if (arg == F_UNLCK ||
> >   	    ((arg == F_RDLCK) && CIFS_CACHE_READ(CIFS_I(inode))) ||
> >
> >
> > as ->setlease() is now never called for non-ISREG files.
> >
> > NeilBrown
>
> Ahh yeah. Good point. I'm fine with including that if Christian wants to
> fold it in.
>
> Thanks for the review!

[re-replying to fix cc linux-cifs@ /vger/. kernel.org ...]

cifsfs.c is a new file being added to the change, cc-ing linux-cifs
for awareness. The SMB3 protocol does support leases on directories, so
this vfs change might be important to note in the future.

In the meantime, for cifsfs.c:

Acked-by: Tom Talpey <tom@xxxxxxxxxx>

Tom.

> > > 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(-)
> > >
> > > 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, void *owner)
> > >   int generic_setlease(struct file *filp, int arg, struct file_lease **flp,
> > >   			void **priv)
> > >   {
> > > -	struct inode *inode = file_inode(filp);
> > > -	vfsuid_t vfsuid = 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 = 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);
> > >   
> > > +
> > > +int
> > > +kernel_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);
> > > +}
> > > +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 = file_inode(filp);
> > > +	vfsuid_t vfsuid = 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 = security_file_lock(filp, arg);
> > > +	if (error)
> > > +		return error;
> > > +	return kernel_setlease(filp, arg, lease, priv);
> > >   }
> > >   EXPORT_SYMBOL_GPL(vfs_setlease);
> > >   
> > > 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);
> > >   
> > >   	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);
> > >   
> > >   	if (ls->ls_recalled)
> > > @@ -199,8 +199,7 @@ nfsd4_layout_setlease(struct nfs4_layout_stateid *ls)
> > >   	fl->c.flc_pid = current->tgid;
> > >   	fl->c.flc_file = ls->ls_file->nf_file;
> > >   
> > > -	status = vfs_setlease(fl->c.flc_file, fl->c.flc_type, &fl,
> > > -			      NULL);
> > > +	status = 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_delegation *dp)
> > >   
> > >   	WARN_ON_ONCE(!fp->fi_delegees);
> > >   
> > > -	vfs_setlease(nf->nf_file, F_UNLCK, NULL, (void **)&dp);
> > > +	kernel_setlease(nf->nf_file, F_UNLCK, NULL, (void **)&dp);
> > >   	put_deleg_file(fp);
> > >   }
> > >   
> > > @@ -5532,8 +5532,8 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > >   	if (!fl)
> > >   		goto out_clnt_odstate;
> > >   
> > > -	status = vfs_setlease(fp->fi_deleg_file->nf_file,
> > > -			      fl->c.flc_type, &fl, NULL);
> > > +	status = 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 nfs4_ol_stateid *stp,
> > >   
> > >   	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 type);
> > >   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 *);
> > >   
> > > @@ -357,6 +358,12 @@ static inline int generic_setlease(struct file *filp, int arg,
> > >   	return -EINVAL;
> > >   }
> > >   
> > > +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)
> > >   {
> > >
> > > ---
> > > base-commit: 1499e59af376949b062cdc039257f811f6c1697f
> > > change-id: 20240202-bz2248830-03e6c7506705
> > >
> > > Best regards,
> > > --
> > > Jeff Layton <jlayton@xxxxxxxxxx>