Re: [PATCH stable 4.19-6.6] filelock: Remove locks reliably when fcntl/close race is detected

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Jul 22, 2024 at 04:22:50PM +0200, Jann Horn wrote:
> commit 3cad1bc010416c6dd780643476bc59ed742436b9 upstream.
> 
> When fcntl_setlk() races with close(), it removes the created lock with
> do_lock_file_wait().
> However, LSMs can allow the first do_lock_file_wait() that created the lock
> while denying the second do_lock_file_wait() that tries to remove the lock.
> In theory (but AFAIK not in practice), posix_lock_file() could also fail to
> remove a lock due to GFP_KERNEL allocation failure (when splitting a range
> in the middle).
> 
> After the bug has been triggered, use-after-free reads will occur in
> lock_get_status() when userspace reads /proc/locks. This can likely be used
> to read arbitrary kernel memory, but can't corrupt kernel memory.
> This only affects systems with SELinux / Smack / AppArmor / BPF-LSM in
> enforcing mode and only works from some security contexts.
> 
> Fix it by calling locks_remove_posix() instead, which is designed to
> reliably get rid of POSIX locks associated with the given file and
> files_struct and is also used by filp_flush().
> 
> Fixes: c293621bbf67 ("[PATCH] stale POSIX lock handling")
> Cc: stable@kernel.org
> Link: https://bugs.chromium.org/p/project-zero/issues/detail?id=2563
> Signed-off-by: Jann Horn <jannh@google.com>
> Link: https://lore.kernel.org/r/20240702-fs-lock-recover-2-v1-1-edd456f63789@google.com
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> [stable fixup: ->c.flc_type was ->fl_type in older kernels]
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  fs/locks.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index fb717dae9029..31659a2d9862 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2381,8 +2381,9 @@ int fcntl_setlk(unsigned int fd, struct file *filp, unsigned int cmd,
>  	error = do_lock_file_wait(filp, cmd, file_lock);
>  
>  	/*
> -	 * Attempt to detect a close/fcntl race and recover by releasing the
> -	 * lock that was just acquired. There is no need to do that when we're
> +	 * Detect close/fcntl races and recover by zapping all POSIX locks
> +	 * associated with this file and our files_struct, just like on
> +	 * filp_flush(). There is no need to do that when we're
>  	 * unlocking though, or for OFD locks.
>  	 */
>  	if (!error && file_lock->fl_type != F_UNLCK &&
> @@ -2397,9 +2398,7 @@ int fcntl_setlk(unsigned int fd, struct file *filp, unsigned int cmd,
>  		f = files_lookup_fd_locked(files, fd);
>  		spin_unlock(&files->file_lock);
>  		if (f != filp) {
> -			file_lock->fl_type = F_UNLCK;
> -			error = do_lock_file_wait(filp, cmd, file_lock);
> -			WARN_ON_ONCE(error);
> +			locks_remove_posix(filp, files);

Wait, this breaks the build on 5.4.y with the error:

fs/locks.c: In function ‘fcntl_setlk’:
fs/locks.c:2545:50: error: ‘files’ undeclared (first use in this function); did you mean ‘file’?
 2545 |                         locks_remove_posix(filp, files);
      |                                                  ^~~~~
      |                                                  file

I didn't do test-builds yesterday, my fault for not noticing this yet.

I've dropped this from the 5.4.y queues for now, can you fix this up and send
an updated version, or give me a hint as to what to do instead?  Odd that this
works on 4.19.y, let me see why...

thanks,

greg k-h