[PATCH net v4] sctp: carry peer capabilities across an INIT collision

[PATCH net v4] sctp: carry peer capabilities across an INIT collision

[Warren Briggs]

sctp_assoc_update() folds a temporary association into the existing one
when an INIT collision is resolved. It copies asoc->c, peer.rwnd,
peer.sack_needed, peer.auth_capable and peer.i, and nothing else.

The remaining peer capability bits therefore keep whatever the surviving
association was given when it was created, rather than what the peer
advertised in the INIT that caused the collision.

Forward TSN is the visible case. The INIT-ACK is built from the
temporary association, so it advertises Forward-TSN-Supported; once the
collision is resolved the surviving association holds
peer.prsctp_capable == 0, and the first FORWARD TSN chunk the peer sends
is answered with ERROR "Unrecognized chunk type". The peer does not
expect this, having been told the capability was supported.
ecn_capable, asconf_capable and reconf_capable are lost in the same
way.

ASCONF and RE-CONFIG also depend on two inbound sequence counters,
peer.addip_serial and strreset_inseq, which sctp_process_init() derives
from the peer's Initial TSN on the temporary association only. Without
them the surviving association would have the capability but discard
the peer's ASCONF and refuse its RE-CONFIG requests as out of sequence,
so they are re-derived from the copied peer.i.

The two address flags fail the other way round. sctp_process_param()
clears ipv4_address and ipv6_address and sets them from the peer's
Supported Address Types, but only on the temporary association. The
surviving association keeps the permissive defaults from
sctp_association_init(), so it can believe a peer supports an address
family that peer never advertised.

peer.adaptation_ind is recorded on the temporary association in the
same way, so the SCTP_ADAPTATION_INDICATION notification raised after
the merge reads a stale value, or none.

intl_capable is lost too but is deliberately not carried here. It
selects asoc->stream.si and affects the fragmentation point, and the
outqueue may already hold data when the merge runs, so changing it
safely needs more than a copy. It will be addressed in a separate
patch.

peer.auth_capable is already carried, added by commit 1be9a950c646
("net: sctp: inherit auth_capable on INIT collisions") for the same
reason. This extends that to the rest of the block.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Suggested-by: Xin Long <lucien.xin@gmail.com>
Signed-off-by: Warren Briggs <wbriggs@cellusys.com>
---
Changes since v3:
 - re-derive peer.addip_serial and strreset_inseq from the peer's
   Initial TSN, so inbound ASCONF and RE-CONFIG are accepted.
 - dropped intl_capable, to be handled in a separate patch.
 - carry peer.adaptation_ind.

