SCTP: Fix dead loop while received unexpected chunk with length set to zero

SCTP: Fix dead loop while received unexpected chunk with length set to zero

[Wei Yongjun]

A ootb chunk such as data in close state or init-ack in estab state will 
cause SCTP to enter dead loop. Look like this:

(1)
  Endpoint A                      Endpoint B
  (Closed)                        (Closed)

  DATA      ----------------->   Kernel dead loop
  (With Length set to zero)

(2)
  Endpoint A                      Endpoint B
  (Established)                   (Established)

  INIT-ACK   ----------------->   Kernel dead loop
  (With Length set to zero)


This is beacuse when process chunks, chunk->chunk_end is set to the 
chunk->chunk_hdr plus chunk length, if chunk length is set to zero, 
chunk->chunk_end will be never changed and process enter dead loop.
Following is the patch.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- a/net/sctp/inqueue.c	2007-08-25 10:53:45.000000000 -0400
+++ b/net/sctp/inqueue.c	2007-08-26 05:45:57.000000000 -0400
@@ -165,10 +165,8 @@ struct sctp_chunk *sctp_inq_pop(struct s
 	skb_pull(chunk->skb, sizeof(sctp_chunkhdr_t));
 	chunk->subh.v = NULL; /* Subheader is no longer valid.  */
 
-	if (chunk->chunk_end < skb_tail_pointer(chunk->skb)) {
-		/* This is not a singleton */
-		chunk->singleton = 0;
-	} else if (chunk->chunk_end > skb_tail_pointer(chunk->skb)) {
+	if (chunk->chunk_end > skb_tail_pointer(chunk->skb) ||
+	    chunk->chunk_end == chunk->chunk_hdr) {
 		/* RFC 2960, Section 6.10  Bundling
 		 *
 		 * Partial chunks MUST NOT be placed in an SCTP packet.
@@ -183,6 +181,9 @@ struct sctp_chunk *sctp_inq_pop(struct s
 		chunk = queue->in_progress = NULL;
 
 		return NULL;
+	} else if (chunk->chunk_end < skb_tail_pointer(chunk->skb)) {
+		/* This is not a singleton */
+		chunk->singleton = 0;
 	} else {
 		/* We are at the end of the packet, so mark the chunk
 		 * in case we need to send a SACK.

Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected chunk with length set to zero

[Wei Yongjun]

Vlad Yasevich wrote:
> Wei Yongjun wrote:
>   
>> A ootb chunk such as data in close state or init-ack in estab state will 
>> cause SCTP to enter dead loop. Look like this:
>>
>> (1)
>>   Endpoint A                      Endpoint B
>>   (Closed)                        (Closed)
>>
>>   DATA      ----------------->   Kernel dead loop
>>   (With Length set to zero)
>>
>> (2)
>>   Endpoint A                      Endpoint B
>>   (Established)                   (Established)
>>
>>   INIT-ACK   ----------------->   Kernel dead loop
>>   (With Length set to zero)
>>
>>
>> This is beacuse when process chunks, chunk->chunk_end is set to the 
>> chunk->chunk_hdr plus chunk length, if chunk length is set to zero, 
>> chunk->chunk_end will be never changed and process enter dead loop.
>> Following is the patch.
>>     
>
> 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.
>
> -vlad
>   
As 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.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- 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);
+
 	/* 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);
 
 	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);
 
 	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));
 }
 

Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected chunk with length set to zero

[Vlad Yasevich]

Wei Yongjun wrote:
> Vlad Yasevich wrote:
>>
>> 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.
>>
>> -vlad
>>   
> As 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.

> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> 
> --- 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.


>     /* 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.

> 
>     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.

> 
>     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));
> }
> 
> 
> 

Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected chunk with length set to zero

[Wei Yongjun]

Vlad Yasevich wrote:
> Wei Yongjun wrote:
>   
>> Vlad Yasevich wrote:
>>     
>>> 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.
>>>
>>> -vlad
>>>   
>>>       
>> As 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.
>
> 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.
>   
This has been changed.
> 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.
>   
It is needed. Because sctp_sf_tabort_8_4_8() is called to handle OOTB 
packet before check the header length.
> This is a static function, so any verifications should already have been
> done.
>   
Removed.

Thanks.

Regards

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- a/net/sctp/sm_statefuns.c	2007-08-17 06:17:14.000000000 -0400
+++ b/net/sctp/sm_statefuns.c	2007-08-18 07:59:25.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 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);
+
 	/* 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);
 
 	if (packet) {
@@ -3240,6 +3256,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 +3677,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 +3742,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 +3756,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 +3772,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 +3847,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 +3866,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));
 }
 

Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected chunk with length set to zero

[Vlad Yasevich]

Wei Yongjun wrote:
> Vlad Yasevich wrote:
>> Wei Yongjun wrote:
>>  
>>> Vlad Yasevich wrote:
>>>    
>>>> 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.
>>>>
>>>> -vlad
>>>>         
>>> As 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.
>>
>> 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.
>>   
> This has been changed.
>> 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.
>>   
> It is needed. Because sctp_sf_tabort_8_4_8() is called to handle OOTB
> packet before check the header length.

But now we are doing the same thing twice (and this is not the only place).
I know I am being really picky here, but I am starting to thing the ootb handling\
is a mess and I really don't want to add to the mess.

Until I (or someone else) prove that it's not a mess or fix it, I am going
to hold off on these patches.

Feel free to go through the spec and fix all the OOTB handling.

Thanks
-vlad

Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected chunk with length set to zero

[Wei Yongjun]

Vlad Yasevich wrote:
> Wei Yongjun wrote:
>   
>> Vlad Yasevich wrote:
>>     
>>> Wei Yongjun wrote:
>>>  
>>>       
>>>> Vlad Yasevich wrote:
>>>>    
>>>>         
>>>>> 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.
>>>>>
>>>>> -vlad
>>>>>         
>>>>>           
>>>> As 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.
>>>
>>> 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.
>>>   
>>>       
>> This has been changed.
>>     
>>> 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.
>>>   
>>>       
>> It is needed. Because sctp_sf_tabort_8_4_8() is called to handle OOTB
>> packet before check the header length.
>>     
>
> But now we are doing the same thing twice (and this is not the only place).
> I know I am being really picky here, but I am starting to thing the ootb handling\
> is a mess and I really don't want to add to the mess.
>
> Until I (or someone else) prove that it's not a mess or fix it, I am going
> to hold off on these patches.
>
> Feel free to go through the spec and fix all the OOTB handling.
>
> Thanks
> -vlad
>   
Hi vlad:

I think this probleam must be check as soon as possible, because this is 
a security hole. This probleam let SCTP module to be unsafe, if we load 
it, single bad format SCTP packet can make my system dead loop and no 
response to anything, console is freeze too. The same as kernel panic, 
and also can be used to attack other machine by send too many ABORT packet.

May be someone can provide a better patch to this probleam. And I'd 
pleased to see someone to resolve this probleam.

Regards
Wei Yongjun

Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected chunk with length set to zero

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 30 Aug 2007 09:45:22 -0400

> But now we are doing the same thing twice (and this is not the only
> place).  I know I am being really picky here, but I am starting to
> thing the ootb handling\ is a mess and I really don't want to add to
> the mess.  Until I (or someone else) prove that it's not a mess or
> fix it, I am going to hold off on these patches.
>
> Feel free to go through the spec and fix all the OOTB handling.

I think you should reprioritize, because as Wei emphasized this
is remotely triggerable stuff.

I realize what a pain it is, but a band-aid fix is better than a
remote hole.

Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected chunk with length set to zero

[Wei Yongjun]

Vlad Yasevich wrote:
> Wei Yongjun wrote:
>   
>> Vlad Yasevich wrote:
>>     
>>> Wei Yongjun wrote:
>>>  
>>>       
>>>> Vlad Yasevich wrote:
>>>>    
>>>>         
>>>>> 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.
>>>>>
>>>>> -vlad
>>>>>         
>>>>>           
>>>> As 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.
>>>
>>> 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.
>>>   
>>>       
>> This has been changed.
>>     
>>> 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.
>>>   
>>>       
>> It is needed. Because sctp_sf_tabort_8_4_8() is called to handle OOTB
>> packet before check the header length.
>>     
>
> But now we are doing the same thing twice (and this is not the only place).
> I know I am being really picky here, but I am starting to thing the ootb handling\
> is a mess and I really don't want to add to the mess.
>
> Until I (or someone else) prove that it's not a mess or fix it, I am going
> to hold off on these patches.
>
> Feel free to go through the spec and fix all the OOTB handling.
>
> Thanks
> -vlad
>   
Packet changed:
1. Used sctp_sf_ootb() to handle OOTB packet
2. Remove length check from sctp_sf_tabort_8_4_8() in last patch
3. Add length check to sctp_sf_ootb()
4. Changed validity check order in sctp_sf_do_5_1B_init() and other functions to fix possible attack.

This patch may be correct.

 
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

diff -Nurp a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
--- a/net/sctp/sm_statefuns.c	2007-08-17 06:17:14.000000000 -0400
+++ b/net/sctp/sm_statefuns.c	2007-08-19 07:52: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,
@@ -181,6 +182,14 @@ sctp_disposition_t sctp_sf_do_4_C(const 
 	struct sctp_chunk *chunk = arg;
 	struct sctp_ulpevent *ev;
 
+	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 6.10 Bundling
 	 *
 	 * An endpoint MUST NOT bundle INIT, INIT ACK or
@@ -189,9 +198,6 @@ sctp_disposition_t sctp_sf_do_4_C(const 
 	if (!chunk->singleton)
 		return SCTP_DISPOSITION_VIOLATION;
 
-	if (!sctp_vtag_verify_either(chunk, asoc))
-		return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
-
 	/* RFC 2960 10.2 SCTP-to-ULP
 	 *
 	 * H) SHUTDOWN COMPLETE notification
@@ -267,6 +273,20 @@ sctp_disposition_t sctp_sf_do_5_1B_init(
 	struct sock *sk;
 	int len;
 
+	/* Make sure that the INIT chunk has a valid length.
+	 * Normally, this would cause an ABORT with a Protocol Violation
+	 * error, but since we don't have an association, we'll
+	 * just discard the packet.
+	 */
+	if (!sctp_chunk_length_valid(chunk, sizeof(sctp_init_chunk_t)))
+		return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
+
+	/* 3.1 A packet containing an INIT chunk MUST have a zero Verification
+	 * Tag.
+	 */
+	if (chunk->sctp_hdr->vtag != 0)
+		return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
+
 	/* 6.10 Bundling
 	 * An endpoint MUST NOT bundle INIT, INIT ACK or
 	 * SHUTDOWN COMPLETE with any other chunks.
@@ -295,20 +315,6 @@ sctp_disposition_t sctp_sf_do_5_1B_init(
 	     sk_acceptq_is_full(sk)))
 		return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
 
-	/* 3.1 A packet containing an INIT chunk MUST have a zero Verification
-	 * Tag.
-	 */
-	if (chunk->sctp_hdr->vtag != 0)
-		return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
-
-	/* Make sure that the INIT chunk has a valid length.
-	 * Normally, this would cause an ABORT with a Protocol Violation
-	 * error, but since we don't have an association, we'll
-	 * just discard the packet.
-	 */
-	if (!sctp_chunk_length_valid(chunk, sizeof(sctp_init_chunk_t)))
-		return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
-
 	/* Verify the INIT chunk before processing it. */
 	err_chunk = NULL;
 	if (!sctp_verify_init(asoc, chunk->chunk_hdr->type,
@@ -591,12 +597,6 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(co
 	int error = 0;
 	struct sctp_chunk *err_chk_p;
 
-	/* If the packet is an OOTB packet which is temporarily on the
-	 * control endpoint, respond with an ABORT.
-	 */
-	if (ep == sctp_sk((sctp_get_ctl_sock()))->ep)
-		return sctp_sf_ootb(ep, asoc, type, arg, commands);
-
 	/* Make sure that the COOKIE_ECHO chunk has a valid length.
 	 * In this case, we check that we have enough for at least a
 	 * chunk header.  More detailed verification is done
@@ -605,6 +605,12 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(co
 	if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t)))
 		return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
 
+	/* If the packet is an OOTB packet which is temporarily on the
+	 * control endpoint, respond with an ABORT.
+	 */
+	if (ep == sctp_sk((sctp_get_ctl_sock()))->ep)
+		return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
+
 	/* "Decode" the chunk.  We have no optional parameters so we
 	 * are in good shape.
 	 */
@@ -1281,6 +1287,20 @@ static sctp_disposition_t sctp_sf_do_une
 	sctp_unrecognized_param_t *unk_param;
 	int len;
 
+	/* Make sure that the INIT chunk has a valid length.
+	 * In this case, we generate a protocol violation since we have
+	 * an association established.
+	 */
+	if (!sctp_chunk_length_valid(chunk, sizeof(sctp_init_chunk_t)))
+		return sctp_sf_violation_chunklen(ep, asoc, type, arg,
+						  commands);
+
+	/* 3.1 A packet containing an INIT chunk MUST have a zero Verification
+	 * Tag.
+	 */
+	if (chunk->sctp_hdr->vtag != 0)
+		return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
+
 	/* 6.10 Bundling
 	 * An endpoint MUST NOT bundle INIT, INIT ACK or
 	 * SHUTDOWN COMPLETE with any other chunks.
@@ -1293,19 +1313,6 @@ static sctp_disposition_t sctp_sf_do_une
 	if (!chunk->singleton)
 		return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
 
-	/* 3.1 A packet containing an INIT chunk MUST have a zero Verification
-	 * Tag.
-	 */
-	if (chunk->sctp_hdr->vtag != 0)
-		return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
-
-	/* Make sure that the INIT chunk has a valid length.
-	 * In this case, we generate a protocol violation since we have
-	 * an association established.
-	 */
-	if (!sctp_chunk_length_valid(chunk, sizeof(sctp_init_chunk_t)))
-		return sctp_sf_violation_chunklen(ep, asoc, type, arg,
-						  commands);
 	/* Grab the INIT header.  */
 	chunk->subh.init_hdr = (sctp_inithdr_t *) chunk->skb->data;
 
@@ -2495,6 +2502,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 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);
+
 	/* Since we are not going to really process this INIT, there
 	 * is no point in verifying chunk boundries.  Just generate
 	 * the SHUTDOWN ACK.
@@ -3146,6 +3158,11 @@ sctp_disposition_t sctp_sf_ootb(const st
 		ch = (sctp_chunkhdr_t *) ch_end;
 	} while (ch_end < skb_tail_pointer(skb));
 
+	/* 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);
+
 	if (ootb_shut_ack)
 		sctp_sf_shut_8_4_5(ep, asoc, type, arg, commands);
 	else
@@ -3240,6 +3257,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 +3678,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 +3743,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 +3757,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 +3773,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 +3848,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 +3867,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));
 }
 
diff -Nurp a/net/sctp/sm_statetable.c b/net/sctp/sm_statetable.c
--- a/net/sctp/sm_statetable.c	2007-08-09 11:58:11.000000000 -0400
+++ b/net/sctp/sm_statetable.c	2007-08-19 05:44:29.000000000 -0400
@@ -110,7 +110,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
 	/* SCTP_STATE_EMPTY */ \
 	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_CLOSED */ \
-	TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
+	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_COOKIE_WAIT */ \
 	TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
 	/* SCTP_STATE_COOKIE_ECHOED */ \
@@ -173,7 +173,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
 	/*  SCTP_STATE_EMPTY */ \
 	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_CLOSED */ \
-	TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
+	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_COOKIE_WAIT */ \
 	TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
 	/* SCTP_STATE_COOKIE_ECHOED */ \
@@ -194,7 +194,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
 	/*  SCTP_STATE_EMPTY */ \
 	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_CLOSED */ \
-	TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
+	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_COOKIE_WAIT */ \
 	TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
 	/* SCTP_STATE_COOKIE_ECHOED */ \
@@ -216,7 +216,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
 	/*  SCTP_STATE_EMPTY */ \
 	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_CLOSED */ \
-	TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
+	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_COOKIE_WAIT */ \
 	TYPE_SCTP_FUNC(sctp_sf_violation), \
 	/* SCTP_STATE_COOKIE_ECHOED */ \
@@ -258,7 +258,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
 	/* SCTP_STATE_EMPTY */ \
 	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_CLOSED */ \
-	TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
+	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_COOKIE_WAIT */ \
 	TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
 	/* SCTP_STATE_COOKIE_ECHOED */ \
@@ -300,7 +300,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
 	/* SCTP_STATE_EMPTY */ \
 	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_CLOSED */ \
-	TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
+	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_COOKIE_WAIT */ \
 	TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
 	/* SCTP_STATE_COOKIE_ECHOED */ \
@@ -499,7 +499,7 @@ static const sctp_sm_table_entry_t addip
 	/* SCTP_STATE_EMPTY */ \
 	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_CLOSED */ \
-	TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
+	TYPE_SCTP_FUNC(sctp_sf_ootb), \
 	/* SCTP_STATE_COOKIE_WAIT */ \
 	TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
 	/* SCTP_STATE_COOKIE_ECHOED */ \
@@ -528,7 +528,7 @@ chunk_event_table_unknown[SCTP_STATE_NUM
 	/* SCTP_STATE_EMPTY */
 	TYPE_SCTP_FUNC(sctp_sf_ootb),
 	/* SCTP_STATE_CLOSED */
-	TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8),
+	TYPE_SCTP_FUNC(sctp_sf_ootb),
 	/* SCTP_STATE_COOKIE_WAIT */
 	TYPE_SCTP_FUNC(sctp_sf_unk_chunk),
 	/* SCTP_STATE_COOKIE_ECHOED */

Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected chunk with length set to zero

[Vlad Yasevich]

Wei Yongjun wrote:

> Packet changed:
> 1. Used sctp_sf_ootb() to handle OOTB packet
> 2. Remove length check from sctp_sf_tabort_8_4_8() in last patch
> 3. Add length check to sctp_sf_ootb()
> 4. Changed validity check order in sctp_sf_do_5_1B_init() and other
> functions to fix possible attack.

Can you explain a little what the attack vector on this order.

See below...

> 
> This patch may be correct.
> 
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> 
> diff -Nurp a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> --- a/net/sctp/sm_statefuns.c    2007-08-17 06:17:14.000000000 -0400
> +++ b/net/sctp/sm_statefuns.c    2007-08-19 07:52: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,
> @@ -181,6 +182,14 @@ sctp_disposition_t sctp_sf_do_4_C(const     struct
> sctp_chunk *chunk = arg;
>     struct sctp_ulpevent *ev;
> 
> +    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 6.10 Bundling
>      *
>      * An endpoint MUST NOT bundle INIT, INIT ACK or
> @@ -189,9 +198,6 @@ sctp_disposition_t sctp_sf_do_4_C(const     if
> (!chunk->singleton)
>         return SCTP_DISPOSITION_VIOLATION;
> 
> -    if (!sctp_vtag_verify_either(chunk, asoc))
> -        return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
> -
>     /* RFC 2960 10.2 SCTP-to-ULP
>      *
>      * H) SHUTDOWN COMPLETE notification

OK, I can see how moving this resolves the attack on SHUTDOWN COMPLETE,
but that was because we reported a rather silly violation message and
not really skipping the chunk properly.

I looking at other handling of SHUDOWN COMPLETE, one could make an
argument for simply discarding the packet packet in both cases.

On the other hand, if one protocol violation deserves an abort, then
why not the other.  They are both very blatant.


> @@ -267,6 +273,20 @@ sctp_disposition_t sctp_sf_do_5_1B_init(
>     struct sock *sk;
>     int len;
> 
> +    /* Make sure that the INIT chunk has a valid length.
> +     * Normally, this would cause an ABORT with a Protocol Violation
> +     * error, but since we don't have an association, we'll
> +     * just discard the packet.
> +     */
> +    if (!sctp_chunk_length_valid(chunk, sizeof(sctp_init_chunk_t)))
> +        return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
> +
> +    /* 3.1 A packet containing an INIT chunk MUST have a zero Verification
> +     * Tag.
> +     */
> +    if (chunk->sctp_hdr->vtag != 0)
> +        return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
> +
>     /* 6.10 Bundling
>      * An endpoint MUST NOT bundle INIT, INIT ACK or
>      * SHUTDOWN COMPLETE with any other chunks.
> @@ -295,20 +315,6 @@ sctp_disposition_t sctp_sf_do_5_1B_init(
>          sk_acceptq_is_full(sk)))
>         return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
> 
> -    /* 3.1 A packet containing an INIT chunk MUST have a zero Verification
> -     * Tag.
> -     */
> -    if (chunk->sctp_hdr->vtag != 0)
> -        return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
> -
> -    /* Make sure that the INIT chunk has a valid length.
> -     * Normally, this would cause an ABORT with a Protocol Violation
> -     * error, but since we don't have an association, we'll
> -     * just discard the packet.
> -     */
> -    if (!sctp_chunk_length_valid(chunk, sizeof(sctp_init_chunk_t)))
> -        return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
> -
>     /* Verify the INIT chunk before processing it. */
>     err_chunk = NULL;
>     if (!sctp_verify_init(asoc, chunk->chunk_hdr->type,

Why re-order here?  Is it because sctp_sf_tabort_8_4_8() doesn't discard
the packet?

An interesting problem with reordering this is that we now respond with
an abort when previously we were silently discarding.  This is the bundled
INIT case.  

I spent some time looking at sctp_sf_tabort_8_4_8().  Based on the spec,
that function should discard the packet.  I finally had time to really
look at all the callers and I looks like they will be ok with that.
This was something I asked before, but never got an answer to.

> @@ -591,12 +597,6 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(co
>     int error = 0;
>     struct sctp_chunk *err_chk_p;
> 
> -    /* If the packet is an OOTB packet which is temporarily on the
> -     * control endpoint, respond with an ABORT.
> -     */
> -    if (ep == sctp_sk((sctp_get_ctl_sock()))->ep)
> -        return sctp_sf_ootb(ep, asoc, type, arg, commands);
> -
>     /* Make sure that the COOKIE_ECHO chunk has a valid length.
>      * In this case, we check that we have enough for at least a
>      * chunk header.  More detailed verification is done
> @@ -605,6 +605,12 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(co
>     if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t)))
>         return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
> 
> +    /* If the packet is an OOTB packet which is temporarily on the
> +     * control endpoint, respond with an ABORT.
> +     */
> +    if (ep == sctp_sk((sctp_get_ctl_sock()))->ep)
> +        return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
> +
>     /* "Decode" the chunk.  We have no optional parameters so we
>      * are in good shape.
>      */
> @@ -1281,6 +1287,20 @@ static sctp_disposition_t sctp_sf_do_une
>     sctp_unrecognized_param_t *unk_param;
>     int len;
> 
> +    /* Make sure that the INIT chunk has a valid length.
> +     * In this case, we generate a protocol violation since we have
> +     * an association established.
> +     */
> +    if (!sctp_chunk_length_valid(chunk, sizeof(sctp_init_chunk_t)))
> +        return sctp_sf_violation_chunklen(ep, asoc, type, arg,
> +                          commands);
> +
> +    /* 3.1 A packet containing an INIT chunk MUST have a zero Verification
> +     * Tag.
> +     */
> +    if (chunk->sctp_hdr->vtag != 0)
> +        return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
> +
>     /* 6.10 Bundling
>      * An endpoint MUST NOT bundle INIT, INIT ACK or
>      * SHUTDOWN COMPLETE with any other chunks.
> @@ -1293,19 +1313,6 @@ static sctp_disposition_t sctp_sf_do_une
>     if (!chunk->singleton)
>         return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
> 
> -    /* 3.1 A packet containing an INIT chunk MUST have a zero Verification
> -     * Tag.
> -     */
> -    if (chunk->sctp_hdr->vtag != 0)
> -        return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
> -
> -    /* Make sure that the INIT chunk has a valid length.
> -     * In this case, we generate a protocol violation since we have
> -     * an association established.
> -     */
> -    if (!sctp_chunk_length_valid(chunk, sizeof(sctp_init_chunk_t)))
> -        return sctp_sf_violation_chunklen(ep, asoc, type, arg,
> -                          commands);
>     /* Grab the INIT header.  */
>     chunk->subh.init_hdr = (sctp_inithdr_t *) chunk->skb->data;
> 

The same question as for the INIT processing above applies here as
well.

> @@ -2495,6 +2502,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 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);
> +
>     /* Since we are not going to really process this INIT, there
>      * is no point in verifying chunk boundries.  Just generate
>      * the SHUTDOWN ACK.
> @@ -3146,6 +3158,11 @@ sctp_disposition_t sctp_sf_ootb(const st
>         ch = (sctp_chunkhdr_t *) ch_end;
>     } while (ch_end < skb_tail_pointer(skb));
> 
> +    /* 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);
> +
>     if (ootb_shut_ack)
>         sctp_sf_shut_8_4_5(ep, asoc, type, arg, commands);
>     else

Hm..  I think you can just replace the 'break' lines in the loop
with a call to sctp_sf_violation_chunklen() can get rid of yet
another conditional.


The rest looks good
Thanks
-vlad

> @@ -3240,6 +3257,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 +3678,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 +3743,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 +3757,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 +3773,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 +3848,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 +3867,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));
> }
> 
> diff -Nurp a/net/sctp/sm_statetable.c b/net/sctp/sm_statetable.c
> --- a/net/sctp/sm_statetable.c    2007-08-09 11:58:11.000000000 -0400
> +++ b/net/sctp/sm_statetable.c    2007-08-19 05:44:29.000000000 -0400
> @@ -110,7 +110,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
>     /* SCTP_STATE_EMPTY */ \
>     TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_CLOSED */ \
> -    TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
> +    TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_COOKIE_WAIT */ \
>     TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
>     /* SCTP_STATE_COOKIE_ECHOED */ \
> @@ -173,7 +173,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
>     /*  SCTP_STATE_EMPTY */ \
>     TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_CLOSED */ \
> -    TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
> +    TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_COOKIE_WAIT */ \
>     TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
>     /* SCTP_STATE_COOKIE_ECHOED */ \
> @@ -194,7 +194,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
>     /*  SCTP_STATE_EMPTY */ \
>     TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_CLOSED */ \
> -    TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
> +    TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_COOKIE_WAIT */ \
>     TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
>     /* SCTP_STATE_COOKIE_ECHOED */ \
> @@ -216,7 +216,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
>     /*  SCTP_STATE_EMPTY */ \
>     TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_CLOSED */ \
> -    TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
> +    TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_COOKIE_WAIT */ \
>     TYPE_SCTP_FUNC(sctp_sf_violation), \
>     /* SCTP_STATE_COOKIE_ECHOED */ \
> @@ -258,7 +258,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
>     /* SCTP_STATE_EMPTY */ \
>     TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_CLOSED */ \
> -    TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
> +    TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_COOKIE_WAIT */ \
>     TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
>     /* SCTP_STATE_COOKIE_ECHOED */ \
> @@ -300,7 +300,7 @@ const sctp_sm_table_entry_t *sctp_sm_loo
>     /* SCTP_STATE_EMPTY */ \
>     TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_CLOSED */ \
> -    TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
> +    TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_COOKIE_WAIT */ \
>     TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
>     /* SCTP_STATE_COOKIE_ECHOED */ \
> @@ -499,7 +499,7 @@ static const sctp_sm_table_entry_t addip
>     /* SCTP_STATE_EMPTY */ \
>     TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_CLOSED */ \
> -    TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8), \
> +    TYPE_SCTP_FUNC(sctp_sf_ootb), \
>     /* SCTP_STATE_COOKIE_WAIT */ \
>     TYPE_SCTP_FUNC(sctp_sf_discard_chunk), \
>     /* SCTP_STATE_COOKIE_ECHOED */ \
> @@ -528,7 +528,7 @@ chunk_event_table_unknown[SCTP_STATE_NUM
>     /* SCTP_STATE_EMPTY */
>     TYPE_SCTP_FUNC(sctp_sf_ootb),
>     /* SCTP_STATE_CLOSED */
> -    TYPE_SCTP_FUNC(sctp_sf_tabort_8_4_8),
> +    TYPE_SCTP_FUNC(sctp_sf_ootb),
>     /* SCTP_STATE_COOKIE_WAIT */
>     TYPE_SCTP_FUNC(sctp_sf_unk_chunk),
>     /* SCTP_STATE_COOKIE_ECHOED */
> 
>