| Message ID | 20240205-bz2248830-v1-1-d0ec0daecba1@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | filelock: don't do security checks on nfsd setlease calls | expand |
On Mon, 05 Feb 2024 07:09:31 -0500, 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. > > [...] Applied to the vfs.file branch of the vfs/vfs.git tree. Patches in the vfs.file branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.file [1/1] filelock: don't do security checks on nfsd setlease calls https://git.kernel.org/vfs/vfs/c/ec457463e0e3
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@gmail.com> > Reported-by: Zdenek Pytela <zpytela@redhat.com> > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830 > 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_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 <jlayton@kernel.org> > > >
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@gmail.com> > > Reported-by: Zdenek Pytela <zpytela@redhat.com> > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830 > > 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_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! > > --- > > 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@kernel.org> > > > > > > >
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@gmail.com> >>> Reported-by: Zdenek Pytela <zpytela@redhat.com> >>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830 >>> 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_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! 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@talpey.com> 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@kernel.org> >>> >>> >>> >> >
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@gmail.com> >>> Reported-by: Zdenek Pytela <zpytela@redhat.com> >>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830 >>> 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_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@talpey.com> 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@kernel.org> >>> >>> >>> >> >
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 <omosnacek@gmail.com> > > Reported-by: Zdenek Pytela <zpytela@redhat.com> > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830 > > 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_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 <neilb@suse.de> 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/170716318935.13976.13465352731929804157@noble.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(-) 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))) ||
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: > > > > > > 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@gmail.com> > > > Reported-by: Zdenek Pytela <zpytela@redhat.com> > > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830 > > > 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_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: No problems at all - thanks for doing this! NeilBrown > > 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 > > ->setlease() is never called on non-regular files now. So remove the > check from cifs_setlease(). > > Link: https://lore.kernel.org/r/170716318935.13976.13465352731929804157@noble.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(-) > > 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 > >
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) {
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@gmail.com> Reported-by: Zdenek Pytela <zpytela@redhat.com> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830 Signed-off-by: Jeff Layton <jlayton@kernel.org> --- 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(-) --- base-commit: 1499e59af376949b062cdc039257f811f6c1697f change-id: 20240202-bz2248830-03e6c7506705 Best regards,