Re: [PATCH v2] sctp: diag: fix uninitialized stack leak via INET_DIAG_LOCALS/PEERS

On Mon, Aug 3, 2026 at 7:37 PM Jakub Kicinski <[email protected]> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email says "AI seems to have spotted another related stack leak, and
> suggests an alternative fix."
> ---
> sctp: diag: fix uninitialized stack leak via INET_DIAG_LOCALS/PEERS
>
> The patch changes inet_diag_msg_sctpladdrs_fill() and
> inet_diag_msg_sctpaddrs_fill() to zero the whole sockaddr_storage slot
> first and then copy only the family-dependent prefix of the stored
> union sctp_addr, so the sockaddr_in6-only tail of an AF_INET address is
> no longer exported through the netlink attributes.
>
> A few questions about the changelog and the placement of the fix.
>
> [Severity: Low]
> [Finding: main-1-2]
> [Sources: opus-5]
> The changelog says:
>
> >     inet_diag_msg_sctpladdrs_fill() copies sizeof(union sctp_addr) (28 bytes,
> >     the size of sockaddr_in6) from each sctp_sockaddr_entry.a into the netlink
> >     INET_DIAG_LOCALS attribute and then only zeroes the bytes from offset 28 to
> >     sizeof(sockaddr_storage).  The same pattern is used by
> >     inet_diag_msg_sctpaddrs_fill() for INET_DIAG_PEERS.
>
> Can the INET_DIAG_PEERS path actually leak anything?  transport->ipaddr
> is allocated zeroed and only ever written with sockaddr_len bytes:
>
> net/sctp/transport.c:sctp_transport_new() {
>         transport = kzalloc_obj(*transport, gfp);
>         ...
> }
>
> net/sctp/transport.c:sctp_transport_init() {
>         peer->af_specific = sctp_get_af_specific(addr->sa.sa_family);
>         memcpy(&peer->ipaddr, addr, peer->af_specific->sockaddr_len);
>         ...
> }
>
> So bytes 16..27 of from->ipaddr look guaranteed to be zero for AF_INET,
> which would make the inet_diag_msg_sctpaddrs_fill() change hardening
> rather than a fix.  Should the changelog say that instead?
>
> The changelog also says:
>
> >     region) are left uninitialized.  Those bytes are propagated verbatim through
> >     sctp_add_bind_addr() (which copies sizeof(union sctp_addr)=28 bytes) and then
>
> sctp_add_bind_addr() copies a caller-supplied length:
>
> net/sctp/bind_addr.c:sctp_add_bind_addr() {
>         memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));
>         ...
> }
>
> Callers such as sctp_do_bind() pass af->sockaddr_len and store nothing
> uninitialized; only the callers that pass sizeof(union sctp_addr) do.
> Could that be spelled out so the scope of the stable backport is clear?
>
> > diff --git a/net/sctp/diag.c b/net/sctp/diag.c
> > index c2a0de2adf6fd..610c937deffd7 100644
> > --- a/net/sctp/diag.c
> > +++ b/net/sctp/diag.c
> > @@ -85,8 +85,12 @@ static int inet_diag_msg_sctpladdrs_fill(struct sk_buff *skb,
> >       info = nla_data(attr);
> >       rcu_read_lock();
> >       list_for_each_entry_rcu(laddr, address_list, list) {
> > -             memcpy(info, &laddr->a, sizeof(laddr->a));
> > -             memset(info + sizeof(laddr->a), 0, addrlen - sizeof(laddr->a));
> > +             size_t copy_len = laddr->a.sa.sa_family == AF_INET ?
> > +                               sizeof(struct sockaddr_in) :
> > +                               sizeof(struct sockaddr_in6);
>
> [Severity: Low]
> [Finding: main-1-1]
> [Sources: opus-5]
> This isn't a bug, but the AF_INET/AF_INET6 to length mapping already
> exists as struct sctp_af::sockaddr_len, and the exact "zero a
> sockaddr_storage, then copy only the valid part" idiom is already in the
> tree:
>
> net/sctp/ulpevent.c:sctp_ulpevent_notify_peer_addr_change() {
>         memset(&addr, 0, sizeof(struct sockaddr_storage));
>         memcpy(&addr, &transport->ipaddr,
>                transport->af_specific->sockaddr_len);
>         ...
> }
>
> Would sctp_get_af_specific(laddr->a.sa.sa_family)->sockaddr_len in
> inet_diag_msg_sctpladdrs_fill(), and from->af_specific->sockaddr_len in
> inet_diag_msg_sctpaddrs_fill(), work here?
>
> Also, the else branch hardcodes sizeof(struct sockaddr_in6) as a stand-in
> for "the whole union", so an unexpected sa_family copies the maximum
> length rather than the minimum, and the constant silently diverges if
> union sctp_addr ever grows a larger member.
>
> > +
> > +             memset(info, 0, addrlen);
> > +             memcpy(info, &laddr->a, copy_len);
> >               info += addrlen;
> >
> >               if (!--addrcnt)
>
> [Severity: Low]
> [Finding: main-1-0]
> [Sources: opus-5]
> Does this leave the uninitialized tail sitting in the bind address list
> itself?  The residue is stored by the producers, and this hunk only stops
> one consumer from exporting it:
>
> net/sctp/socket.c:sctp_send_asconf_add_ip() {
>         memcpy(&saveaddr, addr, af->sockaddr_len);
>         retval = sctp_add_bind_addr(bp, &saveaddr,
>                                     sizeof(saveaddr),
>                                     SCTP_ADDR_NEW, GFP_ATOMIC);
>         ...
> }
>
> net/sctp/bind_addr.c:sctp_raw_to_bind_addrs() {
>         union sctp_addr addr;
>         ...
>         !af->from_addr_param(&addr, rawaddr, htons(port), 0)
>         ...
>         retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
>                                     SCTP_ADDR_SRC, gfp);
> }
>
> In both cases only 16 bytes of the 28-byte stack union are written for
> AF_INET, and sctp_add_bind_addr() then persists all 28 bytes into the
> kzalloc'ed entry, so offsets 16..27 of the stored address stay as stack
> residue after this patch.  The second path is driven by peer-supplied
> INIT/INIT-ACK address parameters.
>
> Would zeroing the union in sctp_v4_from_skb() / sctp_v4_from_sk() /
> sctp_v4_from_addr_param(), or clamping q() to
> af->sockaddr_len, close this for every present and future consumer?  That
> seems worth considering given the Cc: stable tag.
>

Instead of only copying the valid address length when dumping entries from
address_list, the comment suggests that the unused bytes should be
initialized when the address is added to the list.

For transport_addr_list, the path:

  sctp_assoc_add_peer() -> sctp_transport_new() -> sctp_transport_init()

already guarantees this:

  transport = kzalloc_obj(*transport, gfp);
  ...
  memcpy(&peer->ipaddr, addr, peer->af_specific->sockaddr_len);

However, for bp->address_list, sctp_add_bind_addr() relies on the new_size
parameter to copy the correct address length. There are currently three
callers passing sizeof(addr) instead of af->sockaddr_len:

- sctp_raw_to_bind_addrs()
- sctp_unpack_cookie()
- sctp_send_asconf_add_ip()

These should be changed to pass af->sockaddr_len to sctp_add_bind_addr(),
as shown below:

diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 31737f144c7f..1b9bfac17816 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -294,7 +294,7 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr
*bp, __u8 *raw_addr_list,

                if (sctp_bind_addr_state(bp, &addr) != -1)
                        goto next;
-               retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+               retval = sctp_add_bind_addr(bp, &addr, af->sockaddr_len,
                                            SCTP_ADDR_SRC, gfp);
                if (retval)
                        /* Can't finish building the list, clean up. */
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index b472565fc7f2..8da6d97b6062 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1861,8 +1861,11 @@ struct sctp_association *sctp_unpack_cookie(

        /* Also, add the destination address. */
        if (list_empty(&retval->base.bind_addr.address_list)) {
+               struct sctp_af *af;
+
+               af = sctp_get_af_specific(chunk->dest.sa.sa_family);
                sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-                                  sizeof(chunk->dest), SCTP_ADDR_SRC,
+                                  af->sockaddr_len, SCTP_ADDR_SRC,
                                   GFP_ATOMIC);
        }

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index e4ea57a642c1..1f934a9b2e5a 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -634,7 +634,7 @@ static int sctp_send_asconf_add_ip(struct sock
         *sk,
                        af = sctp_get_af_specific(addr->v4.sin_family);
                        memcpy(&saveaddr, addr, af->sockaddr_len);
                        retval = sctp_add_bind_addr(bp, &saveaddr,
-                                                   sizeof(saveaddr),
+                                                   af->sockaddr_len,
                                                    SCTP_ADDR_NEW, GFP_ATOMIC);
                        addr_buf += af->sockaddr_len;
                }

Note that the same issue does NOT exist in the following functions:

- sctp_copy_local_addr_list()
- sctp_copy_one_addr()
- sctp_bind_addr_dup()

Although they use sizeof(addr), the addresses being copied are already
entries from address_list, where the unused bytes have been properly
initialized.


Hi, MingXuan, can you give it a try to fix it in the proposed way above?

Thanks.