linux-kernel - Re: [PATCH] locks: Fix dropped call to ->fl_release

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8048714f47f19944a95a8703086069369bb7193e.camel@kernel.org>
Date:   Wed, 17 Aug 2022 14:02:51 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     David Howells <dhowells@...hat.com>
Cc:     Kuniyuki Iwashima <kuniyu@...zon.com>,
        Chuck Lever <chuck.lever@...cle.com>,
        Marc Dionne <marc.dionne@...istor.com>,
        linux-afs@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] locks: Fix dropped call to ->fl_release_private()

On Wed, 2022-08-17 at 18:33 +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.
> 
> Fixes: 4149be7bda7e ("fs/lock: Don't allocate file_lock in flock_make_lock().")
> cc: Kuniyuki Iwashima <kuniyu@...zon.com>
> cc: Chuck Lever <chuck.lever@...cle.com>
> cc: Jeff Layton <jlayton@...nel.org>
> cc: Marc Dionne <marc.dionne@...istor.com>
> cc: linux-afs@...ts.infradead.org
> cc: linux-fsdevel@...r.kernel.org
> ---
> 
>  fs/locks.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index c266cfdc3291..f2d5aca782c6 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2116,7 +2116,7 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>  
>  	error = security_file_lock(f.file, fl.fl_type);
>  	if (error)
> -		goto out_putf;
> +		goto out_release;

It probably doesn't hurt anything, but I don't think it's necessary to
call locks_release_private if the ->flock op was never called.

>  
>  	can_sleep = !(cmd & LOCK_NB);
>  	if (can_sleep)
> @@ -2128,7 +2128,8 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>  					    &fl);
>  	else
>  		error = locks_lock_file_wait(f.file, &fl);
> -
> +out_release:
> +	locks_release_private(&fl);
^^^
I think we just need the above line.

>   out_putf:
>  	fdput(f);
>  
> 
> 

-- 
Jeff Layton <jlayton@...nel.org>