Changes since v2:
 - dropped asoc->peer.hostname_address. The field no longer exists,
   removed by commit bd4b28189469 ("sctp: delete the obsolete code for
   the host name address param").
 - wrapped the commit message at 75 columns.
 - added the Fixes tag.
 - retargeted at net, subject prefix corrected.

Changes since v1:
 - added ecn_capable, asconf_capable, reconf_capable and intl_capable,
   the missing fields identified in review of v1.
 - also added ipv4_address and ipv6_address, which are set from the
   peer's Supported Address Types on the temporary association and are
   lost at the merge in the same way.

Testing. On a 4.18-based kernel carrying this change (the code paths
involved are the same upstream), an INIT collision was created between
two sockets on one host, one of them held in COOKIE_WAIT, and the
surviving association's peer state was read back and exercised in both
directions.

Read back from the collided association, by socket option or sctp_diag:

prsctp_capable   SCTP_PR_SUPPORTED            unpatched 0, patched 1
reconf_capable   SCTP_RECONFIG_SUPPORTED      unpatched 0, patched 1
asconf_capable   SCTP_ASCONF_SUPPORTED        unpatched 0, patched 1
ecn_capable      SCTP_ECN_SUPPORTED           unpatched 0, patched 1
ipv4_address     sctp_diag sctpi_peer_capable unpatched 1, patched 0,
                 with the peer advertising IPv6 only
ipv6_address     sctp_diag sctpi_peer_capable unpatched 1, patched 0,
                 with the peer advertising IPv4 only

Outbound, sent by the collided association, with the patch applied:
 - SCTP_RESET_STREAMS puts a RE-CONFIG on the wire; an unpatched kernel
   refuses the call and sends nothing.
 - sctp_bindx(SCTP_BINDX_ADD_ADDR) puts an ASCONF on the wire; an
   unpatched kernel sends nothing.
 - a PR-SCTP message it abandons is followed by a FORWARD TSN.
 - DATA it receives in a CE-marked packet is answered with an ECNE.

Inbound, sent by the peer to the collided association:
 - a FORWARD TSN is SACKed; an unpatched kernel answers it with ERROR
   cause 6.
 - a stream reset request is performed; v3 answered it with bad
   sequence number.
 - an ASCONF (set primary) gets an ASCONF-ACK; v3 sent none.
 - the adaptation layer indication in the peer's INIT is delivered to
   the application with the value sent; v3 did not deliver it.

 net/sctp/associola.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 4521be3bd85a..77ac155fdb70 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1107,8 +1107,19 @@ int sctp_assoc_update(struct sctp_association *asoc,
 	asoc->peer.rwnd = new->peer.rwnd;
 	asoc->peer.sack_needed = new->peer.sack_needed;
 	asoc->peer.auth_capable = new->peer.auth_capable;
+	asoc->peer.prsctp_capable = new->peer.prsctp_capable;
+	asoc->peer.ecn_capable = new->peer.ecn_capable;
+	asoc->peer.asconf_capable = new->peer.asconf_capable;
+	asoc->peer.reconf_capable = new->peer.reconf_capable;
+	asoc->peer.ipv4_address = new->peer.ipv4_address;
+	asoc->peer.ipv6_address = new->peer.ipv6_address;
+	asoc->peer.adaptation_ind = new->peer.adaptation_ind;
 	asoc->peer.i = new->peer.i;
 
+	/* Re-derive sequence counters from the peer's Initial TSN */
+	asoc->peer.addip_serial = asoc->peer.i.initial_tsn - 1;
+	asoc->strreset_inseq = asoc->peer.i.initial_tsn;
+
 	if (!sctp_tsnmap_init(&asoc->peer.tsn_map, SCTP_TSN_MAP_INITIAL,
 			      asoc->peer.i.initial_tsn, GFP_ATOMIC))
 		return -ENOMEM;
-- 
2.43.7

Re: [PATCH net v4] sctp: carry peer capabilities across an INIT collision

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] On a peer restart (sctp_sf_do_dupcook_a -> sctp_assoc_update,
  ESTABLISHED branch), the patch re-derives only the inbound counters…
- [Low] The commit message says the change only affects INIT collisions
  (Table 2 action B), but it also changes behaviour when the peer…

--

sctp: carry peer capabilities across an INIT collision

When an INIT collision is resolved, this patch makes sctp_assoc_update()
also copy the peer's prsctp, ecn, asconf and reconf capability bits, the
ipv4/ipv6 address flags and adaptation_ind from the temporary
association. It also re-derives the inbound ASCONF and RE-CONFIG sequence
counters (peer.addip_serial and strreset_inseq) from the copied
peer.i.initial_tsn.

> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 4521be3bd85a..77ac155fdb70 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1107,8 +1107,19 @@ int sctp_assoc_update(struct sctp_association *asoc,
>  	asoc->peer.rwnd = new->peer.rwnd;
>  	asoc->peer.sack_needed = new->peer.sack_needed;
>  	asoc->peer.auth_capable = new->peer.auth_capable;
> +	asoc->peer.prsctp_capable = new->peer.prsctp_capable;
> +	asoc->peer.ecn_capable = new->peer.ecn_capable;
> +	asoc->peer.asconf_capable = new->peer.asconf_capable;
> +	asoc->peer.reconf_capable = new->peer.reconf_capable;
> +	asoc->peer.ipv4_address = new->peer.ipv4_address;
> +	asoc->peer.ipv6_address = new->peer.ipv6_address;
> +	asoc->peer.adaptation_ind = new->peer.adaptation_ind;
>  	asoc->peer.i = new->peer.i;

[Severity: Low]
The commit message describes this as an INIT collision fix, but doesn't
the same code also run on a peer restart?

sctp_assoc_update() has one caller, sctp_sf_do_assoc_update(). That is
called from sctp_sf_do_dupcook_b() for the collision case, and also from
sctp_sf_do_dupcook_a() for the peer restart case:

net/sctp/sm_statefuns.c:sctp_sf_do_dupcook_a() {
    ...
	/* Update the content of current association. */
	if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands))
		goto nomem;
    ...
}

