diff mbox

[v2] sctp: fully initialize the IPv6 address in sctp_v6_to_addr()

Message ID 20170814184304.82747-1-glider@google.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Alexander Potapenko Aug. 14, 2017, 6:43 p.m. UTC
KMSAN reported use of uninitialized sctp_addr->v4.sin_addr.s_addr and
sctp_addr->v6.sin6_scope_id in sctp_v6_cmp_addr() (see below).
Make sure all fields of an IPv6 address are initialized, which
guarantees that the IPv4 fields are also initialized.

Comments

Marcelo Ricardo Leitner Aug. 14, 2017, 11:07 p.m. UTC | #1
On Mon, Aug 14, 2017 at 08:43:04PM +0200, Alexander Potapenko wrote:
> KMSAN reported use of uninitialized sctp_addr->v4.sin_addr.s_addr and
> sctp_addr->v6.sin6_scope_id in sctp_v6_cmp_addr() (see below).
> Make sure all fields of an IPv6 address are initialized, which
> guarantees that the IPv4 fields are also initialized.
> 
> ==================================================================
>  BUG: KMSAN: use of uninitialized memory in sctp_v6_cmp_addr+0x8d4/0x9f0
>  net/sctp/ipv6.c:517
>  CPU: 2 PID: 31056 Comm: syz-executor1 Not tainted 4.11.0-rc5+ #2944
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
>  01/01/2011
>  Call Trace:
>   dump_stack+0x172/0x1c0 lib/dump_stack.c:42
>   is_logbuf_locked mm/kmsan/kmsan.c:59 [inline]
>   kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:938
>   native_save_fl arch/x86/include/asm/irqflags.h:18 [inline]
>   arch_local_save_flags arch/x86/include/asm/irqflags.h:72 [inline]
>   arch_local_irq_save arch/x86/include/asm/irqflags.h:113 [inline]
>   __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:467
>   sctp_v6_cmp_addr+0x8d4/0x9f0 net/sctp/ipv6.c:517
>   sctp_v6_get_dst+0x8c7/0x1630 net/sctp/ipv6.c:290
>   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
>   sctp_assoc_add_peer+0x66d/0x16f0 net/sctp/associola.c:651
>   sctp_sendmsg+0x35a5/0x4f90 net/sctp/socket.c:1871
>   inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762
>   sock_sendmsg_nosec net/socket.c:633 [inline]
>   sock_sendmsg net/socket.c:643 [inline]
>   SYSC_sendto+0x608/0x710 net/socket.c:1696
>   SyS_sendto+0x8a/0xb0 net/socket.c:1664
>   entry_SYSCALL_64_fastpath+0x13/0x94
>  RIP: 0033:0x44b479
>  RSP: 002b:00007f6213f21c08 EFLAGS: 00000286 ORIG_RAX: 000000000000002c
>  RAX: ffffffffffffffda RBX: 0000000020000000 RCX: 000000000044b479
>  RDX: 0000000000000041 RSI: 0000000020edd000 RDI: 0000000000000006
>  RBP: 00000000007080a8 R08: 0000000020b85fe4 R09: 000000000000001c
>  R10: 0000000000040005 R11: 0000000000000286 R12: 00000000ffffffff
>  R13: 0000000000003760 R14: 00000000006e5820 R15: 0000000000ff8000
>  origin description: ----dst_saddr@sctp_v6_get_dst
>  local variable created at:
>   sk_fullsock include/net/sock.h:2321 [inline]
>   inet6_sk include/linux/ipv6.h:309 [inline]
>   sctp_v6_get_dst+0x91/0x1630 net/sctp/ipv6.c:241
>   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
> ==================================================================
>  BUG: KMSAN: use of uninitialized memory in sctp_v6_cmp_addr+0x8d4/0x9f0
>  net/sctp/ipv6.c:517
>  CPU: 2 PID: 31056 Comm: syz-executor1 Not tainted 4.11.0-rc5+ #2944
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
>  01/01/2011
>  Call Trace:
>   dump_stack+0x172/0x1c0 lib/dump_stack.c:42
>   is_logbuf_locked mm/kmsan/kmsan.c:59 [inline]
>   kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:938
>   native_save_fl arch/x86/include/asm/irqflags.h:18 [inline]
>   arch_local_save_flags arch/x86/include/asm/irqflags.h:72 [inline]
>   arch_local_irq_save arch/x86/include/asm/irqflags.h:113 [inline]
>   __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:467
>   sctp_v6_cmp_addr+0x8d4/0x9f0 net/sctp/ipv6.c:517
>   sctp_v6_get_dst+0x8c7/0x1630 net/sctp/ipv6.c:290
>   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
>   sctp_assoc_add_peer+0x66d/0x16f0 net/sctp/associola.c:651
>   sctp_sendmsg+0x35a5/0x4f90 net/sctp/socket.c:1871
>   inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762
>   sock_sendmsg_nosec net/socket.c:633 [inline]
>   sock_sendmsg net/socket.c:643 [inline]
>   SYSC_sendto+0x608/0x710 net/socket.c:1696
>   SyS_sendto+0x8a/0xb0 net/socket.c:1664
>   entry_SYSCALL_64_fastpath+0x13/0x94
>  RIP: 0033:0x44b479
>  RSP: 002b:00007f6213f21c08 EFLAGS: 00000286 ORIG_RAX: 000000000000002c
>  RAX: ffffffffffffffda RBX: 0000000020000000 RCX: 000000000044b479
>  RDX: 0000000000000041 RSI: 0000000020edd000 RDI: 0000000000000006
>  RBP: 00000000007080a8 R08: 0000000020b85fe4 R09: 000000000000001c
>  R10: 0000000000040005 R11: 0000000000000286 R12: 00000000ffffffff
>  R13: 0000000000003760 R14: 00000000006e5820 R15: 0000000000ff8000
>  origin description: ----dst_saddr@sctp_v6_get_dst
>  local variable created at:
>   sk_fullsock include/net/sock.h:2321 [inline]
>   inet6_sk include/linux/ipv6.h:309 [inline]
>   sctp_v6_get_dst+0x91/0x1630 net/sctp/ipv6.c:241
>   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
> ==================================================================
> 
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Reviewed-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
> v2 is identical to v1, resending per request by Marcelo Ricardo Leitner.
> ---
>  net/sctp/ipv6.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 2a186b201ad2..a15d691829c6 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -513,6 +513,8 @@ static void sctp_v6_to_addr(union sctp_addr *addr, struct in6_addr *saddr,
>  	addr->sa.sa_family = AF_INET6;
>  	addr->v6.sin6_port = port;
>  	addr->v6.sin6_addr = *saddr;
> +	addr->v6.sin6_flowinfo = 0;
> +	addr->v6.sin6_scope_id = 0;
>  }
>  
>  /* Compare addresses exactly.
> -- 
> 2.14.0.434.g98096fd7a8-goog
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Hideaki Yoshifuji Aug. 15, 2017, 1:43 a.m. UTC | #2
Hi,

2017-08-15 3:43 GMT+09:00 Alexander Potapenko <glider@google.com>:
> KMSAN reported use of uninitialized sctp_addr->v4.sin_addr.s_addr and
> sctp_addr->v6.sin6_scope_id in sctp_v6_cmp_addr() (see below).
> Make sure all fields of an IPv6 address are initialized, which
> guarantees that the IPv4 fields are also initialized.
>
> ==================================================================
>  BUG: KMSAN: use of uninitialized memory in sctp_v6_cmp_addr+0x8d4/0x9f0
>  net/sctp/ipv6.c:517
>  CPU: 2 PID: 31056 Comm: syz-executor1 Not tainted 4.11.0-rc5+ #2944
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
>  01/01/2011
>  Call Trace:
>   dump_stack+0x172/0x1c0 lib/dump_stack.c:42
>   is_logbuf_locked mm/kmsan/kmsan.c:59 [inline]
>   kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:938
>   native_save_fl arch/x86/include/asm/irqflags.h:18 [inline]
>   arch_local_save_flags arch/x86/include/asm/irqflags.h:72 [inline]
>   arch_local_irq_save arch/x86/include/asm/irqflags.h:113 [inline]
>   __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:467
>   sctp_v6_cmp_addr+0x8d4/0x9f0 net/sctp/ipv6.c:517
>   sctp_v6_get_dst+0x8c7/0x1630 net/sctp/ipv6.c:290
>   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
>   sctp_assoc_add_peer+0x66d/0x16f0 net/sctp/associola.c:651
>   sctp_sendmsg+0x35a5/0x4f90 net/sctp/socket.c:1871
>   inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762
>   sock_sendmsg_nosec net/socket.c:633 [inline]
>   sock_sendmsg net/socket.c:643 [inline]
>   SYSC_sendto+0x608/0x710 net/socket.c:1696
>   SyS_sendto+0x8a/0xb0 net/socket.c:1664
>   entry_SYSCALL_64_fastpath+0x13/0x94
>  RIP: 0033:0x44b479
>  RSP: 002b:00007f6213f21c08 EFLAGS: 00000286 ORIG_RAX: 000000000000002c
>  RAX: ffffffffffffffda RBX: 0000000020000000 RCX: 000000000044b479
>  RDX: 0000000000000041 RSI: 0000000020edd000 RDI: 0000000000000006
>  RBP: 00000000007080a8 R08: 0000000020b85fe4 R09: 000000000000001c
>  R10: 0000000000040005 R11: 0000000000000286 R12: 00000000ffffffff
>  R13: 0000000000003760 R14: 00000000006e5820 R15: 0000000000ff8000
>  origin description: ----dst_saddr@sctp_v6_get_dst
>  local variable created at:
>   sk_fullsock include/net/sock.h:2321 [inline]
>   inet6_sk include/linux/ipv6.h:309 [inline]
>   sctp_v6_get_dst+0x91/0x1630 net/sctp/ipv6.c:241
>   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
> ==================================================================
>  BUG: KMSAN: use of uninitialized memory in sctp_v6_cmp_addr+0x8d4/0x9f0
>  net/sctp/ipv6.c:517
>  CPU: 2 PID: 31056 Comm: syz-executor1 Not tainted 4.11.0-rc5+ #2944
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
>  01/01/2011
>  Call Trace:
>   dump_stack+0x172/0x1c0 lib/dump_stack.c:42
>   is_logbuf_locked mm/kmsan/kmsan.c:59 [inline]
>   kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:938
>   native_save_fl arch/x86/include/asm/irqflags.h:18 [inline]
>   arch_local_save_flags arch/x86/include/asm/irqflags.h:72 [inline]
>   arch_local_irq_save arch/x86/include/asm/irqflags.h:113 [inline]
>   __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:467
>   sctp_v6_cmp_addr+0x8d4/0x9f0 net/sctp/ipv6.c:517
>   sctp_v6_get_dst+0x8c7/0x1630 net/sctp/ipv6.c:290
>   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
>   sctp_assoc_add_peer+0x66d/0x16f0 net/sctp/associola.c:651
>   sctp_sendmsg+0x35a5/0x4f90 net/sctp/socket.c:1871
>   inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762
>   sock_sendmsg_nosec net/socket.c:633 [inline]
>   sock_sendmsg net/socket.c:643 [inline]
>   SYSC_sendto+0x608/0x710 net/socket.c:1696
>   SyS_sendto+0x8a/0xb0 net/socket.c:1664
>   entry_SYSCALL_64_fastpath+0x13/0x94
>  RIP: 0033:0x44b479
>  RSP: 002b:00007f6213f21c08 EFLAGS: 00000286 ORIG_RAX: 000000000000002c
>  RAX: ffffffffffffffda RBX: 0000000020000000 RCX: 000000000044b479
>  RDX: 0000000000000041 RSI: 0000000020edd000 RDI: 0000000000000006
>  RBP: 00000000007080a8 R08: 0000000020b85fe4 R09: 000000000000001c
>  R10: 0000000000040005 R11: 0000000000000286 R12: 00000000ffffffff
>  R13: 0000000000003760 R14: 00000000006e5820 R15: 0000000000ff8000
>  origin description: ----dst_saddr@sctp_v6_get_dst
>  local variable created at:
>   sk_fullsock include/net/sock.h:2321 [inline]
>   inet6_sk include/linux/ipv6.h:309 [inline]
>   sctp_v6_get_dst+0x91/0x1630 net/sctp/ipv6.c:241
>   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
> ==================================================================
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Reviewed-by: Xin Long <lucien.xin@gmail.com>
> ---
> v2 is identical to v1, resending per request by Marcelo Ricardo Leitner.
> ---
>  net/sctp/ipv6.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 2a186b201ad2..a15d691829c6 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -513,6 +513,8 @@ static void sctp_v6_to_addr(union sctp_addr *addr, struct in6_addr *saddr,
>         addr->sa.sa_family = AF_INET6;
>         addr->v6.sin6_port = port;
>         addr->v6.sin6_addr = *saddr;
> +       addr->v6.sin6_flowinfo = 0;
> +       addr->v6.sin6_scope_id = 0;

Please set flowinfo between port and addr.

--yoshfuji

>  }
>
>  /* Compare addresses exactly.
> --
> 2.14.0.434.g98096fd7a8-goog
>
Marcelo Ricardo Leitner Aug. 15, 2017, 1:58 a.m. UTC | #3
On Tue, Aug 15, 2017 at 10:43:59AM +0900, 吉藤英明 wrote:
> Hi,
> 
> 2017-08-15 3:43 GMT+09:00 Alexander Potapenko <glider@google.com>:
> > KMSAN reported use of uninitialized sctp_addr->v4.sin_addr.s_addr and
> > sctp_addr->v6.sin6_scope_id in sctp_v6_cmp_addr() (see below).
> > Make sure all fields of an IPv6 address are initialized, which
> > guarantees that the IPv4 fields are also initialized.
> >
> > ==================================================================
> >  BUG: KMSAN: use of uninitialized memory in sctp_v6_cmp_addr+0x8d4/0x9f0
> >  net/sctp/ipv6.c:517
> >  CPU: 2 PID: 31056 Comm: syz-executor1 Not tainted 4.11.0-rc5+ #2944
> >  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> >  01/01/2011
> >  Call Trace:
> >   dump_stack+0x172/0x1c0 lib/dump_stack.c:42
> >   is_logbuf_locked mm/kmsan/kmsan.c:59 [inline]
> >   kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:938
> >   native_save_fl arch/x86/include/asm/irqflags.h:18 [inline]
> >   arch_local_save_flags arch/x86/include/asm/irqflags.h:72 [inline]
> >   arch_local_irq_save arch/x86/include/asm/irqflags.h:113 [inline]
> >   __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:467
> >   sctp_v6_cmp_addr+0x8d4/0x9f0 net/sctp/ipv6.c:517
> >   sctp_v6_get_dst+0x8c7/0x1630 net/sctp/ipv6.c:290
> >   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
> >   sctp_assoc_add_peer+0x66d/0x16f0 net/sctp/associola.c:651
> >   sctp_sendmsg+0x35a5/0x4f90 net/sctp/socket.c:1871
> >   inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762
> >   sock_sendmsg_nosec net/socket.c:633 [inline]
> >   sock_sendmsg net/socket.c:643 [inline]
> >   SYSC_sendto+0x608/0x710 net/socket.c:1696
> >   SyS_sendto+0x8a/0xb0 net/socket.c:1664
> >   entry_SYSCALL_64_fastpath+0x13/0x94
> >  RIP: 0033:0x44b479
> >  RSP: 002b:00007f6213f21c08 EFLAGS: 00000286 ORIG_RAX: 000000000000002c
> >  RAX: ffffffffffffffda RBX: 0000000020000000 RCX: 000000000044b479
> >  RDX: 0000000000000041 RSI: 0000000020edd000 RDI: 0000000000000006
> >  RBP: 00000000007080a8 R08: 0000000020b85fe4 R09: 000000000000001c
> >  R10: 0000000000040005 R11: 0000000000000286 R12: 00000000ffffffff
> >  R13: 0000000000003760 R14: 00000000006e5820 R15: 0000000000ff8000
> >  origin description: ----dst_saddr@sctp_v6_get_dst
> >  local variable created at:
> >   sk_fullsock include/net/sock.h:2321 [inline]
> >   inet6_sk include/linux/ipv6.h:309 [inline]
> >   sctp_v6_get_dst+0x91/0x1630 net/sctp/ipv6.c:241
> >   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
> > ==================================================================
> >  BUG: KMSAN: use of uninitialized memory in sctp_v6_cmp_addr+0x8d4/0x9f0
> >  net/sctp/ipv6.c:517
> >  CPU: 2 PID: 31056 Comm: syz-executor1 Not tainted 4.11.0-rc5+ #2944
> >  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> >  01/01/2011
> >  Call Trace:
> >   dump_stack+0x172/0x1c0 lib/dump_stack.c:42
> >   is_logbuf_locked mm/kmsan/kmsan.c:59 [inline]
> >   kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:938
> >   native_save_fl arch/x86/include/asm/irqflags.h:18 [inline]
> >   arch_local_save_flags arch/x86/include/asm/irqflags.h:72 [inline]
> >   arch_local_irq_save arch/x86/include/asm/irqflags.h:113 [inline]
> >   __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:467
> >   sctp_v6_cmp_addr+0x8d4/0x9f0 net/sctp/ipv6.c:517
> >   sctp_v6_get_dst+0x8c7/0x1630 net/sctp/ipv6.c:290
> >   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
> >   sctp_assoc_add_peer+0x66d/0x16f0 net/sctp/associola.c:651
> >   sctp_sendmsg+0x35a5/0x4f90 net/sctp/socket.c:1871
> >   inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762
> >   sock_sendmsg_nosec net/socket.c:633 [inline]
> >   sock_sendmsg net/socket.c:643 [inline]
> >   SYSC_sendto+0x608/0x710 net/socket.c:1696
> >   SyS_sendto+0x8a/0xb0 net/socket.c:1664
> >   entry_SYSCALL_64_fastpath+0x13/0x94
> >  RIP: 0033:0x44b479
> >  RSP: 002b:00007f6213f21c08 EFLAGS: 00000286 ORIG_RAX: 000000000000002c
> >  RAX: ffffffffffffffda RBX: 0000000020000000 RCX: 000000000044b479
> >  RDX: 0000000000000041 RSI: 0000000020edd000 RDI: 0000000000000006
> >  RBP: 00000000007080a8 R08: 0000000020b85fe4 R09: 000000000000001c
> >  R10: 0000000000040005 R11: 0000000000000286 R12: 00000000ffffffff
> >  R13: 0000000000003760 R14: 00000000006e5820 R15: 0000000000ff8000
> >  origin description: ----dst_saddr@sctp_v6_get_dst
> >  local variable created at:
> >   sk_fullsock include/net/sock.h:2321 [inline]
> >   inet6_sk include/linux/ipv6.h:309 [inline]
> >   sctp_v6_get_dst+0x91/0x1630 net/sctp/ipv6.c:241
> >   sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
> > ==================================================================
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Reviewed-by: Xin Long <lucien.xin@gmail.com>
> > ---
> > v2 is identical to v1, resending per request by Marcelo Ricardo Leitner.
> > ---
> >  net/sctp/ipv6.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> > index 2a186b201ad2..a15d691829c6 100644
> > --- a/net/sctp/ipv6.c
> > +++ b/net/sctp/ipv6.c
> > @@ -513,6 +513,8 @@ static void sctp_v6_to_addr(union sctp_addr *addr, struct in6_addr *saddr,
> >         addr->sa.sa_family = AF_INET6;
> >         addr->v6.sin6_port = port;
> >         addr->v6.sin6_addr = *saddr;
> > +       addr->v6.sin6_flowinfo = 0;
> > +       addr->v6.sin6_scope_id = 0;
> 
> Please set flowinfo between port and addr.

Why?

> 
> --yoshfuji
> 
> >  }
> >
> >  /* Compare addresses exactly.
> > --
> > 2.14.0.434.g98096fd7a8-goog
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
David Miller Aug. 15, 2017, 2:40 a.m. UTC | #4
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Mon, 14 Aug 2017 22:58:14 -0300

> On Tue, Aug 15, 2017 at 10:43:59AM +0900, 吉藤英明 wrote:
>> > diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> > index 2a186b201ad2..a15d691829c6 100644
>> > --- a/net/sctp/ipv6.c
>> > +++ b/net/sctp/ipv6.c
>> > @@ -513,6 +513,8 @@ static void sctp_v6_to_addr(union sctp_addr *addr, struct in6_addr *saddr,
>> >         addr->sa.sa_family = AF_INET6;
>> >         addr->v6.sin6_port = port;
>> >         addr->v6.sin6_addr = *saddr;
>> > +       addr->v6.sin6_flowinfo = 0;
>> > +       addr->v6.sin6_scope_id = 0;
>> 
>> Please set flowinfo between port and addr.
> 
> Why?

Store buffer compression.

You want to always initialize structure member in the order
they are in memory.

No, the compiler won't do this automatically.
Marcelo Ricardo Leitner Aug. 15, 2017, 3:05 p.m. UTC | #5
On Mon, Aug 14, 2017 at 07:40:51PM -0700, David Miller wrote:
> From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Date: Mon, 14 Aug 2017 22:58:14 -0300
>
> > On Tue, Aug 15, 2017 at 10:43:59AM +0900, 吉藤英明 wrote:
> >> > diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> >> > index 2a186b201ad2..a15d691829c6 100644
> >> > --- a/net/sctp/ipv6.c
> >> > +++ b/net/sctp/ipv6.c
> >> > @@ -513,6 +513,8 @@ static void sctp_v6_to_addr(union sctp_addr *addr, struct in6_addr *saddr,
> >> >         addr->sa.sa_family = AF_INET6;
> >> >         addr->v6.sin6_port = port;
> >> >         addr->v6.sin6_addr = *saddr;
> >> > +       addr->v6.sin6_flowinfo = 0;
> >> > +       addr->v6.sin6_scope_id = 0;
> >>
> >> Please set flowinfo between port and addr.
> >
> > Why?
>
> Store buffer compression.
>
> You want to always initialize structure member in the order
> they are in memory.

Thanks.

>
> No, the compiler won't do this automatically.

Ok, but I should see a difference in the generated code, right?

union sctp_addr is a union of sockaddr_in structs, for easy reference
ipv6 being:
struct sockaddr_in6 {
        short unsigned int         sin6_family;          /*     0     2 */
        __be16                     sin6_port;            /*     2     2 */
        __be32                     sin6_flowinfo;        /*     4     4 */
        struct in6_addr            sin6_addr;            /*     8    16 */
        __u32                      sin6_scope_id;        /*    24     4 */

