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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 24 Jul 2012 09:53:02 +0800
From:	Xufeng Zhang <xufengzhang.main@...il.com>
To:	Neil Horman <nhorman@...driver.com>
Cc:	xufeng zhang <xufeng.zhang@...driver.com>, vyasevich@...il.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/23/12, Neil Horman <nhorman@...driver.com> wrote:
> On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote:
>> On 07/23/2012 08:49 AM, Neil Horman wrote:
>> >
>> >Not sure I understand how you came into this error.  If we get an
>> > invalid
>> >stream, we issue an SCTP_REPORT_TSN side effect, followed by an
>> > SCTP_CMD_REPLY
>> >which sends the error chunk.  The reply goes through
>> >sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk.
>> >That last function checks to see if a sack is already part of the packet,
>> > and if
>> >there isn't one, appends one, using the updated tsn map.
>> Yes, you are right, but consider the invalid stream identifier's
>> DATA chunk is the first
>> DATA chunk in the association which will need SACK immediately.
>> Here is what I thought of the scenario:
>>     sctp_sf_eat_data_6_2()
>>         -->sctp_eat_data()
>>             -->sctp_make_op_error()
>>             -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err))
>>             -->sctp_outq_tail()          /* First enqueue ERROR chunk */
>>         -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE())
>>             -->sctp_gen_sack()
>>                 -->sctp_make_sack()
>>                 -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
>> SCTP_CHUNK(sack))
>>                 -->sctp_outq_tail()          /* Then enqueue SACK chunk
>> */
>>
>> So SACK chunk is enqueued after ERROR chunk.
> Ah, I see.  Since the ERROR and SACK chunks are both control chunks, and
> since
> we explicitly add the SACK to the control queue instead of going through
> the
> bundle path in sctp_packet_append_chunk the ordering gets wrong.
>
> Ok, so the problem makes sense.  I think the soultion could be alot easier
> though.  IIRC SACK chunks always live at the head of a packet, so why not
> just
> special case it in sctp_outq_tail?  I.e. instead of doing a list_add_tail,
> in
> the else clause of sctp_outq_tail check the chunk_hdr->type to see if its
> SCTP_CID_SACK.  If it is, use list_add_head rather than list_add_tail.  I
> think
> that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it?  And
> then
> you won't have keep track of extra state in the packet configuration.

(Please ignore the duplicate messages if you received, sorry for this!)

Yes, it's a good idea, but I think the premise is not correct:
RFC 4960 page 57:
"D) Upon reception of the COOKIE ECHO chunk, endpoint "Z" will reply
   with a COOKIE ACK chunk after building a TCB and moving to the
   ESTABLISHED state. A COOKIE ACK chunk may be bundled with any
   pending DATA chunks (and/or SACK chunks), but the COOKIE ACK chunk
   MUST be the first chunk in the packet."

So we can't put SACK chunk always at the head of the packet.


Thanks,
Xufeng Zhang

>
> Regards
> Neil
>
>
--
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