net: don't call strlen on non-terminated string in dev_set_alias()

PATCH Superseded Alexander Potapenko 2 participants 5 messages Jun 1, 2017

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Alexander Potapenko

author

2017-06-01 12:38 UTC #0 lore raw

PATCH v3 1/1 net: don't call strlen on non-terminated string in dev_set_alias()

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 <[email protected]>

---
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:

==================================================================

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(-)

net/core/dev.c +3 -1

index fca407b4a6ea..3e3b29133cc9 100644
@@ -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;
 }

Signature · 1 line

2.13.0.219.gdb65acc882-goog

Yury Norov

2017-06-01 13:47 UTC #1 lore raw

Quoted 8 lines · Alexander Potapenko

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 <[email protected]>
---
v3: removed the multi-line comment
v2: fixed an off-by-one error spotted by Dmitry Vyukov

[...]

Quoted 9 lines

---
 net/core/dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

net/core/dev.c +0 -0

index fca407b4a6ea..3e3b29133cc9 100644

net/core/dev.c int dev_set_alias(struct net_device *dev, const char *alias, size_t len) · line 1257

Quoted 5 lines

net/core/dev.c +1 -1

@@ -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.

Quoted 7 lines

net/core/dev.c +2 -0

+	memcpy(dev->ifalias, alias, len);
+	dev->ifalias[len] = 0;
 	return len;
 }

Signature · 1 line

2.13.0.219.gdb65acc882-goog

Alexander Potapenko

author

2017-06-01 13:50 UTC #2 lore raw

Quoted 24 lines · Yury Norov

Quoted 8 lines · Alexander Potapenko
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 <[email protected]>
---
v3: removed the multi-line comment
v2: fixed an off-by-one error spotted by Dmitry Vyukov
[...]
Quoted 12 lines
---
 net/core/dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/core/dev.c +0 -0
index fca407b4a6ea..3e3b29133cc9 100644
@@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
              return -ENOMEM;
      dev->ifalias = new_ifalias;
 

Quoted 4 lines

net/core/dev.c int dev_set_alias(struct net_device *dev, const char *alias, size_t len) · line 1257
Quoted 2 lines
net/core/dev.c +1 -1
-     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?

Quoted 7 lines

net/core/dev.c +2 -1

+     memcpy(dev->ifalias, alias, len);
+     dev->ifalias[len] = 0;
      return len;
 }
 
--

2.13.0.219.gdb65acc882-goog

Signature · 10 lines

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

Yury Norov

2017-06-01 14:04 UTC #3 lore raw

Quoted 25 lines · Alexander Potapenko

Quoted 24 lines · Yury Norov
Quoted 8 lines · Alexander Potapenko
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 <[email protected]>
---
v3: removed the multi-line comment
v2: fixed an off-by-one error spotted by Dmitry Vyukov
[...]
Quoted 12 lines
---
 net/core/dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/core/dev.c +0 -0
index fca407b4a6ea..3e3b29133cc9 100644
@@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
              return -ENOMEM;
      dev->ifalias = new_ifalias;
 

Quoted 8 lines

Quoted 4 lines
net/core/dev.c int dev_set_alias(struct net_device *dev, const char *alias, size_t len) · line 1257
Quoted 2 lines
net/core/dev.c +1 -1
-     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

Quoted 21 lines

Quoted 7 lines

net/core/dev.c +2 -1

+     memcpy(dev->ifalias, alias, len);
+     dev->ifalias[len] = 0;
      return len;
 }
 
--

2.13.0.219.gdb65acc882-goog

Signature · 10 lines

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

Alexander Potapenko

author

2017-06-01 14:06 UTC #4 lore raw

Quoted 35 lines · Yury Norov

Quoted 25 lines · Alexander Potapenko
Quoted 24 lines · Yury Norov
Quoted 8 lines · Alexander Potapenko
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 <[email protected]>
---
v3: removed the multi-line comment
v2: fixed an off-by-one error spotted by Dmitry Vyukov
[...]
Quoted 12 lines
---
 net/core/dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/core/dev.c +0 -0
index fca407b4a6ea..3e3b29133cc9 100644
@@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
              return -ENOMEM;
      dev->ifalias = new_ifalias;
 
Quoted 8 lines
Quoted 4 lines
net/core/dev.c int dev_set_alias(struct net_device *dev, const char *alias, size_t len) · line 1257
Quoted 2 lines
net/core/dev.c +1 -1
-     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?

Quoted 8 lines

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.

Quoted 23 lines

Yury
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
net/core/dev.c +2 -1
+     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

Signature · 10 lines

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