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

[netdev-bot+sashiko@kernel.org]

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