sctp: fix ASCONF address change on single-homed hosts

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] In sctp_process_asconf_param() (net/sctp/sm_make_chunk.c:3181-3183), a plain DEL-IP that removes…
- [Low] The rewritten wildcard DEL-IP branch in sctp_process_asconf_param() (net/sctp/sm_make_chunk.c:3162-3170)…

--

sctp: fix ASCONF address change on single-homed hosts

This patch removes the refusal of a DEL-IP for the ASCONF's own transport that commit 9b2854f86f0b added, since that refusal broke the RFC 5061 single-homed renumber sequence. The wildcard DEL-IP now looks up the transport for the packet source, and both DEL-IP paths retarget asconf->transport to a surviving transport.

Quoted 5 lines

net/sctp/sm_make_chunk.c +0 -0

index 90de272842512..15c2d87575a8a 100644

net/sctp/sm_make_chunk.c static __be16 sctp_process_asconf_param(struct sctp_association *asoc, · line 3165

Quoted 9 lines

net/sctp/sm_make_chunk.c +3 -3

@@ -3160,9 +3160,12 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,
               * packet MUST be deleted.
               */
              if (af->is_any(&addr)) {
-                     sctp_assoc_set_primary(asoc, asconf->transport);
-                     sctp_assoc_del_nonprimary_peers(asoc,
-                                                     asconf->transport);
+                     peer = sctp_assoc_lookup_paddr(asoc, &asconf->source);
+                     if (!peer)
+                             return SCTP_ERROR_REQ_REFUSED;

[Severity: Low]

This changes how a wildcard DEL-IP behaves on the wire. Before, this branch always made asconf->transport primary, removed every other peer address and returned SCTP_ERROR_NO_ERROR.

Now the request is refused if the packet source is not already a transport of the association. That can happen when the ASCONF was found through its Address Parameter and no earlier ADD-IP in the same chunk added the source. For example:

    [Addr L] [DEL-IP 0.0.0.0] [ADD-IP S]

The delete used to succeed here. Now it returns an error cause and all the other peer addresses stay.

Refusing may well be more correct. However, the commit message only says the branch now "look[s] up the transport of the packet source rather than reusing asconf->transport". Since this is tagged for stable, could the changelog mention the new SCTP_ERROR_REQ_REFUSED case?

Quoted 9 lines

net/sctp/sm_make_chunk.c +3 -0

+                     sctp_assoc_set_primary(asoc, peer);
+                     sctp_assoc_del_nonprimary_peers(asoc, peer);
+                     asconf->transport = peer;
                      return SCTP_ERROR_NO_ERROR;
              }
 

@@ -3175,13 +3178,9 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,
              if (!peer)
                      return SCTP_ERROR_DNS_FAILED;

net/sctp/sm_make_chunk.c static __be16 sctp_process_asconf_param(struct sctp_association *asoc, · line 3183

Quoted 10 lines

net/sctp/sm_make_chunk.c +2 -6

 
-             /* Don't free asconf->transport; a later wildcard DEL-IP
-              * parameter reuses it.
-              */
-             if (peer == asconf->transport)
-                     return SCTP_ERROR_REQ_REFUSED;
-
              sctp_assoc_rm_peer(asoc, peer);
+             if (asconf->transport == peer)
+                     asconf->transport = asoc->peer.primary_path;

[Severity: Medium]

Can primary_path be a transport other than the packet source here?

The D8 check earlier in sctp_process_asconf_param() returns SCTP_ERROR_DEL_SRC_IP, so this branch only removes asconf->transport when two things are true. The association was found through the Address Parameter L, and the packet came from a different address S.

In that case a transport for S exists only if an earlier ADD-IP S in the same chunk created it. That is the [Addr L] [ADD-IP S] [DEL-IP L] sequence this patch fixes.

With two peer addresses after the ADD, sctp_assoc_rm_peer() makes the first remaining entry (T_S) primary, so this works. With three or more addresses it does not seem to. For example, with peer addresses {A (primary), L}:

    ADD-IP S  -> {A (primary), L, S}
    DEL-IP L  -> sctp_assoc_rm_peer(asoc, T_L), primary stays T_A
                 asconf->transport = T_A

This happens even though T_S exists.

sctp_inq_pop() reuses the same sctp_chunk for every chunk bundled after the ASCONF and does not reset chunk->transport. Those chunks are then credited to T_A in three places.

First, sctp_vtag_verify() copies the packet's encap_port into T_A:

    chunk->transport->encap_port = SCTP_INPUT_CB(chunk->skb)->encap_port;

Second, sctp_assoc_bh_rcv() updates last_data_from and last_time_heard on T_A, so the SACK for bundled DATA goes to A:

    if (sctp_chunk_is_data(chunk))
        asoc->peer.last_data_from = chunk->transport;
    ...
    if (chunk->transport)
        chunk->transport->last_time_heard = ktime_get();

Third, a wildcard SET_PRIMARY later in the same ASCONF resolves sctp_source(asconf) to T_A->ipaddr. A then becomes primary instead of the packet source, and the request is still acked as a success.

Before this patch, bundled chunks in this case were credited to T_L, which was not the source either. Still, the new code picks a transport other than S even when T_S exists.

The wildcard branch in this same patch already calls sctp_assoc_lookup_paddr(asoc, &asconf->source). Would it make sense to do that here too, and fall back to asoc->peer.primary_path only when there is no transport for the source? That would also match the "Remember where we came from" meaning that sctp_rcv() gives chunk->transport.

Commit 9b2854f86f0b ("sctp: don't free the ASCONF's own transport in DEL-IP processing") refuses a DEL-IP of the transport the ASCONF was located by, so that a later wildcard DEL-IP cannot use it after free.

This also breaks the single-homed address change (RFC 5061 5.3.2). The host sends [Address Parameter L] [ADD-IP S] [DEL-IP L], the association is located by L, and DEL-IP L is now refused. The sender never gets an all-success ASCONF-ACK, keeps src_out_of_asoc_ok set and stops sending data, so the association stalls.

Fix the wildcard DEL-IP instead: look up the transport of the packet source rather than reusing asconf->transport. Both DEL-IP paths can now free asconf->transport, and chunks bundled after the ASCONF still use it, so point it at a surviving transport: the packet source for the wildcard delete, the primary path for a plain one.

Found by manual code inspection while reviewing a backport of commit 9b2854f86f0b; reproduced on a two-namespace single-homed renumber.

Fixes 9b2854f86f0b ("sctp: don't free the ASCONF's own transport in DEL-IP processing")

Cc [email protected]

Signed-off-by Andrey Misiurov <[email protected]>

---
v3: a plain DEL-IP of asconf->transport set it to NULL, but chunks

    bundled after the ASCONF dereference chunk->transport (e.g.
    sctp_vtag_verify()); point it at asoc->peer.primary_path instead,
    as suggested by Xin Long. Reported by the netdev Sashiko AI review.
    v2: https://lore.kernel.org/all/[email protected]/

v2: the reworked wildcard DEL-IP branch could itself free

    asconf->transport via sctp_assoc_del_nonprimary_peers() and leave it
    dangling for a bundled DATA chunk (sctp_assoc_bh_rcv() ->
    peer.last_data_from) and the following SACK; retarget
    asconf->transport to the surviving peer. The pre-existing wildcard
    SET_PRIMARY / sctp_source() point is left out of scope.
    Reported by the netdev Sashiko AI review.
    v1: https://lore.kernel.org/all/[email protected]/

 net/sctp/sm_make_chunk.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

net/sctp/sm_make_chunk.c +6 -5

index ddc31a5..9719464 100644
@@ -3151,9 +3151,12 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,
                 * packet MUST be deleted.
                 */
                if (af->is_any(&addr)) {
-                       sctp_assoc_set_primary(asoc, asconf->transport);
-                       sctp_assoc_del_nonprimary_peers(asoc,
-                                                       asconf->transport);
+                       peer = sctp_assoc_lookup_paddr(asoc, &asconf->source);
+                       if (!peer)
+                               return SCTP_ERROR_REQ_REFUSED;
+                       sctp_assoc_set_primary(asoc, peer);
+                       sctp_assoc_del_nonprimary_peers(asoc, peer);
+                       asconf->transport = peer;
                        return SCTP_ERROR_NO_ERROR;
                }
 

@@ -3166,13 +3169,9 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,
                if (!peer)
                        return SCTP_ERROR_DNS_FAILED;
 
-               /* Don't free asconf->transport; a later wildcard DEL-IP
-                * parameter reuses it.