Re: [PATCH net 2/2] sctp: add INIT verification after cookie unpacking

On Wed, Jun 17, 2026 at 6:40 AM Simon Horman <[email protected]> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> Now I have enough verified information to produce the report.
>
> commit 9d74027bfd0cb22a103cca034883f0fcb4e2535a
> Author: Xin Long <[email protected]>
>
> sctp: add INIT verification after cookie unpacking
>
> This commit adds sctp_verify_init() checks after cookie unpacking in
> the COOKIE-ECHO processing paths when cookie_auth_enable is disabled.
> It also changes the bounds comparison inside sctp_verify_init() from
> chunk->chunk_end to peer_init + ntohs(peer_init->chunk_hdr.length).
>
> > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > index 41958b8e59fd..21b9eb1c02e9 100644
> > --- a/net/sctp/sm_make_chunk.c
> > +++ b/net/sctp/sm_make_chunk.c
> > @@ -2298,7 +2298,7 @@ int sctp_verify_init(struct net *net, const struct sctp_endpoint *ep,
> >        * VIOLATION error.  We build the ERROR chunk here and let the normal
> >        * error handling code build and send the packet.
> >        */
> > -     if (param.v != (void *)chunk->chunk_end)
> > +     if (param.v != (void *)peer_init + ntohs(peer_init->chunk_hdr.length))
> >               return sctp_process_inv_paramlength(asoc, param.p, chunk, errp);
>
> [High]
> Can this new comparison reject INIT chunks whose final parameter has a
> length that is not a multiple of 4?
>
> sctp_walk_params() advances pos.v by SCTP_PAD4(ntohs(pos.p->length)) on
> every iteration:
>
>   include/net/sctp/sctp.h:
>   #define _sctp_walk_params(pos, chunk, end)\
>   for (pos.v = (u8 *)(chunk + 1);\
>        ...
>        pos.v += SCTP_PAD4(ntohs(pos.p->length)))
>
> so when the final parameter has an unpadded length L that is not 4-byte
> aligned, pos.v ends at peer_init + offset_of_last_param + SCTP_PAD4(L),
> which exceeds peer_init + ntohs(peer_init->chunk_hdr.length) by 1..3.
>
> The previous comparison used chunk->chunk_end, which inqueue.c computes
> with the padding included:
>
>   net/sctp/inqueue.c:
>   chunk->chunk_end = ((__u8 *)ch) + SCTP_PAD4(ntohs(ch->length));
>
> so the old check accepted unaligned-length final parameters; the new
> check returns 0 from sctp_verify_init(), and the callers then invoke
> sctp_abort_on_init_err() and emit an ABORT.
>
> Note that this change is in sctp_verify_init() itself, so it affects
> every caller, not just the new COOKIE-ECHO sites: sctp_sf_do_5_1B_init,
> sctp_sf_do_5_1C_ack, sctp_sf_do_unexpected_init, sctp_sf_do_5_1D_ce
> and sctp_sf_do_5_2_4_dupcook all reach this comparison.
>
> Realistic triggers include:
>
>   - Locally-generated INITs constructed by sctp_make_init() when both
>     auth_enable and asconf_enable are set: with the default chunk IDs
>     ASCONF + ASCONF_ACK the AUTH-CHUNKS parameter has length 6 and is
>     typically the last parameter, leaving chunk_hdr.length unaligned.
>
>   - Peer INITs containing any final parameter with non-4-aligned
>     length, e.g. SCTP_PARAM_SUPPORTED_ADDRESS_TYPES with 1 or 3 types
>     last, AUTH-CHUNKS with N=1,2,3 entries last, or any unknown TLV.
>
> Even in the COOKIE-ECHO context the new bound seems too strict, since
> sctp_pack_cookie() copies the INIT verbatim with memcpy(cookie + 1,
> init_chunk->chunk_hdr, ntohs(...->length)) followed immediately by
> raw_addrs, so unpacked cookies recovered from peers that sent an INIT
> with an unaligned-length final parameter would also be spuriously
> rejected.
>
> Would comparing against peer_init + SCTP_PAD4(ntohs(peer_init->
> chunk_hdr.length)) preserve the long-standing semantics of chunk_end
> while still validating the cookie-derived peer_init bounds?
>
There's no problem here, as INIT or INIT_ACK chunk's length is always
a multiple of 4, unless it's an abnormal one. We don't need SCTP_PAD4()
for this check.

Thanks.