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

[Xin Long]

On Mon, Nov 2, 2026 at 3:18 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 (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?

Looks like ->transport can't be NULL at any point. I couldn't find anything
in the RFCs about this, so setting it to asoc->peer.primary_path should be
good enough.

Thanks.

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