These new assignments, and the counter resets below, have no state
check. On every peer restart, a live association in ESTABLISHED,
SHUTDOWN_PENDING or a similar state will now:

  - replace its prsctp, ecn, asconf and reconf capabilities
  - replace its ipv4_address and ipv6_address flags
  - replace peer.adaptation_ind
  - reset peer.addip_serial and strreset_inseq

The restart behaviour may well be correct, but the commit message doesn't
mention it, and the testing only covers a COOKIE_WAIT collision. With
Fixes: 1da177e4c3f4, this goes to all stable trees.

Could the commit message describe the restart path too, and could that
path be tested?

>  
> +	/* Re-derive sequence counters from the peer's Initial TSN */
> +	asoc->peer.addip_serial = asoc->peer.i.initial_tsn - 1;
> +	asoc->strreset_inseq = asoc->peer.i.initial_tsn;
> +

[Severity: Medium]
On the peer restart path (sctp_sf_do_dupcook_a()->sctp_assoc_update(),
state >= SCTP_STATE_ESTABLISHED), only the inbound counters are
re-derived here. Shouldn't the outbound counters asoc->addip_serial and
asoc->strreset_outseq be updated as well?

On restart, sctp_tietags_populate() reuses our Initial TSN:

	new_asoc->c.initial_tsn         = asoc->c.initial_tsn;

and sctp_unpack_cookie() bases the temporary association's outbound
counters on it:

	retval->next_tsn = retval->c.initial_tsn;
	retval->ctsn_ack_point = retval->next_tsn - 1;
	retval->addip_serial = retval->c.initial_tsn;
	retval->strreset_outseq = retval->c.initial_tsn;

The restarted peer runs sctp_process_init() on our INIT-ACK, so it now
expects serials starting from our initial_tsn. The ESTABLISHED branch of
sctp_assoc_update() copies next_tsn, ctsn_ack_point and
adv_peer_ack_point from new. It does not copy new->addip_serial or
new->strreset_outseq.

If N ASCONFs were sent before the restart, sctp_make_asconf() builds the
next one with:

	asconf.serial = htonl(asoc->addip_serial++);

and that serial is initial_tsn + N. The peer's sctp_sf_do_asconf() then
discards it silently, because it is above peer.addip_serial + 1:

	} else {
		return SCTP_DISPOSITION_DISCARD;

Would T4 then keep retransmitting this ASCONF, with every later ASCONF
queued behind it, until sctp_sf_t4_timer_expire() aborts the association
at max_retrans? RE-CONFIG requests would carry a stale strreset_outseq in
the same way, and the peer would answer SCTP_STRRESET_ERR_BAD_SEQNO.

Most of this mismatch predates the patch for associations that already
had asconf_capable or reconf_capable set. However, the patch adds a new
trigger. sctp_cmd_process_operr() clears asconf_capable after an ASCONF
has already used a serial:

	asoc->peer.asconf_capable = 0;

The copy above now sets it back to 1 when the restarted peer advertises
ASCONF. Before this patch, such an association never sent another ASCONF.
Now it sends one with a stale serial.

The COOKIE_WAIT collision case doesn't appear to be affected, because the
local outbound counters have not moved yet at that point.

Should the ESTABLISHED branch take addip_serial and strreset_outseq from
new, next to next_tsn?

>  	if (!sctp_tsnmap_init(&asoc->peer.tsn_map, SCTP_TSN_MAP_INITIAL,
>  			      asoc->peer.i.initial_tsn, GFP_ATOMIC))
>  		return -ENOMEM;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928190401.1641260-1-wbriggs%40cellusys.com