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

[John Fastabend <john.fastabend@gmail.com>]

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@iogearbox.net>
> ---

Looks good to me, nice catch by the way.

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