netdev - Re: [PATCH bpf 1/2] bpf, sockmap: fix potential use after free in bpf_tcp

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <42a094b6-baaa-6d03-8e25-9ec7e15cee30@gmail.com>
Date:   Fri, 24 Aug 2018 17:25:57 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        alexei.starovoitov@...il.com
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH bpf 1/2] bpf, sockmap: fix potential use after free in
 bpf_tcp_close

On 08/24/2018 01:08 PM, Daniel Borkmann wrote:
> bpf_tcp_close() we pop the psock linkage to a map via psock_map_pop().
> A parallel update on the sock hash map can happen between psock_map_pop()
> and lookup_elem_raw() where we override the element under link->hash /
> link->key. In bpf_tcp_close()'s lookup_elem_raw() we subsequently only
> test whether an element is present, but we do not test whether the
> element is infact the element we were looking for.
> 
> We lock the sock in bpf_tcp_close() during that time, so do we hold
> the lock in sock_hash_update_elem(). However, the latter locks the
> sock which is newly updated, not the one we're purging from the hash
> table. This means that while one CPU is doing the lookup from bpf_tcp_close(),
> another CPU is doing the map update in parallel, dropped our sock from
> the hlist and released the psock.
> 
> Subsequently the first CPU will find the new sock and attempts to drop
> and release the old sock yet another time. Fix is that we need to check
> the elements for a match after lookup, similar as we do in the sock map.
> Note that the hash tab elems are freed via RCU, so access to their
> link->hash / link->key is fine since we're under RCU read side there.
> 
> Fixes: e9db4ef6bf4c ("bpf: sockhash fix omitted bucket lock in sock_close")
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> ---

Looks good to me, nice catch by the way.

Acked-by: John Fastabend <john.fastabend@...il.com>