* [PATCH v4] net: don't call strlen on non-terminated string in dev_set_alias()
@ 2017-06-06 13:56 Alexander Potapenko
2017-06-06 20:36 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Potapenko @ 2017-06-06 13:56 UTC (permalink / raw)
To: dvyukov, kcc, edumazet, davem, stephen; +Cc: linux-kernel, netdev
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>
---
v4: dropped the comment at all, as suggested by Yury Norov
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, 2 insertions(+), 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index fca407b4a6ea..84e1e86a4bce 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1253,8 +1253,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
if (!new_ifalias)
return -ENOMEM;
dev->ifalias = new_ifalias;
+ memcpy(dev->ifalias, alias, len);
+ dev->ifalias[len] = 0;
- strlcpy(dev->ifalias, alias, len+1);
return len;
}
--
2.13.0.506.g27d5fe0cd-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4] net: don't call strlen on non-terminated string in dev_set_alias()
2017-06-06 13:56 [PATCH v4] net: don't call strlen on non-terminated string in dev_set_alias() Alexander Potapenko
@ 2017-06-06 20:36 ` David Miller
2017-06-06 21:15 ` Alexander Potapenko
2017-06-06 21:57 ` Florian Westphal
0 siblings, 2 replies; 5+ messages in thread
From: David Miller @ 2017-06-06 20:36 UTC (permalink / raw)
To: glider; +Cc: dvyukov, kcc, edumazet, stephen, linux-kernel, netdev
From: Alexander Potapenko <glider@google.com>
Date: Tue, 6 Jun 2017 15:56:54 +0200
> 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>
We should not be allowing non-NULL terminated strings for the
IFLA_IFALIAS attribute. It's defined as type NLA_STRING in
the ifla_policy[] array.
Please figure out why we aren't enforcing the attribute policy
properly, rather than adding a workaround.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] net: don't call strlen on non-terminated string in dev_set_alias()
2017-06-06 20:36 ` David Miller
@ 2017-06-06 21:15 ` Alexander Potapenko
2017-06-06 21:57 ` Florian Westphal
1 sibling, 0 replies; 5+ messages in thread
From: Alexander Potapenko @ 2017-06-06 21:15 UTC (permalink / raw)
To: David Miller
Cc: Dmitriy Vyukov, Kostya Serebryany, Eric Dumazet, stephen, LKML,
Networking
On Tue, Jun 6, 2017 at 10:36 PM, David Miller <davem@davemloft.net> wrote:
> From: Alexander Potapenko <glider@google.com>
> Date: Tue, 6 Jun 2017 15:56:54 +0200
>
>> 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>
>
> We should not be allowing non-NULL terminated strings for the
> IFLA_IFALIAS attribute. It's defined as type NLA_STRING in
> the ifla_policy[] array.
Sorry, I couldn't determine from RFC 2233 whether ifAlias is
zero-terminated or not, but looking at validate_nla() I see that
NLA_STRING is supposed to be such.
I'll check what's going on.
> Please figure out why we aren't enforcing the attribute policy
> properly, rather than adding a workaround.
Guess the string has been previously claimed to be non-terminated
here: https://patchwork.ozlabs.org/patch/996/
> Thanks.
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] net: don't call strlen on non-terminated string in dev_set_alias()
2017-06-06 20:36 ` David Miller
2017-06-06 21:15 ` Alexander Potapenko
@ 2017-06-06 21:57 ` Florian Westphal
2017-06-07 1:04 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2017-06-06 21:57 UTC (permalink / raw)
To: David Miller
Cc: glider, dvyukov, kcc, edumazet, stephen, linux-kernel, netdev
David Miller <davem@davemloft.net> wrote:
> From: Alexander Potapenko <glider@google.com>
> Date: Tue, 6 Jun 2017 15:56:54 +0200
>
> > 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>
>
> We should not be allowing non-NULL terminated strings for the
> IFLA_IFALIAS attribute. It's defined as type NLA_STRING in
> the ifla_policy[] array.
Unfortunately NLA_STRING doesn't check for NUL byte, only
NLA_NUL_STRING does this.
So unless you think we can change kernel and make NLA_STRING
behave like NLA_NUL_STRING I think patch is correct.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] net: don't call strlen on non-terminated string in dev_set_alias()
2017-06-06 21:57 ` Florian Westphal
@ 2017-06-07 1:04 ` David Miller
0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2017-06-07 1:04 UTC (permalink / raw)
To: fw; +Cc: glider, dvyukov, kcc, edumazet, stephen, linux-kernel, netdev
From: Florian Westphal <fw@strlen.de>
Date: Tue, 6 Jun 2017 23:57:35 +0200
> David Miller <davem@davemloft.net> wrote:
>> From: Alexander Potapenko <glider@google.com>
>> Date: Tue, 6 Jun 2017 15:56:54 +0200
>>
>> > 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>
>>
>> We should not be allowing non-NULL terminated strings for the
>> IFLA_IFALIAS attribute. It's defined as type NLA_STRING in
>> the ifla_policy[] array.
>
> Unfortunately NLA_STRING doesn't check for NUL byte, only
> NLA_NUL_STRING does this.
>
> So unless you think we can change kernel and make NLA_STRING
> behave like NLA_NUL_STRING I think patch is correct.
Ok, I missed that, thanks for the clarification.
I'll apply this and queue it up for -stable, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-07 1:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-06 13:56 [PATCH v4] net: don't call strlen on non-terminated string in dev_set_alias() Alexander Potapenko
2017-06-06 20:36 ` David Miller
2017-06-06 21:15 ` Alexander Potapenko
2017-06-06 21:57 ` Florian Westphal
2017-06-07 1:04 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).