Messages in this thread | | | Date | Mon, 5 Feb 2024 17:22:26 -0500 | Subject | Re: [PATCH] filelock: don't do security checks on nfsd setlease calls | From | Tom Talpey <> |
| |
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> >>> >>> >>> >> >
| |