Re: [PATCH net 2/2] sctp: add INIT verification after cookie unpacking
From: Xin Long <lucien.xin@gmail.com>
Date: 2026-06-18 22:09:05
Also in:
linux-sctp
On Wed, Jun 17, 2026 at 6:40 AM Simon Horman [off-list ref] 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 [off-list ref] 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).quoted
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.