On Tue, Jul 23, 2024 at 3:52 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@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.
>
> 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