Current code:
    12a1:       ba 0a 00 00 00          mov    $0xa,%edx
    12a6:       49 8b 5f 68             mov    0x68(%r15),%rbx
    12aa:       66 89 55 a0             mov    %dx,-0x60(%rbp)
                             union/struct start -----^
    12ae:       4d 8d 4f 68             lea    0x68(%r15),%r9
    12b2:       49 8b 55 40             mov    0x40(%r13),%rdx
    12b6:       66 c1 c0 08             rol    $0x8,%ax        (htons)
    12ba:       4c 39 cb                cmp    %r9,%rbx
    12bd:       48 89 55 b0             mov    %rdx,-0x50(%rbp)
                                     thus this     ---^
    12c1:       66 89 45 a2             mov    %ax,-0x5e(%rbp)
                                                     ^-------- port number
    12c5:       49 8b 45 38             mov    0x38(%r13),%rax
    12c9:       48 89 45 a8             mov    %rax,-0x58(%rbp)
                  and this are the ipv6 addr bytes ---^
    12cd:       74 30                   je     12ff <sctp_v6_get_dst+0x37f>

Alexander's:
    12a1:       ba 0a 00 00 00          mov    $0xa,%edx
    12a6:       49 8b 5f 68             mov    0x68(%r15),%rbx
    12aa:       66 89 55 a0             mov    %dx,-0x60(%rbp)
    12ae:       4d 8d 4f 68             lea    0x68(%r15),%r9
    12b2:       49 8b 55 40             mov    0x40(%r13),%rdx
    12b6:       c7 45 a4 00 00 00 00    movl   $0x0,-0x5c(%rbp)
    12bd:       c7 45 b8 00 00 00 00    movl   $0x0,-0x48(%rbp)
    12c4:       66 c1 c0 08             rol    $0x8,%ax
    12c8:       4c 39 cb                cmp    %r9,%rbx
    12cb:       48 89 55 b0             mov    %rdx,-0x50(%rbp)
    12cf:       66 89 45 a2             mov    %ax,-0x5e(%rbp)
    12d3:       49 8b 45 38             mov    0x38(%r13),%rax
    12d7:       48 89 45 a8             mov    %rax,-0x58(%rbp)
    12db:       74 30                   je     130d <sctp_v6_get_dst+0x38d>

