[PATCH v2] sctp: fix race between addr_wq timer handler and backlog draining

[Binbin Deng]

sctp_addr_wq_timeout_handler() walks net->sctp.auto_asconf_splist and,
holding only bh_lock_sock(), calls sctp_asconf_mgmt() on every
auto-asconf socket. Unlike every other asynchronous SCTP entry point
(sctp_rcv() queuing packets to the backlog, and the timer handlers in
sctp_sm_side_effect()), it does not check sock_owned_by_user() to defer
its work while a user context owns the socket.

While a user thread owns the socket (e.g. recvmsg() blocked in a copy
to user, or release_sock() draining the backlog), sk_lock.owned is set
but the socket spinlock has been dropped, so bh_lock_sock() succeeds
and the timer enters

    sctp_asconf_mgmt() -> sctp_send_asconf_add/del_ip() -> sctp_do_sm()

concurrently with the backlog drain path

    release_sock() -> __release_sock() -> sk_backlog_rcv()
      -> sctp_assoc_bh_rcv() -> sctp_do_sm()

Two state machines then run on the same association with no common
serialization and mutate shared state concurrently: the outqueue
control chunk list, transport->packet.chunk_list and packet->auth, and
the transport/dst lifetime.

The bug was found by code review of this timer entry point against the
sock_owned_by_user() avoidance used by all other SCTP asynchronous
entries, and confirmed with a reproducer that races an RTM_DELADDR
address event against a user-owned auto-asconf socket. It triggers the
following, observed with KASAN and refcount debugging:

  - rcuref "imbalanced put()" from dst_release() in
    sctp_transport_route() racing the timer's transport routing,
  - a GPF in sctp_packet_transmit() dereferencing a chunk pointer
    corrupted by concurrent packet/chunk list use, and
  - associations dying silently (ETIMEDOUT, GET_PEER_ADDRS failure).

Defer the work like the other entries do: if the socket is owned by
the user, stop processing, leave the addr_waitq entry queued and
re-arm addr_wq_timer to retry after SCTP_ADDRESS_TICK_DELAY. The
retry is merged with the existing IPv6 DAD retry into a single
mod_timer() outside the loops. Sockets processed earlier for the same
address event will see a duplicate ASCONF on retry; the peer reports
that in an ASCONF-ACK error indication, which the association
tolerates.

Fixes: 9f7d653b67ae ("sctp: Add Auto-ASCONF support (core).")
Signed-off-by: Binbin Deng <18983559317@xxxxxxx>
---
 net/sctp/protocol.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 27c26e12f95d..267dc9b7a0e7 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -603,6 +603,7 @@ static void sctp_addr_wq_timeout_handler(struct timer_list *t)
 	struct net *net = timer_container_of(net, t, sctp.addr_wq_timer);
 	struct sctp_sockaddr_entry *addrw, *temp;
 	struct sctp_sock *sp;
+	bool retry = false;
 
 	spin_lock_bh(&net->sctp.addr_wq_lock);
 
@@ -624,15 +625,12 @@ static void sctp_addr_wq_timeout_handler(struct timer_list *t)
 			in6 = (struct in6_addr *)&addrw->a.v6.sin6_addr;
 			if (ipv6_chk_addr(net, in6, NULL, 0) == 0 &&
 			    addrw->state == SCTP_ADDR_NEW) {
-				unsigned long timeo_val;
 
 				pr_debug("%s: this is on DAD, trying %d sec "
 					 "later\n", __func__,
 					 SCTP_ADDRESS_TICK_DELAY);
 
-				timeo_val = jiffies;
-				timeo_val += msecs_to_jiffies(SCTP_ADDRESS_TICK_DELAY);
-				mod_timer(&net->sctp.addr_wq_timer, timeo_val);
+				retry = true;
 				break;
 			}
 		}
@@ -645,16 +643,27 @@ static void sctp_addr_wq_timeout_handler(struct timer_list *t)
 			if (!sctp_is_ep_boundall(sk))
 				continue;
 			bh_lock_sock(sk);
+			if (sock_owned_by_user(sk)) {
+				bh_unlock_sock(sk);
+				retry = true;
+				break;
+			}
 			if (sctp_asconf_mgmt(sp, addrw) < 0)
 				pr_debug("%s: sctp_asconf_mgmt failed\n", __func__);
 			bh_unlock_sock(sk);
 		}
+		if (retry)
+			break;
 #if IS_ENABLED(CONFIG_IPV6)
 free_next:
 #endif
 		list_del(&addrw->list);
 		kfree(addrw);
 	}
+
+	if (retry)
+		mod_timer(&net->sctp.addr_wq_timer,
+			jiffies + msecs_to_jiffies(SCTP_ADDRESS_TICK_DELAY));
 	spin_unlock_bh(&net->sctp.addr_wq_lock);
 }
 
-- 
2.43.0