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

[Xin Long]

On Mon, Oct 26, 2026 at 1:45 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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?
>
This looks like a legit issue. Either way above works for me.

Thanks.

> > @@ -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