Hideaki's:
    12a1:       ba 0a 00 00 00          mov    $0xa,%edx
    12a6:       49 8b 5f 68             mov    0x68(%r15),%rbx
    12aa:       66 89 55 a0             mov    %dx,-0x60(%rbp)
    12ae:       4d 8d 4f 68             lea    0x68(%r15),%r9
    12b2:       49 8b 55 40             mov    0x40(%r13),%rdx
    12b6:       c7 45 a4 00 00 00 00    movl   $0x0,-0x5c(%rbp)
    12bd:       c7 45 b8 00 00 00 00    movl   $0x0,-0x48(%rbp)
    12c4:       66 c1 c0 08             rol    $0x8,%ax
    12c8:       4c 39 cb                cmp    %r9,%rbx
    12cb:       48 89 55 b0             mov    %rdx,-0x50(%rbp)
    12cf:       66 89 45 a2             mov    %ax,-0x5e(%rbp)
    12d3:       49 8b 45 38             mov    0x38(%r13),%rax
    12d7:       48 89 45 a8             mov    %rax,-0x58(%rbp)
    12db:       74 30                   je     130d <sctp_v6_get_dst+0x38d>

They are the same, and messed up by the compiler breaking the copy of
the ipv6 addr (which was copied backwards) with the port number copy.

  Marcelo
