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

[PATCH net v2] 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, and clear
asconf->transport when a plain DEL-IP removes it. The wildcard delete
can now free the old asconf->transport itself, so point it at the
surviving peer; otherwise a bundled chunk would reuse the freed
transport.

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>
---
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 = NULL;
 		break;
 	case SCTP_PARAM_SET_PRIMARY:
 		/* ADDIP Section 4.2.4

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

[netdev-bot+sashiko]

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

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

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).

- [Critical] sctp: NULL pointer dereference in sctp_vtag_verify() after
  plain DEL-IP

Pre-existing issues:
- [Medium] This problem was not introduced by this patch, but the
  wildcard SET_PRIMARY branch of sctp_process_asconf_param() does not…

--

sctp: fix ASCONF address change on single-homed hosts

This patch changes DEL-IP handling in sctp_process_asconf_param(). A plain
DEL-IP of the transport that located the ASCONF is no longer refused, so
the single-homed renumber case from RFC 5061 5.3.2 works again. The
wildcard DEL-IP now looks up the peer by the packet source, and
asconf->transport is either cleared or pointed at the surviving peer, so
no freed transport is left behind.

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index d64c3ae33298f..0f9538f8040f2 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -3153,9 +3153,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;
>  		}
>  
> @@ -3168,13 +3171,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 = NULL;
>  		break;
>  	case SCTP_PARAM_SET_PRIMARY:
>  		/* ADDIP Section 4.2.4

[Severity: Critical]
Clearing asconf->transport here can leave a NULL transport for any
chunk bundled after the ASCONF in the same packet.

sctp_rcv() allocates one struct sctp_chunk per packet. sctp_inq_pop()
reuses queue->in_progress for every bundled chunk and does not reset
chunk->transport. The asconf seen here through sctp_sf_do_asconf()
and sctp_process_asconf() is therefore the same object that
sctp_assoc_bh_rcv() passes to sctp_do_sm() for the next chunk.

Once this branch sets asconf->transport to NULL, the next bundled
chunk reaches a handler that calls sctp_vtag_verify(). That covers
DATA, SACK, HEARTBEAT, a second ASCONF, or even an unknown chunk.
sctp_vtag_verify() does this:

include/net/sctp/sm.h:sctp_vtag_verify() {
	if (ntohl(chunk->sctp_hdr->vtag) != asoc->c.my_vtag)
		return 0;

	chunk->transport->encap_port =
		SCTP_INPUT_CB(chunk->skb)->encap_port;
	return 1;
}

The vtag matches for a legitimate peer, so this is a NULL pointer
dereference in softirq context. For a bundled DATA chunk,
sctp_assoc_bh_rcv() also stores the NULL into
asoc->peer.last_data_from just before that.

The trigger is the case this patch is meant to fix. The peer
renumbers from L to S and sends [Address Parameter L] [ADD-IP S]
[DEL-IP L] from S. __sctp_rcv_asconf_lookup() locates the association
by L, so asconf->transport is the transport for L. The ADD-IP raises
transport_count to 2, so D7 does not refuse the request, and the
plain DEL-IP now removes L. Any peer of an established association
can bundle another chunk after that ASCONF, so a remote peer can
trigger this.

Before 9b2854f86f0b this path left a freed transport in
asconf->transport. The patch replaces that use-after-free with a
NULL dereference for the same bundled chunks rather than closing it.

The v2 notes already handle the bundled DATA/SACK concern for the
wildcard branch by retargeting asconf->transport to the surviving
peer. Should the plain DEL-IP do the same? One option is the
transport for asconf->source when it exists, which it will after
ADD-IP S. Otherwise asoc->peer.primary_path could be used, since
sctp_assoc_rm_peer() has just updated it. Or do you prefer to make
the receive-path users of chunk->transport, starting with
sctp_vtag_verify(), tolerate NULL?

[Severity: Medium]
This is a pre-existing issue, and this patch did not introduce it. The v2
notes already put it out of scope. It does interact with the new
asconf->transport updates above, though.

The wildcard SET_PRIMARY branch just below says it uses the packet source:

	case SCTP_PARAM_SET_PRIMARY:
		/* ADDIP Section 4.2.4
		 * If the address 0.0.0.0 or ::0 is provided, the receiver
		 * MAY mark the source address of the packet as its
		 * primary.
		 */
		if (af->is_any(&addr))
			memcpy(&addr, sctp_source(asconf), sizeof(addr));

However, sctp_source() returns the transport address whenever one is set:

net/sctp/sm_make_chunk.c:sctp_source() {
	if (chunk->transport) {
		return &chunk->transport->ipaddr;
	} else {
		/* Otherwise, extract it from the IP header.  */
		return &chunk->source;
	}
}

If the ASCONF was matched by its Address Parameter in
__sctp_rcv_asconf_lookup(), asconf->transport is the transport for L, while
asconf->source is the real packet source S.

So if [Addr L][ADD-IP S][SET-PRIMARY 0.0.0.0] is sent from S, wouldn't L
become primary instead of S?

The ADD-IP wildcard branch, the D8 check and the reworked DEL-IP wildcard
branch all use &asconf->source. That leaves "source" with two meanings
inside sctp_process_asconf_param().

This patch now rewrites asconf->transport partway through the chunk. As a
result, the wildcard SET_PRIMARY result depends on parameter order:

  - after a plain DEL-IP of asconf->transport (now NULL), it picks S
  - after a wildcard DEL-IP (retargeted to the peer for S), it picks S
  - otherwise it still picks L

None of these orderings uses a freed transport. Where the result changes,
it moves toward the behaviour the comment describes.

Would it make sense to have the wildcard SET_PRIMARY branch use
&asconf->source directly, like the other wildcard branches, either here or
in a follow-up? That would make the result independent of parameter order.

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