sctp: do asoc update earlier in sctp_sf_do_dupcook_a

There's a panic that occurs in a few of envs, the call trace is as below:

  [] general protection fault, ... 0x29acd70f1000a: 0000 [#1] SMP PTI
  [] RIP: 0010:sctp_ulpevent_notify_peer_addr_change+0x4b/0x1fa [sctp]
  []  sctp_assoc_control_transport+0x1b9/0x210 [sctp]
  []  sctp_do_8_2_transport_strike.isra.16+0x15c/0x220 [sctp]
  []  sctp_cmd_interpreter.isra.21+0x1231/0x1a10 [sctp]
  []  sctp_do_sm+0xc3/0x2a0 [sctp]
  []  sctp_generate_timeout_event+0x81/0xf0 [sctp]

This is caused by a transport use-after-free issue. When processing a
duplicate COOKIE-ECHO chunk in sctp_sf_do_dupcook_a(), both COOKIE-ACK
and SHUTDOWN chunks are allocated with the transort from the new asoc.
However, later in the sideeffect machine, the old asoc is used to send
them out and old asoc's shutdown_last_sent_to is set to the transport
that SHUTDOWN chunk attached to in sctp_cmd_setup_t2(), which actually
belongs to the new asoc. After the new_asoc is freed and the old asoc
T2 timeout, the old asoc's shutdown_last_sent_to that is already freed
would be accessed in sctp_sf_t2_timer_expire().

Thanks Alexander and Jere for helping dig into this issue.

To fix it, this patch is to do the asoc update first, then allocate
the COOKIE-ACK and SHUTDOWN chunks with the 'updated' old asoc. This
would make more sense, as a chunk from an asoc shouldn't be sent out
with another asoc. We had fixed quite a few issues caused by this.

Fixes: 55a1670c1c57 ("sctp: Fix bundling of SHUTDOWN with COOKIE-ACK")
Reported-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Reported-by: syzbot+bbe538efd1046586f587@syzkaller.appspotmail.com
Reported-by: Michal Tesar <mtesar@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 7632714c1e5bf7f100e9d856a903a53d1e9fa3e8..30cb94696843d3a359e15114a8dd6b82489f4577 100644 (file)
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1852,20 +1852,35 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
                        SCTP_TO(SCTP_EVENT_TIMEOUT_T4_RTO));
        sctp_add_cmd_sf(commands, SCTP_CMD_PURGE_ASCONF_QUEUE, SCTP_NULL());
 
-       repl = sctp_make_cookie_ack(new_asoc, chunk);
+       /* Update the content of current association. */
+       if (sctp_assoc_update((struct sctp_association *)asoc, new_asoc)) {
+               struct sctp_chunk *abort;
+
+               abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
+               if (abort) {
+                       sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
+                       sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
+               }
+               sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
+               sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
+                               SCTP_PERR(SCTP_ERROR_RSRC_LOW));
+               SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
+               SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
+               goto nomem;
+       }
+
+       repl = sctp_make_cookie_ack(asoc, chunk);
        if (!repl)
                goto nomem;
 
        /* Report association restart to upper layer. */
        ev = sctp_ulpevent_make_assoc_change(asoc, 0, SCTP_RESTART, 0,
-                                            new_asoc->c.sinit_num_ostreams,
-                                            new_asoc->c.sinit_max_instreams,
+                                            asoc->c.sinit_num_ostreams,
+                                            asoc->c.sinit_max_instreams,
                                             NULL, GFP_ATOMIC);
        if (!ev)
                goto nomem_ev;
 
-       /* Update the content of current association. */
-       sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
        sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
        if ((sctp_state(asoc, SHUTDOWN_PENDING) ||
             sctp_state(asoc, SHUTDOWN_SENT)) &&