[PATCH bpf 0/2] Two BPF sockmap fixes

[PATCH bpf 0/2] Two BPF sockmap fixes

[Daniel Borkmann]

This series contains two fixes found while working with the BPF
sockmap code. One refcount leakage and one case for a small window
of use after free. See patches for more info. Thanks!

Daniel Borkmann (2):
  bpf, sockmap: fix potential use after free in bpf_tcp_close
  bpf, sockmap: fix psock refcount leak in bpf_tcp_recvmsg

 kernel/bpf/sockmap.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

-- 
2.9.5

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

[Daniel Borkmann]

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>
---
 kernel/bpf/sockmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index cf5195c..01879e4 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -369,7 +369,7 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
 			/* If another thread deleted this object skip deletion.
 			 * The refcnt on psock may or may not be zero.
 			 */
-			if (l) {
+			if (l && l == link) {
 				hlist_del_rcu(&link->hash_node);
 				smap_release_sock(psock, link->sk);
 				free_htab_elem(htab, link);
-- 
2.9.5

[PATCH bpf 2/2] bpf, sockmap: fix psock refcount leak in bpf_tcp_recvmsg

[Daniel Borkmann]

In bpf_tcp_recvmsg() we first took a reference on the psock, however
once we find that there are skbs in the normal socket's receive queue
we return with processing them through tcp_recvmsg(). Problem is that
we leak the taken reference on the psock in that path. Given we don't
really do anything with the psock at this point, move the skb_queue_empty()
test before we fetch the psock to fix this case.

Fixes: 8934ce2fd081 ("bpf: sockmap redirect ingress support")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/sockmap.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 01879e4..26d8a30 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -912,6 +912,8 @@ static int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 
 	if (unlikely(flags & MSG_ERRQUEUE))
 		return inet_recv_error(sk, msg, len, addr_len);
+	if (!skb_queue_empty(&sk->sk_receive_queue))
+		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
 
 	rcu_read_lock();
 	psock = smap_psock_sk(sk);
@@ -922,9 +924,6 @@ static int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 		goto out;
 	rcu_read_unlock();
 
-	if (!skb_queue_empty(&sk->sk_receive_queue))
-		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
-
 	lock_sock(sk);
 bytes_ready:
 	while (copied != len) {
-- 
2.9.5

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

[John Fastabend]

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>

Re: [PATCH bpf 2/2] bpf, sockmap: fix psock refcount leak in bpf_tcp_recvmsg

[John Fastabend]

On 08/24/2018 01:08 PM, Daniel Borkmann wrote:
> In bpf_tcp_recvmsg() we first took a reference on the psock, however
> once we find that there are skbs in the normal socket's receive queue
> we return with processing them through tcp_recvmsg(). Problem is that
> we leak the taken reference on the psock in that path. Given we don't
> really do anything with the psock at this point, move the skb_queue_empty()
> test before we fetch the psock to fix this case.
> 
> Fixes: 8934ce2fd081 ("bpf: sockmap redirect ingress support")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---

Oops. Thanks for catching this, all of our use cases and tests
to this point either always did redirect or normal receive and
I missed the mixed case. I'll write a tests for bpf-next to also
ensure we catch anything else.

Thanks.

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

Re: [PATCH bpf 0/2] Two BPF sockmap fixes

[Alexei Starovoitov]

On Fri, Aug 24, 2018 at 10:08:49PM +0200, Daniel Borkmann wrote:
> This series contains two fixes found while working with the BPF
> sockmap code. One refcount leakage and one case for a small window
> of use after free. See patches for more info. Thanks!

Applied, Thanks