Re: [PATCH net] sctp: validate cached peer INIT chunk length in COOKIE_ECHO processing

On Thu, Jun 4, 2026 at 6:48 AM Simon Horman <[email protected]> wrote:
>
> From: 'Simon Horman' <[email protected]>
>
> 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://sashiko.dev
> ---
> sctp: validate cached peer INIT chunk length in COOKIE_ECHO processing
>
> This commit adds a bounds check to ensure the cached peer INIT chunk
> length does not exceed the available data in the COOKIE_ECHO buffer
> during unpacking.
>
> Simon says:
>
> Hi Xin,
>
> Although the issues flagged below are flagged as pre-existing I am
> wondering if they directly impact the effectiveness of this patch.
> Could you take a look?
>
> > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > index de86ac088289e..85264862fb6b6 100644
> > --- a/net/sctp/sm_make_chunk.c
> > +++ b/net/sctp/sm_make_chunk.c
>
> [ ... ]
>
> > @@ -1759,6 +1760,10 @@ struct sctp_association *sctp_unpack_cookie(
> >       cookie = chunk->subh.cookie_hdr;
> >       bear_cookie = &cookie->c;
> >
> > +     ch = (struct sctp_chunkhdr *)(bear_cookie + 1);
> > +     if (ntohs(ch->length) > len - fixed_size)
> > +             goto malformed;
> > +
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this check also
> need to ensure the length is at least the size of the INIT header?
>
> If ch->length is small, for example 4 bytes for just the chunk header,
> this check passes. However, sctp_process_init() appears to unconditionally
> read the init_hdr fields:
>
> net/sctp/sm_make_chunk.c:sctp_process_init() {
>     ...
>     asoc->peer.i.init_tag = ntohl(peer_init->init_hdr.init_tag);
>     asoc->peer.i.a_rwnd = ntohl(peer_init->init_hdr.a_rwnd);
>     ...
> }
>
> Could an attacker send a COOKIE_ECHO chunk with an embedded INIT chunk
> truncated to 4 bytes and trigger an out-of-bounds read past the SKB payload?
>
> [Severity: High]
> This is a pre-existing issue, but is cookie->raw_addr_list_len properly
> validated against the remaining payload length before use?
>
> If SCTP cookie authentication is disabled, an unvalidated
> raw_addr_list_len could lead to out-of-bounds reads when
> sctp_assoc_set_bind_addr_from_cookie() calls c()
> to parse the bind addresses:
>
> net/sctp/bind_addr.c:sctp_raw_to_bind_addrs() {
>     ...
>     while (addrs_len >= sizeof(struct sctp_paramhdr)) {
>         param = (struct sctp_paramhdr *)raw_addr_list;
>         len = ntohs(param->length);
>         addrs_len -= len;
>         raw_addr_list += len;
>     }
>     ...
> }
>
> If the unvalidated addrs_len is artificially large, or if a supplied
> param->length is larger than addrs_len causing an integer underflow on
> addrs_len -= len, could this bypass the loop condition, read into unmapped
> pages, and cause a panic?
>
Yes, these seem like real issues.

We likely need more cookie validation than just checking the chunk length
and raw_addr_list_len. This was probably missed because the cookie is
generated locally, but it can still be maliciously modified.

I will try to reproduce and get them fixed.

Thanks.