[PATCH net v3] sctp: fix ASCONF address change on single-homed hosts

[PATCH net v3] sctp: fix ASCONF address change on single-homed hosts

[Andrey Misiurov]

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: stable@vger.kernel.org
Signed-off-by: Andrey Misiurov <andrey.misyrov@gmail.com>
---
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/20261029193454.1028626-1-andrey.misyrov@gmail.com/

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/20261022163639.2566945-1-andrey.misyrov@gmail.com/

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

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index ddc31a5..9719464 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -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.
-		 */
-		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;
 		break;
 	case SCTP_PARAM_SET_PRIMARY:
 		/* ADDIP Section 4.2.4

Re: [PATCH net v3] sctp: fix ASCONF address change on single-homed hosts

[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] 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.

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 90de272842512..15c2d87575a8a 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -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?

> +			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;
>  
> -		/* 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.

>  		break;
>  	case SCTP_PARAM_SET_PRIMARY:
>  		/* ADDIP Section 4.2.4

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106195126.1563231-1-andrey.misyrov%40gmail.com