[v2] locks: Fix dropped call to ->fl_release_private()

Commit Message

David Howells Aug. 17, 2022, 6:41 p.m. UTC

Prior to commit 4149be7bda7e, sys_flock() would allocate the file_lock
struct it was going to use to pass parameters, call ->flock() and then call
locks_free_lock() to get rid of it - which had the side effect of calling
locks_release_private() and thus ->fl_release_private().

With commit 4149be7bda7e, however, this is no longer the case: the struct
is now allocated on the stack, and locks_free_lock() is no longer called -
and thus any remaining private data doesn't get cleaned up either.

This causes afs flock to cause oops.  Kasan catches this as a UAF by the
list_del_init() in afs_fl_release_private() for the file_lock record
produced by afs_fl_copy_lock() as the original record didn't get delisted.
It can be reproduced using the generic/504 xfstest.

Fix this by reinstating the locks_release_private() call in sys_flock().
I'm not sure if this would affect any other filesystems.  If not, then the
release could be done in afs_flock() instead.

Changes
=======
ver #2)
 - Don't need to call ->fl_release_private() after calling the security
   hook, only after calling ->flock().

Fixes: 4149be7bda7e ("fs/lock: Don't allocate file_lock in flock_make_lock().")
cc: Kuniyuki Iwashima <[e-mail]>
cc: Chuck Lever <[e-mail]>
cc: Jeff Layton <[e-mail]>
cc: Marc Dionne <[e-mail]>
cc: [e-mail]
cc: [e-mail]
Link: https://lore.kernel.org/r/[e-mail]/ # v1
---

 fs/locks.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Jeff Layton Aug. 17, 2022, 6:42 p.m. UTC | #1

On Wed, 2022-08-17 at 19:41 +0100, David Howells wrote:
> Prior to commit 4149be7bda7e, sys_flock() would allocate the file_lock
> struct it was going to use to pass parameters, call ->flock() and then call
> locks_free_lock() to get rid of it - which had the side effect of calling
> locks_release_private() and thus ->fl_release_private().
> 
> With commit 4149be7bda7e, however, this is no longer the case: the struct
> is now allocated on the stack, and locks_free_lock() is no longer called -
> and thus any remaining private data doesn't get cleaned up either.
> 
> This causes afs flock to cause oops.  Kasan catches this as a UAF by the
> list_del_init() in afs_fl_release_private() for the file_lock record
> produced by afs_fl_copy_lock() as the original record didn't get delisted.
> It can be reproduced using the generic/504 xfstest.
> 
> Fix this by reinstating the locks_release_private() call in sys_flock().
> I'm not sure if this would affect any other filesystems.  If not, then the
> release could be done in afs_flock() instead.
> 
> Changes
> =======
> ver #2)
>  - Don't need to call ->fl_release_private() after calling the security
>    hook, only after calling ->flock().
> 
> Fixes: 4149be7bda7e ("fs/lock: Don't allocate file_lock in flock_make_lock().")
> cc: Kuniyuki Iwashima <[e-mail]>
> cc: Chuck Lever <[e-mail]>
> cc: Jeff Layton <[e-mail]>
> cc: Marc Dionne <[e-mail]>
> cc: [e-mail]
> cc: [e-mail]
> Link: https://lore.kernel.org/r/[e-mail]/ # v1
> ---
> 
>  fs/locks.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index c266cfdc3291..607f94a0e789 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2129,6 +2129,7 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>  	else
>  		error = locks_lock_file_wait(f.file, &fl);
>  
> +	locks_release_private(&fl);
>   out_putf:
>  	fdput(f);
>  
> 
> 

Looks good. I'll get this into -next and plan to get it up to Linus
soon.

Thanks!

Iwashima, Kuniyuki Aug. 17, 2022, 6:47 p.m. UTC | #2

From: Jeff Layton <[e-mail]>
Date: Wed, 17 Aug 2022 14:42:57 -0400
> On Wed, 2022-08-17 at 19:41 +0100, David Howells wrote:
> > Prior to commit 4149be7bda7e, sys_flock() would allocate the file_lock
> > struct it was going to use to pass parameters, call ->flock() and then call
> > locks_free_lock() to get rid of it - which had the side effect of calling
> > locks_release_private() and thus ->fl_release_private().
> >
> > With commit 4149be7bda7e, however, this is no longer the case: the struct
> > is now allocated on the stack, and locks_free_lock() is no longer called -
> > and thus any remaining private data doesn't get cleaned up either.
> >
> > This causes afs flock to cause oops.  Kasan catches this as a UAF by the
> > list_del_init() in afs_fl_release_private() for the file_lock record
> > produced by afs_fl_copy_lock() as the original record didn't get delisted.
> > It can be reproduced using the generic/504 xfstest.
> >
> > Fix this by reinstating the locks_release_private() call in sys_flock().
> > I'm not sure if this would affect any other filesystems.  If not, then the
> > release could be done in afs_flock() instead.
> >
> > Changes
> > =======
> > ver #2)
> >  - Don't need to call ->fl_release_private() after calling the security
> >    hook, only after calling ->flock().
> >
> > Fixes: 4149be7bda7e ("fs/lock: Don't allocate file_lock in flock_make_lock().")
> > cc: Kuniyuki Iwashima <[e-mail]>
> > cc: Chuck Lever <[e-mail]>
> > cc: Jeff Layton <[e-mail]>
> > cc: Marc Dionne <[e-mail]>
> > cc: [e-mail]
> > cc: [e-mail]
> > Link: https://lore.kernel.org/r/[e-mail]/ # v1

nit: missing Signed-off-by :)

Acked-by: Kuniyuki Iwashima <[e-mail]>

Thanks for the fix!


> > ---
> >
> >  fs/locks.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/locks.c b/fs/locks.c
> > index c266cfdc3291..607f94a0e789 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -2129,6 +2129,7 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
> >       else
> >               error = locks_lock_file_wait(f.file, &fl);
> >
> > +     locks_release_private(&fl);
> >   out_putf:
> >       fdput(f);
> >
> >
> >
> 
> Looks good. I'll get this into -next and plan to get it up to Linus
> soon.
> 
> Thanks!
> --
> Jeff Layton <[e-mail]>
>

David Howells Aug. 17, 2022, 7 p.m. UTC | #3

David Howells <[e-mail]> wrote:

> Fixes: 4149be7bda7e ("fs/lock: Don't allocate file_lock in flock_make_lock().")
> cc: Kuniyuki Iwashima <[e-mail]>
> cc: Chuck Lever <[e-mail]>
> cc: Jeff Layton <[e-mail]>
> cc: Marc Dionne <[e-mail]>
> cc: [e-mail]
> cc: [e-mail]
> Link: https://lore.kernel.org/r/[e-mail]/ # v1

I forgot:

Signed-off-by: David Howells <[e-mail]>

Patch

diff --git a/fs/locks.c b/fs/locks.c
index c266cfdc3291..607f94a0e789 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2129,6 +2129,7 @@  SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
 	else
 		error = locks_lock_file_wait(f.file, &fl);
 
+	locks_release_private(&fl);
  out_putf:
 	fdput(f);