On Wed, Nov 27, 2019 at 04:49:46PM +0800, Xin Long wrote: > On Wed, Nov 27, 2019 at 2:02 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > Looks like a bug in net/sctp/ where it's passing uninitialized memory into the > > crc32c() function. SCTP maintainers, can you please take a look? > Thanks. > > 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(), which 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. Sweet. > > The fix should be: > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c > index 09050c1d5517..0e73405eba4f 100644 > --- a/net/sctp/sm_make_chunk.c > +++ b/net/sctp/sm_make_chunk.c > @@ -2516,6 +2516,7 @@ static int sctp_process_param(struct > sctp_association *asoc, > if (ipv6_only_sock(asoc->base.sk)) > break; > do_addr_param: > + memset(&addr, 0, sizeof(addr)); > af = sctp_get_af_specific(param_type2af(param.p->type)); > af->from_addr_param(&addr, param.addr, htons(asoc->peer.port), 0); > scope = sctp_scope(peer_addr); > @@ -3040,6 +3041,7 @@ static __be16 sctp_process_asconf_param(struct > sctp_association *asoc, > if (unlikely(!af)) > return SCTP_ERROR_DNS_FAILED; > > + memset(&addr, 0, sizeof(addr)); > af->from_addr_param(&addr, addr_param, htons(asoc->peer.port), 0); In sctp_v4_from_addr_param() itself seems cleaner. (Ditto for sctp_v4_to_addr_param() and related ones, like sctp_v4_dst_saddr()) These functions shouldn't trust that the caller initializes the memory. They are dealing with ipv4 but they know that the buffer they are writting into is larger and the size of it. Marcelo
