Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected chunk with length set to zero
From: Vlad Yasevich <hidden>
Date: 2007-08-29 15:26:33
Wei Yongjun wrote:
Vlad Yasevich wrote:quoted
NACK Section 8.4: An SCTP packet is called an "out of the blue" (OOTB) packet if it is correctly formed (i.e., passed the receiver's CRC32c check; see Section 6.8), but the receiver is not able to identify the association to which this packet belongs. I would argue that the packet is not correctly formed in this case and deserves a protocol violation ABORT in return. -vladAs your comment, patch has been changed. Patch has been split to two, one is resolve this dead loop problem in this mail. And the other is come in another mail to discard partial chunk which chunk length is set to zero.
I am starting to question the entire OOTB packet handling. There are way too many function that do almost the same thing and all handle OOTB a little different.
quoted hunk ↗ jump to hunk
Signed-off-by: Wei Yongjun <redacted>--- a/net/sctp/sm_statefuns.c 2007-08-17 06:17:14.000000000 -0400 +++ b/net/sctp/sm_statefuns.c 2007-08-17 09:57:17.000000000 -0400@@ -98,6 +98,7 @@ static sctp_disposition_t sctp_stop_t1_a struct sctp_transport *transport);static sctp_disposition_t sctp_sf_abort_violation( + const struct sctp_endpoint *ep, const struct sctp_association *asoc, void *arg, sctp_cmd_seq_t *commands,@@ -192,6 +193,11 @@ sctp_disposition_t sctp_sf_do_4_C(const if(!sctp_vtag_verify_either(chunk, asoc)) return sctp_sf_pdiscard(ep, asoc, type, arg, commands); + /* Make sure that the SHUTDOWN_COMPLETE chunk has a valid length. */ + if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t))) + return sctp_sf_violation_chunklen(ep, asoc, type, arg, + commands); + /* RFC 2960 10.2 SCTP-to-ULP * * H) SHUTDOWN COMPLETE notification@@ -2495,6 +2501,11 @@ sctp_disposition_t sctp_sf_do_9_2_reshut struct sctp_chunk *chunk = (struct sctp_chunk *) arg; struct sctp_chunk *reply; + /* Make sure that the INIT chunk has a valid length */ + if (!sctp_chunk_length_valid(chunk, sizeof(sctp_init_chunk_t))) + return sctp_sf_violation_chunklen(ep, asoc, type, arg, + commands); +
sctp_sf_do_9_2_reshutack() is also called during sctp_sf_do_dupcook_a() processing, so checking for INIT chunk is wrong. Checking for just the chunkhdr_t should be enough.
quoted hunk ↗ jump to hunk
/* Since we are not going to really process this INIT, there * is no point in verifying chunk boundries. Just generate * the SHUTDOWN ACK.@@ -2938,6 +2949,11 @@ sctp_disposition_t sctp_sf_tabort_8_4_8( struct sctp_chunk *chunk = arg; struct sctp_chunk *abort; + /* Make sure that the chunk has a valid length. */ + if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t))) + return sctp_sf_violation_chunklen(ep, asoc, type, arg, + commands); + packet = sctp_ootb_pkt_new(asoc, chunk);
sctp_sf_tabort_8_4_8 is used directly as well (not just through the state machine). Not sure if the header verification is appropriate.
quoted hunk ↗ jump to hunk
if (packet) {@@ -3185,6 +3201,11 @@ static sctp_disposition_t sctp_sf_shut_8 struct sctp_chunk *chunk = arg; struct sctp_chunk *shut; + /* Make sure that the SHUTDOWN_ACK chunk has a valid length. */ + if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t))) + return sctp_sf_violation_chunklen(ep, asoc, type, arg, + commands); + packet = sctp_ootb_pkt_new(asoc, chunk);
This is a static function, so any verifications should already have been done.
quoted hunk ↗ jump to hunk
if (packet) {@@ -3240,6 +3261,13 @@ sctp_disposition_t sctp_sf_do_8_5_1_E_sa void *arg, sctp_cmd_seq_t *commands){ + struct sctp_chunk *chunk = arg; + + /* Make sure that the SHUTDOWN_ACK chunk has a valid length. */ + if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t))) + return sctp_sf_violation_chunklen(ep, asoc, type, arg, + commands); + /* Although we do have an association in this case, it corresponds * to a restarted association. So the packet is treated as an OOTB * packet and the state function that handles OOTB SHUTDOWN_ACK is@@ -3654,6 +3682,16 @@ sctp_disposition_t sctp_sf_discard_chunk void *arg, sctp_cmd_seq_t *commands){ + struct sctp_chunk *chunk = arg; + + /* Make sure that the chunk has a valid length. + * Since we don't know the chunk type, we use a general + * chunkhdr structure to make a comparison. + */ + if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t))) + return sctp_sf_violation_chunklen(ep, asoc, type, arg, + commands); + SCTP_DEBUG_PRINTK("Chunk %d is discarded\n", type.chunk); return SCTP_DISPOSITION_DISCARD; }@@ -3709,6 +3747,13 @@ sctp_disposition_t sctp_sf_violation(con void *arg, sctp_cmd_seq_t *commands){ + struct sctp_chunk *chunk = arg; + + /* Make sure that the chunk has a valid length. */ + if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t))) + return sctp_sf_violation_chunklen(ep, asoc, type, arg, + commands); + return SCTP_DISPOSITION_VIOLATION; }@@ -3716,12 +3761,14 @@ sctp_disposition_t sctp_sf_violation(con * Common function to handle a protocol violation. */static sctp_disposition_t sctp_sf_abort_violation( + const struct sctp_endpoint *ep, const struct sctp_association *asoc, void *arg, sctp_cmd_seq_t *commands, const __u8 *payload, const size_t paylen) { + struct sctp_packet *packet = NULL; struct sctp_chunk *chunk = arg; struct sctp_chunk *abort = NULL;@@ -3730,22 +3777,41 @@ static sctp_disposition_t sctp_sf_abort_ if (!abort) goto nomem; - sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort)); - SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS); + if (asoc) { + sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort)); + SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS); - if (asoc->state <= SCTP_STATE_COOKIE_ECHOED) { - sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP, - SCTP_TO(SCTP_EVENT_TIMEOUT_T1_INIT)); - sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR, - SCTP_ERROR(ECONNREFUSED)); - sctp_add_cmd_sf(commands, SCTP_CMD_INIT_FAILED, - SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION)); + if (asoc->state <= SCTP_STATE_COOKIE_ECHOED) { + sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP, + SCTP_TO(SCTP_EVENT_TIMEOUT_T1_INIT)); + sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR, + SCTP_ERROR(ECONNREFUSED)); + sctp_add_cmd_sf(commands, SCTP_CMD_INIT_FAILED, + SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION)); + } else { + 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_PROTO_VIOLATION)); + SCTP_DEC_STATS(SCTP_MIB_CURRESTAB); + } } else { - 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_PROTO_VIOLATION)); - SCTP_DEC_STATS(SCTP_MIB_CURRESTAB); + packet = sctp_ootb_pkt_new(asoc, chunk); + + if (!packet) + goto nomem; + + if (sctp_test_T_bit(abort)) + packet->vtag = ntohl(chunk->sctp_hdr->vtag); + + abort->skb->sk = ep->base.sk; + + sctp_packet_append_chunk(packet, abort); + + sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT, +SCTP_PACKET(packet)); + + SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS); } sctp_add_cmd_sf(commands, SCTP_CMD_DISCARD_PACKET, SCTP_NULL());@@ -3786,7 +3852,7 @@ static sctp_disposition_t sctp_sf_violat{ char err_str[]="The following chunk had invalid length:"; - return sctp_sf_abort_violation(asoc, arg, commands, err_str, + return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str, sizeof(err_str)); }@@ -3805,7 +3871,7 @@ static sctp_disposition_t sctp_sf_violat{ char err_str[]="The cumulative tsn ack beyond the max tsn currently sent:"; - return sctp_sf_abort_violation(asoc, arg, commands, err_str, + return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str, sizeof(err_str)); }