From 35b4f24415c854cd718ccdf38dbea6297f010aae Mon Sep 17 00:00:00 2001 From: Xin Long Date: Sat, 1 May 2021 04:02:58 +0800 Subject: [PATCH 1/3] 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: 145cb2f7177d ("sctp: Fix bundling of SHUTDOWN with COOKIE-ACK") Reported-by: Alexander Sverdlin Reported-by: syzbot+bbe538efd1046586f587@syzkaller.appspotmail.com Reported-by: Michal Tesar Signed-off-by: Xin Long Acked-by: Marcelo Ricardo Leitner Signed-off-by: David S. Miller --- net/sctp/sm_statefuns.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index 7632714c1e5b..30cb94696843 100644 --- 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)) && From 7e9269a5acec6d841d22e12770a0b02db4f5d8f2 Mon Sep 17 00:00:00 2001 From: Xin Long Date: Sat, 1 May 2021 04:02:59 +0800 Subject: [PATCH 2/3] Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK" This can be reverted as shutdown and cookie_ack chunk are using the same asoc since the last patch. This reverts commit 145cb2f7177d94bc54563ed26027e952ee0ae03c. Signed-off-by: Xin Long Acked-by: Marcelo Ricardo Leitner Signed-off-by: David S. Miller --- net/sctp/sm_statefuns.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index 30cb94696843..e8ccc4e8b086 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -1892,7 +1892,7 @@ static enum sctp_disposition sctp_sf_do_dupcook_a( */ sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl)); return sctp_sf_do_9_2_start_shutdown(net, ep, asoc, - SCTP_ST_CHUNK(0), repl, + SCTP_ST_CHUNK(0), NULL, commands); } else { sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE, @@ -5536,7 +5536,7 @@ enum sctp_disposition sctp_sf_do_9_2_start_shutdown( * in the Cumulative TSN Ack field the last sequential TSN it * has received from the peer. */ - reply = sctp_make_shutdown(asoc, arg); + reply = sctp_make_shutdown(asoc, NULL); if (!reply) goto nomem; @@ -6134,7 +6134,7 @@ enum sctp_disposition sctp_sf_autoclose_timer_expire( disposition = SCTP_DISPOSITION_CONSUME; if (sctp_outq_is_empty(&asoc->outqueue)) { disposition = sctp_sf_do_9_2_start_shutdown(net, ep, asoc, type, - NULL, commands); + arg, commands); } return disposition; From 51eac7f2f06b5f60d22dfb06c48d98a227507b8e Mon Sep 17 00:00:00 2001 From: Xin Long Date: Sat, 1 May 2021 04:03:00 +0800 Subject: [PATCH 3/3] sctp: do asoc update earlier in sctp_sf_do_dupcook_b The same thing should be done for sctp_sf_do_dupcook_b(). Meanwhile, SCTP_CMD_UPDATE_ASSOC cmd can be removed. v1->v2: - Fix the return value in sctp_sf_do_assoc_update(). Signed-off-by: Xin Long Acked-by: Marcelo Ricardo Leitner Signed-off-by: David S. Miller --- include/net/sctp/command.h | 1 - net/sctp/sm_sideeffect.c | 26 --------------------- net/sctp/sm_statefuns.c | 47 ++++++++++++++++++++++++-------------- 3 files changed, 30 insertions(+), 44 deletions(-) diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h index e8df72e1627a..5e848884ff61 100644 --- a/include/net/sctp/command.h +++ b/include/net/sctp/command.h @@ -68,7 +68,6 @@ enum sctp_verb { SCTP_CMD_ASSOC_FAILED, /* Handle association failure. */ SCTP_CMD_DISCARD_PACKET, /* Discard the whole packet. */ SCTP_CMD_GEN_SHUTDOWN, /* Generate a SHUTDOWN chunk. */ - SCTP_CMD_UPDATE_ASSOC, /* Update association information. */ SCTP_CMD_PURGE_OUTQUEUE, /* Purge all data waiting to be sent. */ SCTP_CMD_SETUP_T2, /* Hi-level, setup T2-shutdown parms. */ SCTP_CMD_RTO_PENDING, /* Set transport's rto_pending. */ diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index 0948f14ce221..ce15d590a615 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -826,28 +826,6 @@ static void sctp_cmd_setup_t2(struct sctp_cmd_seq *cmds, asoc->timeouts[SCTP_EVENT_TIMEOUT_T2_SHUTDOWN] = t->rto; } -static void sctp_cmd_assoc_update(struct sctp_cmd_seq *cmds, - struct sctp_association *asoc, - struct sctp_association *new) -{ - struct net *net = asoc->base.net; - struct sctp_chunk *abort; - - if (!sctp_assoc_update(asoc, new)) - return; - - 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(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort)); - } - sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED)); - sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED, - SCTP_PERR(SCTP_ERROR_RSRC_LOW)); - SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS); - SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB); -} - /* Helper function to change the state of an association. */ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds, struct sctp_association *asoc, @@ -1301,10 +1279,6 @@ static int sctp_cmd_interpreter(enum sctp_event_type event_type, sctp_endpoint_add_asoc(ep, asoc); break; - case SCTP_CMD_UPDATE_ASSOC: - sctp_cmd_assoc_update(commands, asoc, cmd->obj.asoc); - break; - case SCTP_CMD_PURGE_OUTQUEUE: sctp_outq_teardown(&asoc->outqueue); break; diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index e8ccc4e8b086..a42844904b81 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -1773,6 +1773,30 @@ enum sctp_disposition sctp_sf_do_5_2_3_initack( return sctp_sf_discard_chunk(net, ep, asoc, type, arg, commands); } +static int sctp_sf_do_assoc_update(struct sctp_association *asoc, + struct sctp_association *new, + struct sctp_cmd_seq *cmds) +{ + struct net *net = asoc->base.net; + struct sctp_chunk *abort; + + if (!sctp_assoc_update(asoc, new)) + return 0; + + 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(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort)); + } + sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED)); + sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED, + SCTP_PERR(SCTP_ERROR_RSRC_LOW)); + SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS); + SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB); + + return -ENOMEM; +} + /* Unexpected COOKIE-ECHO handler for peer restart (Table 2, action 'A') * * Section 5.2.4 @@ -1853,21 +1877,8 @@ static enum sctp_disposition sctp_sf_do_dupcook_a( sctp_add_cmd_sf(commands, SCTP_CMD_PURGE_ASCONF_QUEUE, SCTP_NULL()); /* 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); + if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands)) goto nomem; - } repl = sctp_make_cookie_ack(asoc, chunk); if (!repl) @@ -1940,14 +1951,16 @@ static enum sctp_disposition sctp_sf_do_dupcook_b( if (!sctp_auth_chunk_verify(net, chunk, new_asoc)) return SCTP_DISPOSITION_DISCARD; - /* 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_NEW_STATE, SCTP_STATE(SCTP_STATE_ESTABLISHED)); SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB); sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL()); - repl = sctp_make_cookie_ack(new_asoc, chunk); + /* Update the content of current association. */ + if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands)) + goto nomem; + + repl = sctp_make_cookie_ack(asoc, chunk); if (!repl) goto nomem;