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 <[email protected]>
> Reported-by: Zdenek Pytela <[email protected]>
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830
> Signed-off-by: Jeff Layton <[email protected]>
Reviewed-by: NeilBrown <[email protected]>
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
> ---
> 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 <[email protected]>
>
>
>
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:
> >
> > 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 <[email protected]>
> > Reported-by: Zdenek Pytela <[email protected]>
> > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830
> > Signed-off-by: Jeff Layton <[email protected]>
>
> Reviewed-by: NeilBrown <[email protected]>
>
> 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.
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:
From d30e52329760873bf0d7984a442cace3a4b5f39d Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Tue, 6 Feb 2024 14:08:57 +0100
Subject: [PATCH] smb: remove redundant check
->setlease() is never called on non-regular files now. So remove the
check from cifs_setlease().
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: NeilBrown <[email protected]>
Signed-off-by: Christian Brauner <[email protected]>
---
fs/smb/client/cifsfs.c | 3 ---
1 file changed, 3 deletions(-)
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 = 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))) ||
--
2.43.0