linux-kernel - 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:	Mon, 23 Jul 2012 08:14:11 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	xufeng zhang <xufeng.zhang@...driver.com>
Cc:	xufengzhang.main@...il.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 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.

Regards
Neil

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/