Eric Dumazet Aug. 15, 2017, 3:37 p.m. UTC | #6
On Tue, 2017-08-15 at 12:05 -0300, Marcelo Ricardo Leitner wrote:

> Ok, but I should see a difference in the generated code, right?

Depends on the compiler. Have you tried older versions ?

One argument is that following struct member definition eases code
review.

(It is easier to catch a field init is missing)
Marcelo Ricardo Leitner Aug. 15, 2017, 4:31 p.m. UTC | #7
On Tue, Aug 15, 2017 at 08:37:49AM -0700, Eric Dumazet wrote:
> On Tue, 2017-08-15 at 12:05 -0300, Marcelo Ricardo Leitner wrote:
> 
> > Ok, but I should see a difference in the generated code, right?
> 
> Depends on the compiler. Have you tried older versions ?
> 

This was with gcc 6.4.1, fc25 standard. Only tested with it and didn't
check clang either.

> One argument is that following struct member definition eases code
> review.
> 
> (It is easier to catch a field init is missing)

And a good one, yes.

Thanks,
Marcelo
diff mbox

Patch

==================================================================
 BUG: KMSAN: use of uninitialized memory in sctp_v6_cmp_addr+0x8d4/0x9f0
 net/sctp/ipv6.c:517
 CPU: 2 PID: 31056 Comm: syz-executor1 Not tainted 4.11.0-rc5+ #2944
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
 01/01/2011
 Call Trace:
  dump_stack+0x172/0x1c0 lib/dump_stack.c:42
  is_logbuf_locked mm/kmsan/kmsan.c:59 [inline]
  kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:938
  native_save_fl arch/x86/include/asm/irqflags.h:18 [inline]
  arch_local_save_flags arch/x86/include/asm/irqflags.h:72 [inline]
  arch_local_irq_save arch/x86/include/asm/irqflags.h:113 [inline]
  __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:467
  sctp_v6_cmp_addr+0x8d4/0x9f0 net/sctp/ipv6.c:517
  sctp_v6_get_dst+0x8c7/0x1630 net/sctp/ipv6.c:290
  sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
  sctp_assoc_add_peer+0x66d/0x16f0 net/sctp/associola.c:651
  sctp_sendmsg+0x35a5/0x4f90 net/sctp/socket.c:1871
  inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762
  sock_sendmsg_nosec net/socket.c:633 [inline]
  sock_sendmsg net/socket.c:643 [inline]
  SYSC_sendto+0x608/0x710 net/socket.c:1696
  SyS_sendto+0x8a/0xb0 net/socket.c:1664
  entry_SYSCALL_64_fastpath+0x13/0x94
 RIP: 0033:0x44b479
 RSP: 002b:00007f6213f21c08 EFLAGS: 00000286 ORIG_RAX: 000000000000002c
 RAX: ffffffffffffffda RBX: 0000000020000000 RCX: 000000000044b479
 RDX: 0000000000000041 RSI: 0000000020edd000 RDI: 0000000000000006
 RBP: 00000000007080a8 R08: 0000000020b85fe4 R09: 000000000000001c
 R10: 0000000000040005 R11: 0000000000000286 R12: 00000000ffffffff
 R13: 0000000000003760 R14: 00000000006e5820 R15: 0000000000ff8000
 origin description: ----dst_saddr@sctp_v6_get_dst
 local variable created at:
  sk_fullsock include/net/sock.h:2321 [inline]
  inet6_sk include/linux/ipv6.h:309 [inline]
  sctp_v6_get_dst+0x91/0x1630 net/sctp/ipv6.c:241
  sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
