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

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

[Jann Horn]

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);
 			error = -EBADF;
 		}
 	}

base-commit: 2eaf5c0d81911ba05bace3a722cbcd708fdbbcba
-- 
2.45.2.1089.g2a221341d9-goog

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

[Greg KH]

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(-)

Now queued up, thanks.

greg k-h

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

[Greg KH]

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

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

[Jann Horn]

On Tue, Jul 23, 2024 at 2:56 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> 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.

Ugh, sorry, I think maybe I only test-built on 6.6...

> 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?

Yeah, I'll have a look.

> Odd that this
> works on 4.19.y, let me see why...

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

[Greg KH]

On Tue, Jul 23, 2024 at 02:56:08PM +0200, Greg KH wrote:
> 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...

Ah, I see why, it applied to the wrong function in 4.19 and that didn't
get built on my test systems (i.e. 64bit only.)  And I see how to fix
this up, let me go do that now, sorry for the noise.

greg k-h

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

[Greg KH]

On Tue, Jul 23, 2024 at 03:00:28PM +0200, Greg KH wrote:
> On Tue, Jul 23, 2024 at 02:56:08PM +0200, Greg KH wrote:
> > 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...
> 
> Ah, I see why, it applied to the wrong function in 4.19 and that didn't
> get built on my test systems (i.e. 64bit only.)  And I see how to fix
> this up, let me go do that now, sorry for the noise.

And it's fixed now on 5.4.y as well, I just reference current->files and
all is good.

greg k-h

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

[Jann Horn]

On Tue, Jul 23, 2024 at 3:09 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jul 23, 2024 at 03:00:28PM +0200, Greg KH wrote:
> > On Tue, Jul 23, 2024 at 02:56:08PM +0200, Greg KH wrote:
> > > 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...
> >
> > Ah, I see why, it applied to the wrong function in 4.19 and that didn't
> > get built on my test systems (i.e. 64bit only.)  And I see how to fix
> > this up, let me go do that now, sorry for the noise.
>
> And it's fixed now on 5.4.y as well, I just reference current->files and
> all is good.

Uuuugh, but actually as you mentioned the buggy code is duplicated
(which was why you had that build success for 4.19). Even in mainline
there are two versions and I missed the one for 64-bit offsets on
32-bit systems.

So I guess I gotta go back and send another patch to mainline for the
second path, and then get that through stable too... bleh.

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

[Greg KH]

On Tue, Jul 23, 2024 at 03:36:07PM +0200, Jann Horn wrote:
> On Tue, Jul 23, 2024 at 3:09 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jul 23, 2024 at 03:00:28PM +0200, Greg KH wrote:
> > > On Tue, Jul 23, 2024 at 02:56:08PM +0200, Greg KH wrote:
> > > > 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...
> > >
> > > Ah, I see why, it applied to the wrong function in 4.19 and that didn't
> > > get built on my test systems (i.e. 64bit only.)  And I see how to fix
> > > this up, let me go do that now, sorry for the noise.
> >
> > And it's fixed now on 5.4.y as well, I just reference current->files and
> > all is good.
> 
> Uuuugh, but actually as you mentioned the buggy code is duplicated
> (which was why you had that build success for 4.19). Even in mainline
> there are two versions and I missed the one for 64-bit offsets on
> 32-bit systems.
> 
> So I guess I gotta go back and send another patch to mainline for the
> second path, and then get that through stable too... bleh.

Hey, the stable review process found a bug in mainline, that's a good
thing!  :)

If you need help backporting, I'm glad to do so.

thanks,

greg k-h

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

[Jann Horn]

On Tue, Jul 23, 2024 at 3:52 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Jul 23, 2024 at 03:36:07PM +0200, Jann Horn wrote:
> > On Tue, Jul 23, 2024 at 3:09 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Jul 23, 2024 at 03:00:28PM +0200, Greg KH wrote:
> > > > On Tue, Jul 23, 2024 at 02:56:08PM +0200, Greg KH wrote:
> > > > > 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...
> > > >
> > > > Ah, I see why, it applied to the wrong function in 4.19 and that didn't
> > > > get built on my test systems (i.e. 64bit only.)  And I see how to fix
> > > > this up, let me go do that now, sorry for the noise.
> > >
> > > And it's fixed now on 5.4.y as well, I just reference current->files and
> > > all is good.
> >
> > Uuuugh, but actually as you mentioned the buggy code is duplicated
> > (which was why you had that build success for 4.19). Even in mainline
> > there are two versions and I missed the one for 64-bit offsets on
> > 32-bit systems.
> >
> > So I guess I gotta go back and send another patch to mainline for the
> > second path, and then get that through stable too... bleh.
>
> Hey, the stable review process found a bug in mainline, that's a good
> thing!  :)

Yeah, I'm just a bit mad at myself-from-three-weeks-ago for not
noticing this, I know I must have seen that compat code several
times...

I've sent a patch to the VFS folks now for fixing the compat path in
mainline, with CC stable.

> If you need help backporting, I'm glad to do so.

Thanks for the help!

>
> thanks,
>
> greg k-h