Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2] locks: Fix dropped call to ->fl_release_private() | From | "Iwashima, Kuniyuki" <> | Date | Wed, 17 Aug 2022 18:47:54 +0000 |
| |
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> >
| |