==================================================================
 BUG: KMSAN: use of uninitialized memory in sctp_v6_cmp_addr+0x8d4/0x9f0
 net/sctp/ipv6.c:517
 CPU: 2 PID: 31056 Comm: syz-executor1 Not tainted 4.11.0-rc5+ #2944
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
 01/01/2011
 Call Trace:
  dump_stack+0x172/0x1c0 lib/dump_stack.c:42
  is_logbuf_locked mm/kmsan/kmsan.c:59 [inline]
  kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:938
  native_save_fl arch/x86/include/asm/irqflags.h:18 [inline]
  arch_local_save_flags arch/x86/include/asm/irqflags.h:72 [inline]
  arch_local_irq_save arch/x86/include/asm/irqflags.h:113 [inline]
  __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:467
  sctp_v6_cmp_addr+0x8d4/0x9f0 net/sctp/ipv6.c:517
  sctp_v6_get_dst+0x8c7/0x1630 net/sctp/ipv6.c:290
  sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
  sctp_assoc_add_peer+0x66d/0x16f0 net/sctp/associola.c:651
  sctp_sendmsg+0x35a5/0x4f90 net/sctp/socket.c:1871
  inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762
  sock_sendmsg_nosec net/socket.c:633 [inline]
  sock_sendmsg net/socket.c:643 [inline]
  SYSC_sendto+0x608/0x710 net/socket.c:1696
  SyS_sendto+0x8a/0xb0 net/socket.c:1664
  entry_SYSCALL_64_fastpath+0x13/0x94
 RIP: 0033:0x44b479
 RSP: 002b:00007f6213f21c08 EFLAGS: 00000286 ORIG_RAX: 000000000000002c
 RAX: ffffffffffffffda RBX: 0000000020000000 RCX: 000000000044b479
 RDX: 0000000000000041 RSI: 0000000020edd000 RDI: 0000000000000006
 RBP: 00000000007080a8 R08: 0000000020b85fe4 R09: 000000000000001c
 R10: 0000000000040005 R11: 0000000000000286 R12: 00000000ffffffff
 R13: 0000000000003760 R14: 00000000006e5820 R15: 0000000000ff8000
 origin description: ----dst_saddr@sctp_v6_get_dst
 local variable created at:
  sk_fullsock include/net/sock.h:2321 [inline]
  inet6_sk include/linux/ipv6.h:309 [inline]
  sctp_v6_get_dst+0x91/0x1630 net/sctp/ipv6.c:241
  sctp_transport_route+0x101/0x570 net/sctp/transport.c:292
==================================================================

Signed-off-by: Alexander Potapenko <glider@google.com>
Reviewed-by: Xin Long <lucien.xin@gmail.com>
---
v2 is identical to v1, resending per request by Marcelo Ricardo Leitner.
---
 net/sctp/ipv6.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 2a186b201ad2..a15d691829c6 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -513,6 +513,8 @@  static void sctp_v6_to_addr(union sctp_addr *addr, struct in6_addr *saddr,
 	addr->sa.sa_family = AF_INET6;
 	addr->v6.sin6_port = port;
 	addr->v6.sin6_addr = *saddr;
+	addr->v6.sin6_flowinfo = 0;
+	addr->v6.sin6_scope_id = 0;
 }
 
 /* Compare addresses exactly.