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

[Simon Horman <horms@kernel.org>]

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 <lucien.xin@gmail.com>

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?

> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
[ ... ]