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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240206-gewaschen-bauen-f7932047a1be@brauner>
Date: Tue, 6 Feb 2024 14:12:38 +0100
From: Christian Brauner <brauner@...nel.org>
To: NeilBrown <neilb@...e.de>
Cc: Jeff Layton <jlayton@...nel.org>, 
	Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>, Chuck Lever <chuck.lever@...cle.com>, 
	Olga Kornievskaia <kolga@...app.com>, Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org, 
	Ondrej Mosnáček <omosnacek@...il.com>, Zdenek Pytela <zpytela@...hat.com>
Subject: Re: [PATCH] filelock: don't do security checks on nfsd setlease calls

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@...il.com>
> > Reported-by: Zdenek Pytela <zpytela@...hat.com>
> > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2248830
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> 
> Reviewed-by: NeilBrown <neilb@...e.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@...e.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@...e.de>
Signed-off-by: Christian Brauner <brauner@...nel.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