Message ID | 20170601123829.51794-1-glider@google.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote: > KMSAN reported a use of uninitialized memory in dev_set_alias(), > which was caused by calling strlcpy() (which in turn called strlen()) > on the user-supplied non-terminated string. > > Signed-off-by: Alexander Potapenko <glider@google.com> > --- > v3: removed the multi-line comment > v2: fixed an off-by-one error spotted by Dmitry Vyukov [...] > --- > net/core/dev.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index fca407b4a6ea..3e3b29133cc9 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len) > return -ENOMEM; > dev->ifalias = new_ifalias; > > - strlcpy(dev->ifalias, alias, len+1); > + /* alias comes from the userspace and may not be zero-terminated. */ So if the comment is correct, you'd use copy_from_user() instead. > + memcpy(dev->ifalias, alias, len); > + dev->ifalias[len] = 0; > return len; > } > > -- > 2.13.0.219.gdb65acc882-goog
On Thu, Jun 1, 2017 at 3:47 PM, Yury Norov <ynorov@caviumnetworks.com> wrote: > On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote: >> KMSAN reported a use of uninitialized memory in dev_set_alias(), >> which was caused by calling strlcpy() (which in turn called strlen()) >> on the user-supplied non-terminated string. >> >> Signed-off-by: Alexander Potapenko <glider@google.com> >> --- >> v3: removed the multi-line comment >> v2: fixed an off-by-one error spotted by Dmitry Vyukov > > [...] > >> --- >> net/core/dev.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index fca407b4a6ea..3e3b29133cc9 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len) >> return -ENOMEM; >> dev->ifalias = new_ifalias; >> >> - strlcpy(dev->ifalias, alias, len+1); >> + /* alias comes from the userspace and may not be zero-terminated. */ > > So if the comment is correct, you'd use copy_from_user() instead. Well, the contents of |alias| have been previously copied from the userspace, but this is a pointer to a kernel buffer, as the function prototype tells. Do you think a confusion is possible here? >> + memcpy(dev->ifalias, alias, len); >> + dev->ifalias[len] = 0; >> return len; >> } >> >> -- >> 2.13.0.219.gdb65acc882-goog
On Thu, Jun 01, 2017 at 03:50:33PM +0200, Alexander Potapenko wrote: > On Thu, Jun 1, 2017 at 3:47 PM, Yury Norov <ynorov@caviumnetworks.com> wrote: > > On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote: > >> KMSAN reported a use of uninitialized memory in dev_set_alias(), > >> which was caused by calling strlcpy() (which in turn called strlen()) > >> on the user-supplied non-terminated string. > >> > >> Signed-off-by: Alexander Potapenko <glider@google.com> > >> --- > >> v3: removed the multi-line comment > >> v2: fixed an off-by-one error spotted by Dmitry Vyukov > > > > [...] > > > >> --- > >> net/core/dev.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/net/core/dev.c b/net/core/dev.c > >> index fca407b4a6ea..3e3b29133cc9 100644 > >> --- a/net/core/dev.c > >> +++ b/net/core/dev.c > >> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len) > >> return -ENOMEM; > >> dev->ifalias = new_ifalias; > >> > >> - strlcpy(dev->ifalias, alias, len+1); > >> + /* alias comes from the userspace and may not be zero-terminated. */ > > > > So if the comment is correct, you'd use copy_from_user() instead. > Well, the contents of |alias| have been previously copied from the > userspace, but this is a pointer to a kernel buffer, as the function > prototype tells. > Do you think a confusion is possible here? Yes, I think so... If pointer comes from userspace, it normally points to userspace data. If you have the data already copied, just say: + /* alias may not be zero-terminated. */ Or say nothing, because at the first glance, using the strlcpy() instead of simple memcpy() looks weird in this case - you know the length of the string from the beginning, and should not recalculate it again. Yury > >> + memcpy(dev->ifalias, alias, len); > >> + dev->ifalias[len] = 0; > >> return len; > >> } > >> > >> -- > >> 2.13.0.219.gdb65acc882-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
On Thu, Jun 1, 2017 at 4:04 PM, Yury Norov <ynorov@caviumnetworks.com> wrote: > On Thu, Jun 01, 2017 at 03:50:33PM +0200, Alexander Potapenko wrote: >> On Thu, Jun 1, 2017 at 3:47 PM, Yury Norov <ynorov@caviumnetworks.com> wrote: >> > On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote: >> >> KMSAN reported a use of uninitialized memory in dev_set_alias(), >> >> which was caused by calling strlcpy() (which in turn called strlen()) >> >> on the user-supplied non-terminated string. >> >> >> >> Signed-off-by: Alexander Potapenko <glider@google.com> >> >> --- >> >> v3: removed the multi-line comment >> >> v2: fixed an off-by-one error spotted by Dmitry Vyukov >> > >> > [...] >> > >> >> --- >> >> net/core/dev.c | 4 +++- >> >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> >> index fca407b4a6ea..3e3b29133cc9 100644 >> >> --- a/net/core/dev.c >> >> +++ b/net/core/dev.c >> >> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len) >> >> return -ENOMEM; >> >> dev->ifalias = new_ifalias; >> >> >> >> - strlcpy(dev->ifalias, alias, len+1); >> >> + /* alias comes from the userspace and may not be zero-terminated. */ >> > >> > So if the comment is correct, you'd use copy_from_user() instead. >> Well, the contents of |alias| have been previously copied from the >> userspace, but this is a pointer to a kernel buffer, as the function >> prototype tells. >> Do you think a confusion is possible here? > > Yes, I think so... If pointer comes from userspace, it normally points to > userspace data. If you have the data already copied, just say: > + /* alias may not be zero-terminated. */ > > Or say nothing, because at the first glance, using the strlcpy() > instead of simple memcpy() looks weird in this case - you know the > length of the string from the beginning, and should not recalculate it > again. You're right, I'll just drop this line. > Yury > >> >> + memcpy(dev->ifalias, alias, len); >> >> + dev->ifalias[len] = 0; >> >> return len; >> >> } >> >> >> >> -- >> >> 2.13.0.219.gdb65acc882-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
================================================================== BUG: KMSAN: use of unitialized memory in strlcpy+0x8b/0x1b0 CPU: 0 PID: 1062 Comm: probe Not tainted 4.11.0-rc5+ #2763 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:16 dump_stack+0x143/0x1b0 lib/dump_stack.c:52 kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:1016 __kmsan_warning_32+0x66/0xb0 mm/kmsan/kmsan_instr.c:491 strlen lib/string.c:484 strlcpy+0x8b/0x1b0 lib/string.c:144 dev_set_alias+0x295/0x360 net/core/dev.c:1255 do_setlink+0xfe5/0x5750 net/core/rtnetlink.c:2007 rtnl_setlink+0x469/0x4f0 net/core/rtnetlink.c:2249 rtnetlink_rcv_msg+0x5da/0xb40 net/core/rtnetlink.c:4107 netlink_rcv_skb+0x339/0x5a0 net/netlink/af_netlink.c:2339 rtnetlink_rcv+0x83/0xa0 net/core/rtnetlink.c:4113 netlink_unicast_kernel net/netlink/af_netlink.c:1272 netlink_unicast+0x13b7/0x1480 net/netlink/af_netlink.c:1298 netlink_sendmsg+0x10b8/0x10f0 net/netlink/af_netlink.c:1844 sock_sendmsg_nosec net/socket.c:633 sock_sendmsg net/socket.c:643 sock_write_iter+0x395/0x440 net/socket.c:857 call_write_iter ./include/linux/fs.h:1733 new_sync_write fs/read_write.c:497 __vfs_write+0x619/0x700 fs/read_write.c:510 vfs_write+0x47e/0x8a0 fs/read_write.c:558 SYSC_write+0x17e/0x2f0 fs/read_write.c:605 SyS_write+0x87/0xb0 fs/read_write.c:597 entry_SYSCALL_64_fastpath+0x13/0x94 arch/x86/entry/entry_64.S:204 RIP: 0033:0x4052f1 RSP: 002b:00007fde1ed05d80 EFLAGS: 00000293 ORIG_RAX: 0000000000000001 RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00000000004052f1 RDX: 0000000000000026 RSI: 00000000006cf0c0 RDI: 0000000000000032 RBP: 00007fde1ed05da0 R08: 00007fde1ed06700 R09: 00007fde1ed06700 R10: 00007fde1ed069d0 R11: 0000000000000293 R12: 0000000000000000 R13: 0000000000000000 R14: 00007fde1ed069c0 R15: 00007fde1ed06700 origin: 00000000aec00057 save_stack_trace+0x59/0x60 arch/x86/kernel/stacktrace.c:59 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:352 kmsan_internal_poison_shadow+0xb1/0x1a0 mm/kmsan/kmsan.c:247 kmsan_poison_shadow+0x6d/0xc0 mm/kmsan/kmsan.c:260 slab_alloc_node mm/slub.c:2743 __kmalloc_node_track_caller+0x1f4/0x390 mm/slub.c:4349 __kmalloc_reserve net/core/skbuff.c:138 __alloc_skb+0x2cd/0x740 net/core/skbuff.c:231 alloc_skb ./include/linux/skbuff.h:933 netlink_alloc_large_skb net/netlink/af_netlink.c:1144 netlink_sendmsg+0x934/0x10f0 net/netlink/af_netlink.c:1819 sock_sendmsg_nosec net/socket.c:633 sock_sendmsg net/socket.c:643 sock_write_iter+0x395/0x440 net/socket.c:857 call_write_iter ./include/linux/fs.h:1733 new_sync_write fs/read_write.c:497 __vfs_write+0x619/0x700 fs/read_write.c:510 vfs_write+0x47e/0x8a0 fs/read_write.c:558 SYSC_write+0x17e/0x2f0 fs/read_write.c:605 SyS_write+0x87/0xb0 fs/read_write.c:597 entry_SYSCALL_64_fastpath+0x13/0x94 arch/x86/entry/entry_64.S:204 ================================================================== and the reproducer: ================================================================== #include <pthread.h> int sock = -1; char buf[] = "\x26\x00\x00\x00\x13\x00\x47\xf1\x07\x01\xc1\xb0" "\x0e\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00" "\x09\xef\x18\xff\xff\x00\xf1\x32\x05\x00\x14\x00" "\x6e\x35"; void do_work(int arg) { switch (arg) { case 0: sock = socket(0x10, 0x3, 0x0); break; case 1: write(sock, buf, 0x26); break; } } void *thread_fn(void *arg) { int actual_arg = (int)arg; int iter = 10000, i; for (i = 0; i < iter; i++) { do_work(actual_arg); usleep(100); } return NULL; } int main() { pthread_t thr[4]; int i; for (i = 0; i < 4; i++) { pthread_create(&thr, NULL, thread_fn, (void*)(i % 2)); } sleep(10); return 0; } ================================================================== --- net/core/dev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/net/core/dev.c b/net/core/dev.c index fca407b4a6ea..3e3b29133cc9 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len) return -ENOMEM; dev->ifalias = new_ifalias; - strlcpy(dev->ifalias, alias, len+1); + /* alias comes from the userspace and may not be zero-terminated. */ + memcpy(dev->ifalias, alias, len); + dev->ifalias[len] = 0; return len; }
KMSAN reported a use of uninitialized memory in dev_set_alias(), which was caused by calling strlcpy() (which in turn called strlen()) on the user-supplied non-terminated string. Signed-off-by: Alexander Potapenko <glider@google.com> --- v3: removed the multi-line comment v2: fixed an off-by-one error spotted by Dmitry Vyukov For the record, here is the KMSAN report: