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

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

[MingXuan]

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.

The IPv4 address-filling helpers sctp_v4_from_addr_param() and
sctp_v4_from_skb() only initialize the sockaddr_in portion (16 bytes) of the
union sctp_addr; the trailing 12 bytes (offset 16..27, the sockaddr_in6-only
region) are left uninitialized.  Those bytes are propagated verbatim through
sctp_add_bind_addr() (which copies sizeof(union sctp_addr)=28 bytes) and then
copied straight to userspace by the diag fill functions, leaking 12 bytes of
kernel stack residue per local/peer address to any process that can issue a
SOCK_DIAG_BY_FAMILY dump for IPPROTO_SCTP.

Fix it by computing the actually-initialized length of the address from its
sa_family (struct sockaddr_in for AF_INET, the whole union otherwise) and
copying only that many bytes into an already-zeroed sockaddr_storage slot, so
the uninitialized tail is never read and never reaches userspace.

Fixes: 8f840e47f190 ("sctp: add the sctp_diag.c file")
Cc: stable@vger.kernel.org
Signed-off-by: MingXuan <omeux327@gmail.com>
Acked-by: Xin Long <lucien.xin@gmail.com>
---
Changes in v2 (per review feedback on v1):
  - trim the Fixes: commit hash to the recommended abbreviated form (12 chars)
  - rename the introduced local from addr_len to copy_len so it does not
    collide with the existing addrlen (slot stride) in the same scope

No semantic change relative to v1; Xin Long's Acked-by is carried forward.

 net/sctp/diag.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/sctp/diag.c b/net/sctp/diag.c
index d758f5c3e06e..12557e924cc2 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);
+
+		memset(info, 0, addrlen);
+		memcpy(info, &laddr->a, copy_len);
 		info += addrlen;
 
 		if (!--addrcnt)
@@ -113,9 +117,12 @@ static int inet_diag_msg_sctpaddrs_fill(struct sk_buff *skb,
 	info = nla_data(attr);
 	list_for_each_entry(from, &asoc->peer.transport_addr_list,
 			    transports) {
-		memcpy(info, &from->ipaddr, sizeof(from->ipaddr));
-		memset(info + sizeof(from->ipaddr), 0,
-		       addrlen - sizeof(from->ipaddr));
+		size_t copy_len = from->ipaddr.sa.sa_family == AF_INET ?
+				  sizeof(struct sockaddr_in) :
+				  sizeof(struct sockaddr_in6);
+
+		memset(info, 0, addrlen);
+		memcpy(info, &from->ipaddr, copy_len);
 		info += addrlen;
 	}
 
-- 
2.50.1 (Apple Git-155)

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

[Jakub Kicinski]

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 sctp_add_bind_addr() to
af->sockaddr_len, close this for every present and future consumer?  That
seems worth considering given the Cc: stable tag.

> @@ -114,9 +118,12 @@ static int inet_diag_msg_sctpaddrs_fill(struct sk_buff *skb,
>  	info = nla_data(attr);
>  	list_for_each_entry(from, &asoc->peer.transport_addr_list,
>  			    transports) {
> -		memcpy(info, &from->ipaddr, sizeof(from->ipaddr));
> -		memset(info + sizeof(from->ipaddr), 0,
> -		       addrlen - sizeof(from->ipaddr));
> +		size_t copy_len = from->ipaddr.sa.sa_family == AF_INET ?
> +				  sizeof(struct sockaddr_in) :
> +				  sizeof(struct sockaddr_in6);
> +
> +		memset(info, 0, addrlen);
> +		memcpy(info, &from->ipaddr, copy_len);
>  		info += addrlen;
>  	}
>
-- 
pw-bot: cr

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

[Xin Long]

On Mon, Aug 3, 2026 at 7:37 PM Jakub Kicinski <kuba@kernel.org> 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.