netdev - Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+=dFzh1wQx2rBs_2RAwrXsz79WS3njnO8=2ntQZUbB5So69gg@mail.gmail.com>
Date:	Mon, 30 Jul 2012 13:47:22 +0800
From:	Xufeng Zhang <xufengzhang.main@...il.com>
To:	Vlad Yasevich <vyasevich@...il.com>
Cc:	Neil Horman <nhorman@...driver.com>,
	xufeng zhang <xufeng.zhang@...driver.com>, sri@...ibm.com,
	davem@...emloft.net, linux-sctp@...r.kernel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK
 when bundling

On 7/30/12, Xufeng Zhang <xufengzhang.main@...il.com> wrote:
> On 7/28/12, Vlad Yasevich <vyasevich@...il.com> wrote:
>> here is an untested prototype of what I was talking about.  This should
>> handle multiple data chunks.
>
> Yes, it works if only the end of the DATA chunk in a packet has
> invalid stream identifier
> and I have verified this patch by my test case, but what happens if
> there are multiple
> DATA chunks which have invalid stream identifier in a packet?
>
> Consider the below example:
> A packet has several chunks bundling together: "COOKIE_ECHO DATA DATA",
> both
> of the two DATA chunks have invalid stream identifier, then the
> response will be
> "COOKIE_ACK ERROR SACK ERROR", right?

I just wrote a test case for my above assumption and have verified
that SACK always
bundled before the end of the ERROR chunk if multiple error DATA
chunks happened.

So this patch didn't handle all the situations and this is really what
I suspected before.


Thanks,
Xufeng Zhang
>
>
>
> Thanks,
> Xufeng Zhang
>
>>
>> -vlad
>>
>> ---
>>   include/net/sctp/command.h |    1 +
>>   net/sctp/sm_sideeffect.c   |   22 ++++++++++++++++++++++
>>   net/sctp/sm_statefuns.c    |   18 ++++++++++--------
>>   3 files changed, 33 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
>> index 712b3be..4043445 100644
>> --- a/include/net/sctp/command.h
>> +++ b/include/net/sctp/command.h
>> @@ -110,6 +110,7 @@ typedef enum {
>>   	SCTP_CMD_SEND_NEXT_ASCONF, /* Send the next ASCONF after ACK */
>>   	SCTP_CMD_PURGE_ASCONF_QUEUE, /* Purge all asconf queues.*/
>>   	SCTP_CMD_SET_ASOC,	 /* Restore association context */
>> +	SCTP_CMD_GEN_BAD_STREAM, /* Issue an Invalid Stream error */
>>   	SCTP_CMD_LAST
>>   } sctp_verb_t;
>>
>> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
>> index 1ff51c9..c5a1322 100644
>> --- a/net/sctp/sm_sideeffect.c
>> +++ b/net/sctp/sm_sideeffect.c
>> @@ -1036,6 +1036,22 @@ static void sctp_cmd_send_asconf(struct
>> sctp_association *asoc)
>>   	}
>>   }
>>
>> +static void sctp_cmd_make_inv_stream_err(sctp_cmd_seq_t *commands,
>> +					 struct sctp_association *asoc,
>> +					 struct sctp_chunk *chunk,
>> +					 struct sctp_datahdr *data_hdr)
>> +{
>> +	struct sctp_chunk *err;
>> +
>> +	err = sctp_make_op_error(asoc, chunk, SCTP_ERROR_INV_STRM,
>> +				 &data_hdr->stream,
>> +				 sizeof(data_hdr->stream),
>> +				 sizeof(u16));
>> +	if (err)
>> +		sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
>> +					SCTP_CHUNK(err));
>> +}
>> +
>>
>>   /* These three macros allow us to pull the debugging code out of the
>>    * main flow of sctp_do_sm() to keep attention focused on the real
>> @@ -1700,6 +1716,12 @@ static int sctp_cmd_interpreter(sctp_event_t
>> event_type,
>>   			asoc = cmd->obj.asoc;
>>   			break;
>>
>> +		case SCTP_CMD_GEN_BAD_STREAM:
>> +			sctp_cmd_make_inv_stream_err(commands,
>> +					 asoc, chunk,
>> +					 (struct sctp_datahdr *)cmd->obj.ptr);
>> +			break;
>> +
>>   		default:
>>   			pr_warn("Impossible command: %u, %p\n",
>>   				cmd->verb, cmd->obj.ptr);
>> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
>> index 891f5db..57532e3 100644
>> --- a/net/sctp/sm_statefuns.c
>> +++ b/net/sctp/sm_statefuns.c
>> @@ -2972,6 +2972,12 @@ discard_noforce:
>>   	if (chunk->end_of_packet)
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force);
>>
>> +	/* Queue the INVALID STREAM error after the SACK if one is needed. */
>> +	if (SCTP_IERROR_BAD_STREAM == error) {
>> +		sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM,
>> +				SCTP_PTR(chunk->subh.data_hdr));
>> +	}
>> +
>>   	return SCTP_DISPOSITION_DISCARD;
>>   consume:
>>   	return SCTP_DISPOSITION_CONSUME;
>> @@ -3044,6 +3050,10 @@ sctp_disposition_t
>> sctp_sf_eat_data_fast_4_4(const struct sctp_endpoint *ep,
>>   		 */
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SHUTDOWN, SCTP_NULL());
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE());
>> +		if (SCTP_IERROR_BAD_STREAM == error) {
>> +			sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM,
>> +					SCTP_PTR(chunk->subh.data_hdr));
>> +		}
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
>>   				SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
>>   	}
>> @@ -6140,14 +6150,6 @@ static int sctp_eat_data(const struct
>> sctp_association *asoc,
>>   	if (sid >= asoc->c.sinit_max_instreams) {
>>   		/* Mark tsn as received even though we drop it */
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_REPORT_TSN, SCTP_U32(tsn));
>> -
>> -		err = sctp_make_op_error(asoc, chunk, SCTP_ERROR_INV_STRM,
>> -					 &data_hdr->stream,
>> -					 sizeof(data_hdr->stream),
>> -					 sizeof(u16));
>> -		if (err)
>> -			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
>> -					SCTP_CHUNK(err));
>>   		return SCTP_IERROR_BAD_STREAM;
>>   	}
>>
>> -- 1.7.7.6
>>
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html