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

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, drop both locks, leave the addr_waitq entry queued and
re-arm addr_wq_timer to retry after SCTP_ADDRESS_TICK_DELAY. 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 <redacted>
---
 net/sctp/protocol.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 27c26e12f95d..7528a40a87f5 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c

@@ -645,6 +645,14 @@ 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);
+                               spin_unlock_bh(&net->sctp.addr_wq_lock);
+                               /* Try again later. */
+                               mod_timer(&net->sctp.addr_wq_timer,
+                                       jiffies + msecs_to_jiffies(SCTP_ADDRESS_TICK_DELAY));
+                               return;
+                       }