| Message ID | 20170713181034.41123-1-glider@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jul 13, 2017 at 8:10 PM, Alexander Potapenko <glider@google.com> wrote: > If the iterator (|pos.p| or |err|) has already reached the end of > chunk, we shouldn't access iterator->length. > > This bug has been detected by KMSAN. For the following pair of system > calls: > > socket(PF_INET6, SOCK_STREAM, 0x84 /* IPPROTO_??? */) = 3 > sendto(3, "A", 1, MSG_OOB, {sa_family=AF_INET6, sin6_port=htons(0), > inet_pton(AF_INET6, "::1", &sin6_addr), sin6_flowinfo=0, > sin6_scope_id=0}, 28) = 1 > > the tool has reported a use of uninitialized memory: > > ================================================================== > BUG: KMSAN: use of uninitialized memory in sctp_rcv+0x17b8/0x43b0 > CPU: 1 PID: 2940 Comm: probe Not tainted 4.11.0-rc5+ #2926 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs > 01/01/2011 > Call Trace: > <IRQ> > __dump_stack lib/dump_stack.c:16 > dump_stack+0x172/0x1c0 lib/dump_stack.c:52 > kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:927 > __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:469 > __sctp_rcv_init_lookup net/sctp/input.c:1074 > __sctp_rcv_lookup_harder net/sctp/input.c:1233 > __sctp_rcv_lookup net/sctp/input.c:1255 > sctp_rcv+0x17b8/0x43b0 net/sctp/input.c:170 > sctp6_rcv+0x32/0x70 net/sctp/ipv6.c:984 > ip6_input_finish+0x82f/0x1ee0 net/ipv6/ip6_input.c:279 > NF_HOOK ./include/linux/netfilter.h:257 > ip6_input+0x239/0x290 net/ipv6/ip6_input.c:322 > dst_input ./include/net/dst.h:492 > ip6_rcv_finish net/ipv6/ip6_input.c:69 > NF_HOOK ./include/linux/netfilter.h:257 > ipv6_rcv+0x1dbd/0x22e0 net/ipv6/ip6_input.c:203 > __netif_receive_skb_core+0x2f6f/0x3a20 net/core/dev.c:4208 > __netif_receive_skb net/core/dev.c:4246 > process_backlog+0x667/0xba0 net/core/dev.c:4866 > napi_poll net/core/dev.c:5268 > net_rx_action+0xc95/0x1590 net/core/dev.c:5333 > __do_softirq+0x485/0x942 kernel/softirq.c:284 > do_softirq_own_stack+0x1c/0x30 arch/x86/entry/entry_64.S:902 > </IRQ> > do_softirq kernel/softirq.c:328 > __local_bh_enable_ip+0x25b/0x290 kernel/softirq.c:181 > local_bh_enable+0x37/0x40 ./include/linux/bottom_half.h:31 > rcu_read_unlock_bh ./include/linux/rcupdate.h:931 > ip6_finish_output2+0x19b2/0x1cf0 net/ipv6/ip6_output.c:124 > ip6_finish_output+0x764/0x970 net/ipv6/ip6_output.c:149 > NF_HOOK_COND ./include/linux/netfilter.h:246 > ip6_output+0x456/0x520 net/ipv6/ip6_output.c:163 > dst_output ./include/net/dst.h:486 > NF_HOOK ./include/linux/netfilter.h:257 > ip6_xmit+0x1841/0x1c00 net/ipv6/ip6_output.c:261 > sctp_v6_xmit+0x3b7/0x470 net/sctp/ipv6.c:225 > sctp_packet_transmit+0x38cb/0x3a20 net/sctp/output.c:632 > sctp_outq_flush+0xeb3/0x46e0 net/sctp/outqueue.c:885 > sctp_outq_uncork+0xb2/0xd0 net/sctp/outqueue.c:750 > sctp_side_effects net/sctp/sm_sideeffect.c:1773 > sctp_do_sm+0x6962/0x6ec0 net/sctp/sm_sideeffect.c:1147 > sctp_primitive_ASSOCIATE+0x12c/0x160 net/sctp/primitive.c:88 > sctp_sendmsg+0x43e5/0x4f90 net/sctp/socket.c:1954 > inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762 > sock_sendmsg_nosec net/socket.c:633 > sock_sendmsg net/socket.c:643 > SYSC_sendto+0x608/0x710 net/socket.c:1696 > SyS_sendto+0x8a/0xb0 net/socket.c:1664 > do_syscall_64+0xe6/0x130 arch/x86/entry/common.c:285 > entry_SYSCALL64_slow_path+0x25/0x25 arch/x86/entry/entry_64.S:246 > RIP: 0033:0x401133 > RSP: 002b:00007fff6d99cd38 EFLAGS: 00000246 ORIG_RAX: 000000000000002c > RAX: ffffffffffffffda RBX: 00000000004002b0 RCX: 0000000000401133 > RDX: 0000000000000001 RSI: 0000000000494088 RDI: 0000000000000003 > RBP: 00007fff6d99cd90 R08: 00007fff6d99cd50 R09: 000000000000001c > R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000000 > R13: 00000000004063d0 R14: 0000000000406460 R15: 0000000000000000 > origin: > save_stack_trace+0x37/0x40 arch/x86/kernel/stacktrace.c:59 > kmsan_save_stack_with_flags mm/kmsan/kmsan.c:302 > kmsan_internal_poison_shadow+0xb1/0x1a0 mm/kmsan/kmsan.c:198 > kmsan_poison_shadow+0x6d/0xc0 mm/kmsan/kmsan.c:211 > slab_alloc_node mm/slub.c:2743 > __kmalloc_node_track_caller+0x200/0x360 mm/slub.c:4351 > __kmalloc_reserve net/core/skbuff.c:138 > __alloc_skb+0x26b/0x840 net/core/skbuff.c:231 > alloc_skb ./include/linux/skbuff.h:933 > sctp_packet_transmit+0x31e/0x3a20 net/sctp/output.c:570 > sctp_outq_flush+0xeb3/0x46e0 net/sctp/outqueue.c:885 > sctp_outq_uncork+0xb2/0xd0 net/sctp/outqueue.c:750 > sctp_side_effects net/sctp/sm_sideeffect.c:1773 > sctp_do_sm+0x6962/0x6ec0 net/sctp/sm_sideeffect.c:1147 > sctp_primitive_ASSOCIATE+0x12c/0x160 net/sctp/primitive.c:88 > sctp_sendmsg+0x43e5/0x4f90 net/sctp/socket.c:1954 > inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762 > sock_sendmsg_nosec net/socket.c:633 > sock_sendmsg net/socket.c:643 > SYSC_sendto+0x608/0x710 net/socket.c:1696 > SyS_sendto+0x8a/0xb0 net/socket.c:1664 > do_syscall_64+0xe6/0x130 arch/x86/entry/common.c:285 > return_from_SYSCALL_64+0x0/0x6a arch/x86/entry/entry_64.S:246 > ================================================================== > > Signed-off-by: Alexander Potapenko <glider@google.com> > --- > include/net/sctp/sctp.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h > index a9519a06a23b..f13632ee33f0 100644 > --- a/include/net/sctp/sctp.h > +++ b/include/net/sctp/sctp.h > @@ -469,6 +469,7 @@ _sctp_walk_params((pos), (chunk), ntohs((chunk)->chunk_hdr.length), member) > > #define _sctp_walk_params(pos, chunk, end, member)\ > for (pos.v = chunk->member;\ > + pos.v < (void *)chunk + end &&\ checkpatch.pl warns about spaces here, but I wanted to be consistent with the rest of the file. > pos.v <= (void *)chunk + end - ntohs(pos.p->length) &&\ > ntohs(pos.p->length) >= sizeof(struct sctp_paramhdr);\ > pos.v += SCTP_PAD4(ntohs(pos.p->length))) > @@ -479,6 +480,7 @@ _sctp_walk_errors((err), (chunk_hdr), ntohs((chunk_hdr)->length)) > #define _sctp_walk_errors(err, chunk_hdr, end)\ > for (err = (sctp_errhdr_t *)((void *)chunk_hdr + \ > sizeof(struct sctp_chunkhdr));\ > + (void *)err <= (void *)chunk_hdr + end &&\ > (void *)err <= (void *)chunk_hdr + end - ntohs(err->length) &&\ > ntohs(err->length) >= sizeof(sctp_errhdr_t); \ > err = (sctp_errhdr_t *)((void *)err + SCTP_PAD4(ntohs(err->length)))) > -- > 2.13.2.932.g7449e964c-goog >
On Thu, Jul 13, 2017 at 8:14 PM, Alexander Potapenko <glider@google.com> wrote: > On Thu, Jul 13, 2017 at 8:10 PM, Alexander Potapenko <glider@google.com> wrote: >> If the iterator (|pos.p| or |err|) has already reached the end of >> chunk, we shouldn't access iterator->length. >> >> This bug has been detected by KMSAN. For the following pair of system >> calls: >> >> socket(PF_INET6, SOCK_STREAM, 0x84 /* IPPROTO_??? */) = 3 >> sendto(3, "A", 1, MSG_OOB, {sa_family=AF_INET6, sin6_port=htons(0), >> inet_pton(AF_INET6, "::1", &sin6_addr), sin6_flowinfo=0, >> sin6_scope_id=0}, 28) = 1 >> >> the tool has reported a use of uninitialized memory: >> >> ================================================================== >> BUG: KMSAN: use of uninitialized memory in sctp_rcv+0x17b8/0x43b0 >> CPU: 1 PID: 2940 Comm: probe Not tainted 4.11.0-rc5+ #2926 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs >> 01/01/2011 >> Call Trace: >> <IRQ> >> __dump_stack lib/dump_stack.c:16 >> dump_stack+0x172/0x1c0 lib/dump_stack.c:52 >> kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:927 >> __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:469 >> __sctp_rcv_init_lookup net/sctp/input.c:1074 >> __sctp_rcv_lookup_harder net/sctp/input.c:1233 >> __sctp_rcv_lookup net/sctp/input.c:1255 >> sctp_rcv+0x17b8/0x43b0 net/sctp/input.c:170 >> sctp6_rcv+0x32/0x70 net/sctp/ipv6.c:984 >> ip6_input_finish+0x82f/0x1ee0 net/ipv6/ip6_input.c:279 >> NF_HOOK ./include/linux/netfilter.h:257 >> ip6_input+0x239/0x290 net/ipv6/ip6_input.c:322 >> dst_input ./include/net/dst.h:492 >> ip6_rcv_finish net/ipv6/ip6_input.c:69 >> NF_HOOK ./include/linux/netfilter.h:257 >> ipv6_rcv+0x1dbd/0x22e0 net/ipv6/ip6_input.c:203 >> __netif_receive_skb_core+0x2f6f/0x3a20 net/core/dev.c:4208 >> __netif_receive_skb net/core/dev.c:4246 >> process_backlog+0x667/0xba0 net/core/dev.c:4866 >> napi_poll net/core/dev.c:5268 >> net_rx_action+0xc95/0x1590 net/core/dev.c:5333 >> __do_softirq+0x485/0x942 kernel/softirq.c:284 >> do_softirq_own_stack+0x1c/0x30 arch/x86/entry/entry_64.S:902 >> </IRQ> >> do_softirq kernel/softirq.c:328 >> __local_bh_enable_ip+0x25b/0x290 kernel/softirq.c:181 >> local_bh_enable+0x37/0x40 ./include/linux/bottom_half.h:31 >> rcu_read_unlock_bh ./include/linux/rcupdate.h:931 >> ip6_finish_output2+0x19b2/0x1cf0 net/ipv6/ip6_output.c:124 >> ip6_finish_output+0x764/0x970 net/ipv6/ip6_output.c:149 >> NF_HOOK_COND ./include/linux/netfilter.h:246 >> ip6_output+0x456/0x520 net/ipv6/ip6_output.c:163 >> dst_output ./include/net/dst.h:486 >> NF_HOOK ./include/linux/netfilter.h:257 >> ip6_xmit+0x1841/0x1c00 net/ipv6/ip6_output.c:261 >> sctp_v6_xmit+0x3b7/0x470 net/sctp/ipv6.c:225 >> sctp_packet_transmit+0x38cb/0x3a20 net/sctp/output.c:632 >> sctp_outq_flush+0xeb3/0x46e0 net/sctp/outqueue.c:885 >> sctp_outq_uncork+0xb2/0xd0 net/sctp/outqueue.c:750 >> sctp_side_effects net/sctp/sm_sideeffect.c:1773 >> sctp_do_sm+0x6962/0x6ec0 net/sctp/sm_sideeffect.c:1147 >> sctp_primitive_ASSOCIATE+0x12c/0x160 net/sctp/primitive.c:88 >> sctp_sendmsg+0x43e5/0x4f90 net/sctp/socket.c:1954 >> inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762 >> sock_sendmsg_nosec net/socket.c:633 >> sock_sendmsg net/socket.c:643 >> SYSC_sendto+0x608/0x710 net/socket.c:1696 >> SyS_sendto+0x8a/0xb0 net/socket.c:1664 >> do_syscall_64+0xe6/0x130 arch/x86/entry/common.c:285 >> entry_SYSCALL64_slow_path+0x25/0x25 arch/x86/entry/entry_64.S:246 >> RIP: 0033:0x401133 >> RSP: 002b:00007fff6d99cd38 EFLAGS: 00000246 ORIG_RAX: 000000000000002c >> RAX: ffffffffffffffda RBX: 00000000004002b0 RCX: 0000000000401133 >> RDX: 0000000000000001 RSI: 0000000000494088 RDI: 0000000000000003 >> RBP: 00007fff6d99cd90 R08: 00007fff6d99cd50 R09: 000000000000001c >> R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000000 >> R13: 00000000004063d0 R14: 0000000000406460 R15: 0000000000000000 >> origin: >> save_stack_trace+0x37/0x40 arch/x86/kernel/stacktrace.c:59 >> kmsan_save_stack_with_flags mm/kmsan/kmsan.c:302 >> kmsan_internal_poison_shadow+0xb1/0x1a0 mm/kmsan/kmsan.c:198 >> kmsan_poison_shadow+0x6d/0xc0 mm/kmsan/kmsan.c:211 >> slab_alloc_node mm/slub.c:2743 >> __kmalloc_node_track_caller+0x200/0x360 mm/slub.c:4351 >> __kmalloc_reserve net/core/skbuff.c:138 >> __alloc_skb+0x26b/0x840 net/core/skbuff.c:231 >> alloc_skb ./include/linux/skbuff.h:933 >> sctp_packet_transmit+0x31e/0x3a20 net/sctp/output.c:570 >> sctp_outq_flush+0xeb3/0x46e0 net/sctp/outqueue.c:885 >> sctp_outq_uncork+0xb2/0xd0 net/sctp/outqueue.c:750 >> sctp_side_effects net/sctp/sm_sideeffect.c:1773 >> sctp_do_sm+0x6962/0x6ec0 net/sctp/sm_sideeffect.c:1147 >> sctp_primitive_ASSOCIATE+0x12c/0x160 net/sctp/primitive.c:88 >> sctp_sendmsg+0x43e5/0x4f90 net/sctp/socket.c:1954 >> inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762 >> sock_sendmsg_nosec net/socket.c:633 >> sock_sendmsg net/socket.c:643 >> SYSC_sendto+0x608/0x710 net/socket.c:1696 >> SyS_sendto+0x8a/0xb0 net/socket.c:1664 >> do_syscall_64+0xe6/0x130 arch/x86/entry/common.c:285 >> return_from_SYSCALL_64+0x0/0x6a arch/x86/entry/entry_64.S:246 >> ================================================================== >> >> Signed-off-by: Alexander Potapenko <glider@google.com> >> --- >> include/net/sctp/sctp.h | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h >> index a9519a06a23b..f13632ee33f0 100644 >> --- a/include/net/sctp/sctp.h >> +++ b/include/net/sctp/sctp.h >> @@ -469,6 +469,7 @@ _sctp_walk_params((pos), (chunk), ntohs((chunk)->chunk_hdr.length), member) >> >> #define _sctp_walk_params(pos, chunk, end, member)\ >> for (pos.v = chunk->member;\ >> + pos.v < (void *)chunk + end &&\ > checkpatch.pl warns about spaces here, but I wanted to be consistent > with the rest of the file. >> pos.v <= (void *)chunk + end - ntohs(pos.p->length) &&\ >> ntohs(pos.p->length) >= sizeof(struct sctp_paramhdr);\ >> pos.v += SCTP_PAD4(ntohs(pos.p->length))) >> @@ -479,6 +480,7 @@ _sctp_walk_errors((err), (chunk_hdr), ntohs((chunk_hdr)->length)) >> #define _sctp_walk_errors(err, chunk_hdr, end)\ >> for (err = (sctp_errhdr_t *)((void *)chunk_hdr + \ >> sizeof(struct sctp_chunkhdr));\ >> + (void *)err <= (void *)chunk_hdr + end &&\ Should be "<", obviously. >> (void *)err <= (void *)chunk_hdr + end - ntohs(err->length) &&\ >> ntohs(err->length) >= sizeof(sctp_errhdr_t); \ >> err = (sctp_errhdr_t *)((void *)err + SCTP_PAD4(ntohs(err->length)))) >> -- >> 2.13.2.932.g7449e964c-goog >> > > > > -- > Alexander Potapenko > Software Engineer > > Google Germany GmbH > Erika-Mann-Straße, 33 > 80636 München > > Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg
From: Alexander Potapenko <glider@google.com> Date: Thu, 13 Jul 2017 20:10:34 +0200 > diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h > index a9519a06a23b..f13632ee33f0 100644 > --- a/include/net/sctp/sctp.h > +++ b/include/net/sctp/sctp.h > @@ -469,6 +469,7 @@ _sctp_walk_params((pos), (chunk), ntohs((chunk)->chunk_hdr.length), member) > > #define _sctp_walk_params(pos, chunk, end, member)\ > for (pos.v = chunk->member;\ > + pos.v < (void *)chunk + end &&\ > pos.v <= (void *)chunk + end - ntohs(pos.p->length) &&\ > ntohs(pos.p->length) >= sizeof(struct sctp_paramhdr);\ > pos.v += SCTP_PAD4(ntohs(pos.p->length))) > @@ -479,6 +480,7 @@ _sctp_walk_errors((err), (chunk_hdr), ntohs((chunk_hdr)->length)) > #define _sctp_walk_errors(err, chunk_hdr, end)\ > for (err = (sctp_errhdr_t *)((void *)chunk_hdr + \ > sizeof(struct sctp_chunkhdr));\ > + (void *)err <= (void *)chunk_hdr + end &&\ > (void *)err <= (void *)chunk_hdr + end - ntohs(err->length) &&\ > ntohs(err->length) >= sizeof(sctp_errhdr_t); \ > err = (sctp_errhdr_t *)((void *)err + SCTP_PAD4(ntohs(err->length)))) Even with the "err < ..." fixed in the second hunk, I still think you need to tweak these checks some more. What is necessary is that the first two members of sctp_paramhdr or sctp_errhdr are in the range ptr to end. struct sctp_paramhdr { __be16 type; __be16 length; }; typedef struct sctp_errhdr { __be16 cause; __be16 length; __u8 variable[0]; } sctp_errhdr_t; so that we can legally dereference ->length. But that is not what your new check is doing. Your new check is only making sure there is at least one byte there. I guess it's not easy to write a clean test that doesn't hardcode the offset of the length field and it's size. Something like: pos.v + offsetof(pos.v, length) + sizeof(pos.v->length) <= (void *) chunk + end and (void *)err + offsetof(err, length) + sizeof(err->length) <= (void *) chunk_hdr + end And yeah, that isn't exactly concise nor pretty...
On Thu, Jul 13, 2017 at 8:32 PM, David Miller <davem@davemloft.net> wrote: > From: Alexander Potapenko <glider@google.com> > Date: Thu, 13 Jul 2017 20:10:34 +0200 > >> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h >> index a9519a06a23b..f13632ee33f0 100644 >> --- a/include/net/sctp/sctp.h >> +++ b/include/net/sctp/sctp.h >> @@ -469,6 +469,7 @@ _sctp_walk_params((pos), (chunk), ntohs((chunk)->chunk_hdr.length), member) >> >> #define _sctp_walk_params(pos, chunk, end, member)\ >> for (pos.v = chunk->member;\ >> + pos.v < (void *)chunk + end &&\ >> pos.v <= (void *)chunk + end - ntohs(pos.p->length) &&\ >> ntohs(pos.p->length) >= sizeof(struct sctp_paramhdr);\ >> pos.v += SCTP_PAD4(ntohs(pos.p->length))) >> @@ -479,6 +480,7 @@ _sctp_walk_errors((err), (chunk_hdr), ntohs((chunk_hdr)->length)) >> #define _sctp_walk_errors(err, chunk_hdr, end)\ >> for (err = (sctp_errhdr_t *)((void *)chunk_hdr + \ >> sizeof(struct sctp_chunkhdr));\ >> + (void *)err <= (void *)chunk_hdr + end &&\ >> (void *)err <= (void *)chunk_hdr + end - ntohs(err->length) &&\ >> ntohs(err->length) >= sizeof(sctp_errhdr_t); \ >> err = (sctp_errhdr_t *)((void *)err + SCTP_PAD4(ntohs(err->length)))) > > Even with the "err < ..." fixed in the second hunk, I still think you need > to tweak these checks some more. > > What is necessary is that the first two members of sctp_paramhdr or > sctp_errhdr are in the range ptr to end. > > struct sctp_paramhdr { > __be16 type; > __be16 length; > }; > > typedef struct sctp_errhdr { > __be16 cause; > __be16 length; > __u8 variable[0]; > } sctp_errhdr_t; > > so that we can legally dereference ->length. > > But that is not what your new check is doing. Your new check is only > making sure there is at least one byte there. > > I guess it's not easy to write a clean test that doesn't hardcode the > offset of the length field and it's size. > > Something like: > > pos.v + offsetof(pos.v, length) + sizeof(pos.v->length) <= (void *) chunk + end Do we need to bother about truncated structures? Shouldn't it be enough to check that there's at least sizeof(struct sctp_paramhdr) bytes left then? > and > > (void *)err + offsetof(err, length) + sizeof(err->length) <= (void *) chunk_hdr + end > > And yeah, that isn't exactly concise nor pretty...
From: Alexander Potapenko <glider@google.com> Date: Thu, 13 Jul 2017 21:28:39 +0200 > On Thu, Jul 13, 2017 at 8:32 PM, David Miller <davem@davemloft.net> wrote: >> struct sctp_paramhdr { >> __be16 type; >> __be16 length; >> }; >> >> typedef struct sctp_errhdr { >> __be16 cause; >> __be16 length; >> __u8 variable[0]; >> } sctp_errhdr_t; ... >> Something like: >> >> pos.v + offsetof(pos.v, length) + sizeof(pos.v->length) <= (void *) chunk + end > > Do we need to bother about truncated structures? Shouldn't it be > enough to check that there's at least sizeof(struct sctp_paramhdr) > bytes left then? With the zero length array at the end, it's arguable what the "size" of such a thing is. That's why I tried to be explicit with the length field.
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h index a9519a06a23b..f13632ee33f0 100644 --- a/include/net/sctp/sctp.h +++ b/include/net/sctp/sctp.h @@ -469,6 +469,7 @@ _sctp_walk_params((pos), (chunk), ntohs((chunk)->chunk_hdr.length), member) #define _sctp_walk_params(pos, chunk, end, member)\ for (pos.v = chunk->member;\ + pos.v < (void *)chunk + end &&\ pos.v <= (void *)chunk + end - ntohs(pos.p->length) &&\ ntohs(pos.p->length) >= sizeof(struct sctp_paramhdr);\ pos.v += SCTP_PAD4(ntohs(pos.p->length))) @@ -479,6 +480,7 @@ _sctp_walk_errors((err), (chunk_hdr), ntohs((chunk_hdr)->length)) #define _sctp_walk_errors(err, chunk_hdr, end)\ for (err = (sctp_errhdr_t *)((void *)chunk_hdr + \ sizeof(struct sctp_chunkhdr));\ + (void *)err <= (void *)chunk_hdr + end &&\ (void *)err <= (void *)chunk_hdr + end - ntohs(err->length) &&\ ntohs(err->length) >= sizeof(sctp_errhdr_t); \ err = (sctp_errhdr_t *)((void *)err + SCTP_PAD4(ntohs(err->length))))
If the iterator (|pos.p| or |err|) has already reached the end of chunk, we shouldn't access iterator->length. This bug has been detected by KMSAN. For the following pair of system calls: socket(PF_INET6, SOCK_STREAM, 0x84 /* IPPROTO_??? */) = 3 sendto(3, "A", 1, MSG_OOB, {sa_family=AF_INET6, sin6_port=htons(0), inet_pton(AF_INET6, "::1", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, 28) = 1 the tool has reported a use of uninitialized memory: ================================================================== BUG: KMSAN: use of uninitialized memory in sctp_rcv+0x17b8/0x43b0 CPU: 1 PID: 2940 Comm: probe Not tainted 4.11.0-rc5+ #2926 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 Call Trace: <IRQ> __dump_stack lib/dump_stack.c:16 dump_stack+0x172/0x1c0 lib/dump_stack.c:52 kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:927 __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:469 __sctp_rcv_init_lookup net/sctp/input.c:1074 __sctp_rcv_lookup_harder net/sctp/input.c:1233 __sctp_rcv_lookup net/sctp/input.c:1255 sctp_rcv+0x17b8/0x43b0 net/sctp/input.c:170 sctp6_rcv+0x32/0x70 net/sctp/ipv6.c:984 ip6_input_finish+0x82f/0x1ee0 net/ipv6/ip6_input.c:279 NF_HOOK ./include/linux/netfilter.h:257 ip6_input+0x239/0x290 net/ipv6/ip6_input.c:322 dst_input ./include/net/dst.h:492 ip6_rcv_finish net/ipv6/ip6_input.c:69 NF_HOOK ./include/linux/netfilter.h:257 ipv6_rcv+0x1dbd/0x22e0 net/ipv6/ip6_input.c:203 __netif_receive_skb_core+0x2f6f/0x3a20 net/core/dev.c:4208 __netif_receive_skb net/core/dev.c:4246 process_backlog+0x667/0xba0 net/core/dev.c:4866 napi_poll net/core/dev.c:5268 net_rx_action+0xc95/0x1590 net/core/dev.c:5333 __do_softirq+0x485/0x942 kernel/softirq.c:284 do_softirq_own_stack+0x1c/0x30 arch/x86/entry/entry_64.S:902 </IRQ> do_softirq kernel/softirq.c:328 __local_bh_enable_ip+0x25b/0x290 kernel/softirq.c:181 local_bh_enable+0x37/0x40 ./include/linux/bottom_half.h:31 rcu_read_unlock_bh ./include/linux/rcupdate.h:931 ip6_finish_output2+0x19b2/0x1cf0 net/ipv6/ip6_output.c:124 ip6_finish_output+0x764/0x970 net/ipv6/ip6_output.c:149 NF_HOOK_COND ./include/linux/netfilter.h:246 ip6_output+0x456/0x520 net/ipv6/ip6_output.c:163 dst_output ./include/net/dst.h:486 NF_HOOK ./include/linux/netfilter.h:257 ip6_xmit+0x1841/0x1c00 net/ipv6/ip6_output.c:261 sctp_v6_xmit+0x3b7/0x470 net/sctp/ipv6.c:225 sctp_packet_transmit+0x38cb/0x3a20 net/sctp/output.c:632 sctp_outq_flush+0xeb3/0x46e0 net/sctp/outqueue.c:885 sctp_outq_uncork+0xb2/0xd0 net/sctp/outqueue.c:750 sctp_side_effects net/sctp/sm_sideeffect.c:1773 sctp_do_sm+0x6962/0x6ec0 net/sctp/sm_sideeffect.c:1147 sctp_primitive_ASSOCIATE+0x12c/0x160 net/sctp/primitive.c:88 sctp_sendmsg+0x43e5/0x4f90 net/sctp/socket.c:1954 inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762 sock_sendmsg_nosec net/socket.c:633 sock_sendmsg net/socket.c:643 SYSC_sendto+0x608/0x710 net/socket.c:1696 SyS_sendto+0x8a/0xb0 net/socket.c:1664 do_syscall_64+0xe6/0x130 arch/x86/entry/common.c:285 entry_SYSCALL64_slow_path+0x25/0x25 arch/x86/entry/entry_64.S:246 RIP: 0033:0x401133 RSP: 002b:00007fff6d99cd38 EFLAGS: 00000246 ORIG_RAX: 000000000000002c RAX: ffffffffffffffda RBX: 00000000004002b0 RCX: 0000000000401133 RDX: 0000000000000001 RSI: 0000000000494088 RDI: 0000000000000003 RBP: 00007fff6d99cd90 R08: 00007fff6d99cd50 R09: 000000000000001c R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000000 R13: 00000000004063d0 R14: 0000000000406460 R15: 0000000000000000 origin: save_stack_trace+0x37/0x40 arch/x86/kernel/stacktrace.c:59 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:302 kmsan_internal_poison_shadow+0xb1/0x1a0 mm/kmsan/kmsan.c:198 kmsan_poison_shadow+0x6d/0xc0 mm/kmsan/kmsan.c:211 slab_alloc_node mm/slub.c:2743 __kmalloc_node_track_caller+0x200/0x360 mm/slub.c:4351 __kmalloc_reserve net/core/skbuff.c:138 __alloc_skb+0x26b/0x840 net/core/skbuff.c:231 alloc_skb ./include/linux/skbuff.h:933 sctp_packet_transmit+0x31e/0x3a20 net/sctp/output.c:570 sctp_outq_flush+0xeb3/0x46e0 net/sctp/outqueue.c:885 sctp_outq_uncork+0xb2/0xd0 net/sctp/outqueue.c:750 sctp_side_effects net/sctp/sm_sideeffect.c:1773 sctp_do_sm+0x6962/0x6ec0 net/sctp/sm_sideeffect.c:1147 sctp_primitive_ASSOCIATE+0x12c/0x160 net/sctp/primitive.c:88 sctp_sendmsg+0x43e5/0x4f90 net/sctp/socket.c:1954 inet_sendmsg+0x498/0x670 net/ipv4/af_inet.c:762 sock_sendmsg_nosec net/socket.c:633 sock_sendmsg net/socket.c:643 SYSC_sendto+0x608/0x710 net/socket.c:1696 SyS_sendto+0x8a/0xb0 net/socket.c:1664 do_syscall_64+0xe6/0x130 arch/x86/entry/common.c:285 return_from_SYSCALL_64+0x0/0x6a arch/x86/entry/entry_64.S:246 ================================================================== Signed-off-by: Alexander Potapenko <glider@google.com> --- include/net/sctp/sctp.h | 2 ++ 1 file changed, 2 insertions(+)