Re: [PATCH v1 net] sctp: Fix null-ptr-deref in reuseport_add_sock().
From: Dmitry Antipov <hidden>
Date: 2024-07-30 13:46:15
Also in:
linux-sctp
Subsystem:
networking [general], sctp protocol, the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Marcelo Ricardo Leitner, Xin Long, Linus Torvalds
Hm. Note both 'reuseport_add_sock()' and 'reuseport_detach_sock()' uses global 'reuseport_lock' internally. So what about using read lock to protect 'sctp_for_each_entry()' loop and upgrade to write lock only if hash bucket should be actually updated? E.g.:
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 17fcaa9b0df9..4fbff388b1b4 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c@@ -735,6 +735,7 @@ static int __sctp_hash_endpoint(struct sctp_endpoint *ep) struct sock *sk = ep->base.sk; struct net *net = sock_net(sk); struct sctp_hashbucket *head; + int err = 0; ep->hashent = sctp_ep_hashfn(net, ep->base.bind_addr.port); head = &sctp_ep_hashtable[ep->hashent];
@@ -743,11 +744,14 @@ static int __sctp_hash_endpoint(struct sctp_endpoint *ep) bool any = sctp_is_ep_boundall(sk); struct sctp_endpoint *ep2; struct list_head *list; - int cnt = 0, err = 1; + int cnt = 0; + + err = 1; list_for_each(list, &ep->base.bind_addr.address_list) cnt++; + read_lock(&head->lock); sctp_for_each_hentry(ep2, &head->chain) { struct sock *sk2 = ep2->base.sk;
@@ -760,25 +764,30 @@ static int __sctp_hash_endpoint(struct sctp_endpoint *ep) sctp_sk(sk), cnt); if (!err) { err = reuseport_add_sock(sk, sk2, any); - if (err) - return err; + if (err) { + read_unlock(&head->lock); + goto out; + } break; } else if (err < 0) { - return err; + read_unlock(&head->lock); + goto out; } } + read_unlock(&head->lock); if (err) { err = reuseport_alloc(sk, any); if (err) - return err; + goto out; } } write_lock(&head->lock); hlist_add_head(&ep->node, &head->chain); write_unlock(&head->lock); - return 0; +out: + return err; } /* Add an endpoint to the hash. Local BH-safe. */
Dmitry