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: Eric Dumazet <edumazet@google.com> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> Acked-by: John Fastabend <john.fastabend@gmail.com> Link: https://lore.kernel.org/r/20230113-sockmap-fix-v2-1-1e0ee7ac2f90@cloudflare.com Signed-off-by: Alexei Starovoitov <ast@kernel.org>
This commit is contained in:
parent
74bc3a5acc
commit
5b4a79ba65
1 changed files with 34 additions and 27 deletions
|
@ -1569,14 +1569,15 @@ 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;
|
saved_unhash = psock->saved_unhash;
|
||||||
sock_map_remove_links(sk, psock);
|
sock_map_remove_links(sk, psock);
|
||||||
rcu_read_unlock();
|
rcu_read_unlock();
|
||||||
|
}
|
||||||
|
if (WARN_ON_ONCE(saved_unhash == sock_map_unhash))
|
||||||
|
return;
|
||||||
|
if (saved_unhash)
|
||||||
saved_unhash(sk);
|
saved_unhash(sk);
|
||||||
}
|
}
|
||||||
EXPORT_SYMBOL_GPL(sock_map_unhash);
|
EXPORT_SYMBOL_GPL(sock_map_unhash);
|
||||||
|
@ -1590,16 +1591,17 @@ 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;
|
saved_destroy = psock->saved_destroy;
|
||||||
sock_map_remove_links(sk, psock);
|
sock_map_remove_links(sk, psock);
|
||||||
rcu_read_unlock();
|
rcu_read_unlock();
|
||||||
sk_psock_stop(psock);
|
sk_psock_stop(psock);
|
||||||
sk_psock_put(sk, psock);
|
sk_psock_put(sk, psock);
|
||||||
|
}
|
||||||
|
if (WARN_ON_ONCE(saved_destroy == sock_map_destroy))
|
||||||
|
return;
|
||||||
|
if (saved_destroy)
|
||||||
saved_destroy(sk);
|
saved_destroy(sk);
|
||||||
}
|
}
|
||||||
EXPORT_SYMBOL_GPL(sock_map_destroy);
|
EXPORT_SYMBOL_GPL(sock_map_destroy);
|
||||||
|
@ -1615,9 +1617,8 @@ 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;
|
saved_close = psock->saved_close;
|
||||||
sock_map_remove_links(sk, psock);
|
sock_map_remove_links(sk, psock);
|
||||||
rcu_read_unlock();
|
rcu_read_unlock();
|
||||||
|
@ -1625,6 +1626,12 @@ void sock_map_close(struct sock *sk, long timeout)
|
||||||
release_sock(sk);
|
release_sock(sk);
|
||||||
cancel_work_sync(&psock->work);
|
cancel_work_sync(&psock->work);
|
||||||
sk_psock_put(sk, psock);
|
sk_psock_put(sk, psock);
|
||||||
|
}
|
||||||
|
/* Make sure we do not recurse. This is a bug.
|
||||||
|
* Leak the socket instead of crashing on a stack overflow.
|
||||||
|
*/
|
||||||
|
if (WARN_ON_ONCE(saved_close == sock_map_close))
|
||||||
|
return;
|
||||||
saved_close(sk, timeout);
|
saved_close(sk, timeout);
|
||||||
}
|
}
|
||||||
EXPORT_SYMBOL_GPL(sock_map_close);
|
EXPORT_SYMBOL_GPL(sock_map_close);
|
||||||
|
|
Loading…
Add table
Reference in a new issue