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

[Xin Long]

On Sat, Nov 7, 2026 at 2:52 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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.
>

This requires a rare combination: an unknown source address, an earlier
ADD-IP creating its transport, a subsequent DEL-IP removing the looked-up
transport, and later processing (bundled DATA or wildcard SET_PRIMARY)
relying on chunk->transport.

The impact is minor: an incorrect liveness timestamp or a misrouted SACK
for one packet. No crash, UAF, or data corruption.

I'll use primary_path as the fallback to keep the code simple, as already
done in this patch.

Thanks.