Commit 5b4a79ba authored by Jakub Sitnicki's avatar Jakub Sitnicki Committed by Alexei Starovoitov

bpf, sockmap: Don't let sock_map_{close,destroy,unhash} call itself

sock_map proto callbacks should never call themselves by design. Protect
against bugs like [1] and break out of the recursive loop to avoid a stack
overflow in favor of a resource leak.

[1] https://lore.kernel.org/all/00000000000073b14905ef2e7401@google.com/Suggested-by: default avatarEric Dumazet <edumazet@google.com>
Signed-off-by: default avatarJakub Sitnicki <jakub@cloudflare.com>
Acked-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
Link: https://lore.kernel.org/r/20230113-sockmap-fix-v2-1-1e0ee7ac2f90@cloudflare.comSigned-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
parent 74bc3a5a
...@@ -1569,15 +1569,16 @@ void sock_map_unhash(struct sock *sk) ...@@ -1569,15 +1569,16 @@ void sock_map_unhash(struct sock *sk)
psock = sk_psock(sk); psock = sk_psock(sk);
if (unlikely(!psock)) { if (unlikely(!psock)) {
rcu_read_unlock(); rcu_read_unlock();
if (sk->sk_prot->unhash) saved_unhash = READ_ONCE(sk->sk_prot)->unhash;
sk->sk_prot->unhash(sk); } else {
return; saved_unhash = psock->saved_unhash;
sock_map_remove_links(sk, psock);
rcu_read_unlock();
} }
if (WARN_ON_ONCE(saved_unhash == sock_map_unhash))
saved_unhash = psock->saved_unhash; return;
sock_map_remove_links(sk, psock); if (saved_unhash)
rcu_read_unlock(); saved_unhash(sk);
saved_unhash(sk);
} }
EXPORT_SYMBOL_GPL(sock_map_unhash); EXPORT_SYMBOL_GPL(sock_map_unhash);
...@@ -1590,17 +1591,18 @@ void sock_map_destroy(struct sock *sk) ...@@ -1590,17 +1591,18 @@ void sock_map_destroy(struct sock *sk)
psock = sk_psock_get(sk); psock = sk_psock_get(sk);
if (unlikely(!psock)) { if (unlikely(!psock)) {
rcu_read_unlock(); rcu_read_unlock();
if (sk->sk_prot->destroy) saved_destroy = READ_ONCE(sk->sk_prot)->destroy;
sk->sk_prot->destroy(sk); } else {
return; saved_destroy = psock->saved_destroy;
sock_map_remove_links(sk, psock);
rcu_read_unlock();
sk_psock_stop(psock);
sk_psock_put(sk, psock);
} }
if (WARN_ON_ONCE(saved_destroy == sock_map_destroy))
saved_destroy = psock->saved_destroy; return;
sock_map_remove_links(sk, psock); if (saved_destroy)
rcu_read_unlock(); saved_destroy(sk);
sk_psock_stop(psock);
sk_psock_put(sk, psock);
saved_destroy(sk);
} }
EXPORT_SYMBOL_GPL(sock_map_destroy); EXPORT_SYMBOL_GPL(sock_map_destroy);
...@@ -1615,16 +1617,21 @@ void sock_map_close(struct sock *sk, long timeout) ...@@ -1615,16 +1617,21 @@ void sock_map_close(struct sock *sk, long timeout)
if (unlikely(!psock)) { if (unlikely(!psock)) {
rcu_read_unlock(); rcu_read_unlock();
release_sock(sk); release_sock(sk);
return sk->sk_prot->close(sk, timeout); saved_close = READ_ONCE(sk->sk_prot)->close;
} else {
saved_close = psock->saved_close;
sock_map_remove_links(sk, psock);
rcu_read_unlock();
sk_psock_stop(psock);
release_sock(sk);
cancel_work_sync(&psock->work);
sk_psock_put(sk, psock);
} }
/* Make sure we do not recurse. This is a bug.
saved_close = psock->saved_close; * Leak the socket instead of crashing on a stack overflow.
sock_map_remove_links(sk, psock); */
rcu_read_unlock(); if (WARN_ON_ONCE(saved_close == sock_map_close))
sk_psock_stop(psock); return;
release_sock(sk);
cancel_work_sync(&psock->work);
sk_psock_put(sk, psock);
saved_close(sk, timeout); saved_close(sk, timeout);
} }
EXPORT_SYMBOL_GPL(sock_map_close); EXPORT_SYMBOL_GPL(sock_map_close);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment