* [PATCH v2] locks: Fix dropped call to ->fl_release_private()
@ 2022-08-17 18:41 David Howells
2022-08-17 18:42 ` Jeff Layton
2022-08-17 19:00 ` David Howells
0 siblings, 2 replies; 4+ messages in thread
From: David Howells @ 2022-08-17 18:41 UTC (permalink / raw)
To: jlayton
Cc: Kuniyuki Iwashima, Chuck Lever, Marc Dionne, linux-afs,
linux-fsdevel, dhowells, linux-kernel
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 <kuniyu@amazon.com>
cc: Chuck Lever <chuck.lever@oracle.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
cc: linux-fsdevel@vger.kernel.org
Link: https://lore.kernel.org/r/166075758809.3532462.13307935588777587536.stgit@warthog.procyon.org.uk/ # 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);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] locks: Fix dropped call to ->fl_release_private()
2022-08-17 18:41 [PATCH v2] locks: Fix dropped call to ->fl_release_private() David Howells
@ 2022-08-17 18:42 ` Jeff Layton
2022-08-17 18:47 ` Iwashima, Kuniyuki
2022-08-17 19:00 ` David Howells
1 sibling, 1 reply; 4+ messages in thread
From: Jeff Layton @ 2022-08-17 18:42 UTC (permalink / raw)
To: David Howells
Cc: Kuniyuki Iwashima, Chuck Lever, Marc Dionne, linux-afs,
linux-fsdevel, linux-kernel
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 <kuniyu@amazon.com>
> cc: Chuck Lever <chuck.lever@oracle.com>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> cc: linux-fsdevel@vger.kernel.org
> Link: https://lore.kernel.org/r/166075758809.3532462.13307935588777587536.stgit@warthog.procyon.org.uk/ # 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!
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] locks: Fix dropped call to ->fl_release_private()
2022-08-17 18:42 ` Jeff Layton
@ 2022-08-17 18:47 ` Iwashima, Kuniyuki
0 siblings, 0 replies; 4+ messages in thread
From: Iwashima, Kuniyuki @ 2022-08-17 18:47 UTC (permalink / raw)
To: Jeff Layton, David Howells
Cc: Chuck Lever, Marc Dionne, linux-afs, linux-fsdevel, linux-kernel,
Iwashima, Kuniyuki
From: Jeff Layton <jlayton@kernel.org>
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 <kuniyu@amazon.com>
> > cc: Chuck Lever <chuck.lever@oracle.com>
> > cc: Jeff Layton <jlayton@kernel.org>
> > cc: Marc Dionne <marc.dionne@auristor.com>
> > cc: linux-afs@lists.infradead.org
> > cc: linux-fsdevel@vger.kernel.org
> > Link: https://lore.kernel.org/r/166075758809.3532462.13307935588777587536.stgit@warthog.procyon.org.uk/ # v1
nit: missing Signed-off-by :)
Acked-by: Kuniyuki Iwashima <kuniyu@amazon.com>
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 <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] locks: Fix dropped call to ->fl_release_private()
2022-08-17 18:41 [PATCH v2] locks: Fix dropped call to ->fl_release_private() David Howells
2022-08-17 18:42 ` Jeff Layton
@ 2022-08-17 19:00 ` David Howells
1 sibling, 0 replies; 4+ messages in thread
From: David Howells @ 2022-08-17 19:00 UTC (permalink / raw)
To: jlayton
Cc: dhowells, Kuniyuki Iwashima, Chuck Lever, Marc Dionne, linux-afs,
linux-fsdevel, linux-kernel
David Howells <dhowells@redhat.com> wrote:
> Fixes: 4149be7bda7e ("fs/lock: Don't allocate file_lock in flock_make_lock().")
> cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> cc: Chuck Lever <chuck.lever@oracle.com>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> cc: linux-fsdevel@vger.kernel.org
> Link: https://lore.kernel.org/r/166075758809.3532462.13307935588777587536.stgit@warthog.procyon.org.uk/ # v1
I forgot:
Signed-off-by: David Howells <dhowells@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-17 19:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-17 18:41 [PATCH v2] locks: Fix dropped call to ->fl_release_private() David Howells
2022-08-17 18:42 ` Jeff Layton
2022-08-17 18:47 ` Iwashima, Kuniyuki
2022-08-17 19:00 ` David Howells
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).