On Tue, Jul 23, 2024 at 3:09 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx
> > > > Link: https://bugs.chromium.org/p/project-zero/issues/detail?id=2563
> > > > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> > > > Link: https://lore.kernel.org/r/20240702-fs-lock-recover-2-v1-1-edd456f63789@xxxxxxxxxx
> > > > Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> > > > [stable fixup: ->c.flc_type was ->fl_type in older kernels]
> > > > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> > > > ---
> > > > 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.
