netdev - Re: [PATCH net] sctp: fully initialize v4 addr in some functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191209115324.GA24499@hmswarspite.think-freely.org>
Date:   Mon, 9 Dec 2019 06:53:24 -0500
From:   Neil Horman <nhorman@...driver.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem@...emloft.net,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net] sctp: fully initialize v4 addr in some functions

On Mon, Dec 09, 2019 at 01:45:54PM +0800, Xin Long wrote:
> Syzbot found a crash:
> 
>   BUG: KMSAN: uninit-value in crc32_body lib/crc32.c:112 [inline]
>   BUG: KMSAN: uninit-value in crc32_le_generic lib/crc32.c:179 [inline]
>   BUG: KMSAN: uninit-value in __crc32c_le_base+0x4fa/0xd30 lib/crc32.c:202
>   Call Trace:
>     crc32_body lib/crc32.c:112 [inline]
>     crc32_le_generic lib/crc32.c:179 [inline]
>     __crc32c_le_base+0x4fa/0xd30 lib/crc32.c:202
>     chksum_update+0xb2/0x110 crypto/crc32c_generic.c:90
>     crypto_shash_update+0x4c5/0x530 crypto/shash.c:107
>     crc32c+0x150/0x220 lib/libcrc32c.c:47
>     sctp_csum_update+0x89/0xa0 include/net/sctp/checksum.h:36
>     __skb_checksum+0x1297/0x12a0 net/core/skbuff.c:2640
>     sctp_compute_cksum include/net/sctp/checksum.h:59 [inline]
>     sctp_packet_pack net/sctp/output.c:528 [inline]
>     sctp_packet_transmit+0x40fb/0x4250 net/sctp/output.c:597
>     sctp_outq_flush_transports net/sctp/outqueue.c:1146 [inline]
>     sctp_outq_flush+0x1823/0x5d80 net/sctp/outqueue.c:1194
>     sctp_outq_uncork+0xd0/0xf0 net/sctp/outqueue.c:757
>     sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1781 [inline]
>     sctp_side_effects net/sctp/sm_sideeffect.c:1184 [inline]
>     sctp_do_sm+0x8fe1/0x9720 net/sctp/sm_sideeffect.c:1155
>     sctp_primitive_REQUESTHEARTBEAT+0x175/0x1a0 net/sctp/primitive.c:185
>     sctp_apply_peer_addr_params+0x212/0x1d40 net/sctp/socket.c:2433
>     sctp_setsockopt_peer_addr_params net/sctp/socket.c:2686 [inline]
>     sctp_setsockopt+0x189bb/0x19090 net/sctp/socket.c:4672
> 
> The issue was caused by transport->ipaddr set with uninit addr param, which
> was passed by:
> 
>   sctp_transport_init net/sctp/transport.c:47 [inline]
>   sctp_transport_new+0x248/0xa00 net/sctp/transport.c:100
>   sctp_assoc_add_peer+0x5ba/0x2030 net/sctp/associola.c:611
>   sctp_process_param net/sctp/sm_make_chunk.c:2524 [inline]
> 
> where 'addr' is set by sctp_v4_from_addr_param(), and it doesn't initialize
> the padding of addr->v4.
> 
> Later when calling sctp_make_heartbeat(), hbinfo.daddr(=transport->ipaddr)
> will become the part of skb, and the issue occurs.
> 
> This patch is to fix it by initializing the padding of addr->v4 in
> sctp_v4_from_addr_param(), as well as other functions that do the similar
> thing, and these functions shouldn't trust that the caller initializes the
> memory, as Marcelo suggested.
> 
> Reported-by: syzbot+6dcbfea81cd3d4dd0b02@...kaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
>  net/sctp/protocol.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index fbbf191..78af2fc 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -227,6 +227,7 @@ static void sctp_v4_from_skb(union sctp_addr *addr, struct sk_buff *skb,
>  		sa->sin_port = sh->dest;
>  		sa->sin_addr.s_addr = ip_hdr(skb)->daddr;
>  	}
> +	memset(sa->sin_zero, 0, sizeof(sa->sin_zero));
>  }
>  
>  /* Initialize an sctp_addr from a socket. */
> @@ -235,6 +236,7 @@ static void sctp_v4_from_sk(union sctp_addr *addr, struct sock *sk)
>  	addr->v4.sin_family = AF_INET;
>  	addr->v4.sin_port = 0;
>  	addr->v4.sin_addr.s_addr = inet_sk(sk)->inet_rcv_saddr;
> +	memset(addr->v4.sin_zero, 0, sizeof(addr->v4.sin_zero));
>  }
>  
>  /* Initialize sk->sk_rcv_saddr from sctp_addr. */
> @@ -257,6 +259,7 @@ static void sctp_v4_from_addr_param(union sctp_addr *addr,
>  	addr->v4.sin_family = AF_INET;
>  	addr->v4.sin_port = port;
>  	addr->v4.sin_addr.s_addr = param->v4.addr.s_addr;
> +	memset(addr->v4.sin_zero, 0, sizeof(addr->v4.sin_zero));
>  }
>  
>  /* Initialize an address parameter from a sctp_addr and return the length
> @@ -281,6 +284,7 @@ static void sctp_v4_dst_saddr(union sctp_addr *saddr, struct flowi4 *fl4,
>  	saddr->v4.sin_family = AF_INET;
>  	saddr->v4.sin_port = port;
>  	saddr->v4.sin_addr.s_addr = fl4->saddr;
> +	memset(saddr->v4.sin_zero, 0, sizeof(saddr->v4.sin_zero));
>  }
>  
>  /* Compare two addresses exactly. */
> @@ -303,6 +307,7 @@ static void sctp_v4_inaddr_any(union sctp_addr *addr, __be16 port)
>  	addr->v4.sin_family = AF_INET;
>  	addr->v4.sin_addr.s_addr = htonl(INADDR_ANY);
>  	addr->v4.sin_port = port;
> +	memset(addr->v4.sin_zero, 0, sizeof(addr->v4.sin_zero));
>  }
>  
>  /* Is this a wildcard address? */
> -- 
> 2.1.0
> 
> 
Acked-by: Neil Horman <nhorman@...driver.com>