Re: [PATCH net] 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 · Low: 0 (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).

- [Critical] The patch rewrites the wildcard DEL-IP (0.0.0.0) branch of
  sctp_process_asconf_param() (net/sctp/sm_make_chunk.c:3153-3159) to…

Pre-existing issues:
- [Low] The wildcard SET_PRIMARY branch of sctp_process_asconf_param()
  (net/sctp/sm_make_chunk.c:3182) takes the packet source address from…

--

sctp: fix ASCONF address change on single-homed hosts

This patch drops the refusal of a plain DEL-IP of asconf->transport that
commit 9b2854f86f0b added, so the RFC 5061 5.3.2 single-homed address
change works again. The wildcard DEL-IP branch now looks up the
transport of the packet source instead of reusing asconf->transport. The
plain DEL-IP branch now clears asconf->transport when it removes that
peer.

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index ddc31a5dbad32..2b76e451d5cb0 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -3151,9 +3151,11 @@ 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);
>  			return SCTP_ERROR_NO_ERROR;
>  		}

[Severity: Critical]

Can this leave asconf->transport pointing at a freed transport?

Suppose __sctp_rcv_asconf_lookup() found the association through the
ASCONF Address Parameter L. Then asconf->transport is T(L), while
asconf->source is the packet source S. Take a packet from a new source S
like this:

  [AUTH][ASCONF: AddrParam L, ADD-IP S (or ADD-IP 0.0.0.0), DEL-IP 0.0.0.0][DATA]

ADD-IP S creates T(S). The wildcard DEL-IP then keeps T(S) and removes
everything else, including T(L):

sctp_assoc_del_nonprimary_peers(asoc, T(S))
  sctp_assoc_rm_peer(asoc, T(L))
    sctp_transport_free(T(L))

Nothing in this branch clears or retargets asconf->transport. Only the
plain DEL-IP branch below does that.

sctp_inq_pop() reuses the same chunk struct for the bundled DATA chunk.
sctp_assoc_bh_rcv() then runs:

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

This stores the removed T(L). The last_data_from fixup in
sctp_assoc_rm_peer() has already run, so it does not catch this.
sctp_rcv() then drops its lookup reference, and T(L) is freed after the
RCU grace period.

When the delayed SACK timer fires:

sctp_gen_sack()
  trans = asoc->peer.last_data_from
  sctp_make_sack()
    retval->transport = asoc->peer.last_data_from;
  ...
  sctp_outq_select_transport()
    reads new_transport->state
    list_add_tail(&ctx->transport->send_ready, &ctx->transport_list)

Is that a use-after-free read and write of the transport?

Before this patch, the wildcard branch always kept asconf->transport, and
9b2854f86f0b refused a plain DEL-IP of it. So asconf->transport could not
be freed during ASCONF processing.

Any peer of an association with ASCONF enabled appears able to reach
this, including a well-behaved peer following the RFC 5061 wildcard
procedure.

Would it close this to set asconf->transport = peer after
sctp_assoc_del_nonprimary_peers(), or to clear it whenever it is not the
surviving transport?

> @@ -3166,13 +3168,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: Low]

This is a pre-existing issue and was not introduced by this patch. The
wildcard SET_PRIMARY handling in sctp_process_asconf_param() uses
sctp_source() instead of &asconf->source:

	if (af->is_any(&addr))
		memcpy(&addr, sctp_source(asconf), sizeof(addr));

sctp_source() returns &chunk->transport->ipaddr whenever chunk->transport
is set. When the association was located via the Address Parameter
(transport T(L), source S), SET_PRIMARY 0.0.0.0 makes L primary instead
of the packet source S. That doesn't match the comment. The ADD-IP and
DEL-IP wildcard branches both use &asconf->source.

With this patch the result also depends on parameter order:

- After a plain DEL-IP L clears asconf->transport, sctp_source() returns
  S.
- After a wildcard DEL-IP, it returns the ipaddr of the removed T(L), the
  dangling pointer described above. The lookup then fails and the
  parameter gets SCTP_ERROR_DNS_FAILED.

Should this branch use &asconf->source like the other two?

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