Re: KMSAN: uninit-value in __dev_mc_add
Hello, Eric, all,
> I dunno, your patch looks quite not the right fix.
I agree, it looks more like a dirty hack. Unfortunately, I lack the deep
expertise in the network stack subsystem, so I've posted the patch to,
sort of, start a discussion and probably get some hints.
> If TUN is able to change dev->type, how comes it does not set the
> appropriate dev->addr_len at the same time ?
Well,... probably, nobody cared to do so:
[drivers/net/tun.c]
case TUNSETLINK:
...
tun->dev->type = (int) arg; //<--- that's all!
tun_debug(KERN_INFO, tun, "linktype set to %d\n",
tun->dev->type);
ret = 0;
}
break;
> Really the bug seems to be deeper, and without setting proper
> dev->addr_len, we'll need more 'fixes' like yours.
Absolutely. Unfortunately, I wasn't able to just write such deeper patch.
Let me share what I have found and let me hope to get an advise.
- So setting just the tun->dev->type makes the dev struct inconsistent.
- There are more field to adjust, at least dev->broadcast. Also, there are
a number of *_ops fields which are all set for the Ethernet type, most
probably they must be adjusted also.
- There is no get_addr_len_by_link_type() or a simple way to get link layer
properties by dev->type. Such settings are scattered in *_setup and
*_init functions, like ipgre_tunnel_init() { ... dev->addr_len = 4; ...}
Having these, I can imagine 2 ways for a proper fix.
1) Destroy the net_device in question and recreate it when changing a link
type. This way all the dev fields are set right. Create it in a similar way
as rtnl_newlink() does. Again, we do not have get_X_by_link_type(), so it
probably will be some large switch()/case:
$ grep '^#define ARPHRD_' include/uapi/linux/if_arp.h | wc -l
59
2) Leave tun an Ethernet device, add some tun->pretend_to_be_this_link_type
field and change only it on TUNSETLINK. And use this field in cases for which
TUNSETLINK was invented in the first place. Unfortunately, I do not have such
a list. The initial the commit ff4cc3ac93e1 says:
For use with
wireless and other networking types it should be possible to set the
ARP type via an ioctl.
Surely, there can be something else which I do not see. Could anyone suggest
an advice on this?
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
Re: KMSAN: uninit-value in __dev_mc_add
Hello,
This report is actually for the same bug which was reported in:
https://syzkaller.appspot.com/bug?id=088efeac32fdde781038a777a63e436c0d4d7036
The note there that the bug was fixed by "Commits: net: fix uninit-value in
__hw_addr_add_ex()" is wrong. A C-reproducer from the 2nd syzkaller report
can trigger the bug from this one.
I've researched this and a result is a proposed patch, the problem is the tun
device code allowing to set an arbitrary link type.
https://lkml.org/lkml/2018/9/26/416
https://lore.kernel.org/lkml/20180926093018.6646-1-vdro...@redhat.com/T/#u
https://marc.info/?l=linux-netdev=153795423320016=2
A simplified reproducer is attached.
Best regards,
Vladis Dronov
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
int main(int argc, char **argv)
{
int ret, sockfd, tunfd;
syscall(__NR_mmap, 0x2000, 0x100, 3, 0x32, -1, 0);
// socket(AF_PACKET, SOCK_DGRAM|SOCK_NONBLOCK, 0)
sockfd = syscall(__NR_socket, 0x11, 0x10802, 0);
if (sockfd < 0) {
perror("socket()");
ret = 1;
goto exit_end;
}
memcpy((void*)0x2240, "/dev/net/tun", 13);
tunfd = open((char *)0x2240, 0);
if (tunfd < 0) {
perror("open()");
ret = 2;
goto exit_sock_close;
}
memcpy((void*)0x20c0,
"\x69\x67\x62\x30\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", 16);
*(uint16_t*)0x20d0 = 0x4012;
ret = syscall(__NR_ioctl, tunfd, 0x400454ca, 0x20c0); // TUNSETIFF
_IOW('T', 202, int)
if (ret < 0) {
perror("ioctl(TUNSETIFF)");
ret = 3;
goto exit_tun_close;
}
// TUNSETLINK _IOW('T', 205, int) / 0x30a = 778 = ARPHRD_IPGRE
if (argc < 2)
ret = syscall(__NR_ioctl, tunfd, 0x400454cd, 0x30a);
else
ret = syscall(__NR_ioctl, tunfd, 0x400454cd, atoi(argv[1]));
if (ret < 0) {
perror("ioctl(TUNSETLINK)");
ret = 4;
goto exit_tun_close;
}
memcpy((void*)0x2040,
"\x69\x67\x62\x30\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", 16);
*(uint16_t*)0x2050 = 0xa201;
ret = syscall(__NR_ioctl, sockfd, 0x8914, 0x2040); // SIOCSIFFLAGS 0x8914
if (ret < 0) {
perror("ioctl(SIOCSIFFLAGS)");
ret = 5;
goto exit_tun_close;
}
printf("done:\n");
system("/usr/sbin/ip -details link show igb0");
exit_tun_close:
close(tunfd);
exit_sock_close:
close(sockfd);
exit_end:
munmap((void *)0x2000, 0x100);
return 0;
}
Re: KMSAN: uninit-value in memcmp (2)
Hello, Dmirty, Thank you for the explanation of how syzkaller/syzbot works in this and other emails. I understand that is it a complicated task to determine and categorize bugs based on just crash dump and messages, and syzkaller does a great job of doing so. > Re __hw_addr_add_ex bug, as Alex noted the crash was detected _after_ > the fixing commit went in. So it's something new and different and > can't be fixed by the older commit. Indeed, you're right, there is another issue with tun/tap devices which leads to this bug. I've posted a patch (https://lkml.org/lkml/2018/9/26/416) to fix it. I hope I did not do much damage, reporting previous fix as a fix for this bug, as syzkaller will probably create another "KMSAN: uninit-value in <...>" report. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer - Original Message - > From: "Dmitry Vyukov" > To: "Alexander Potapenko" > Cc: "Vladis Dronov" , "syzbot" > , > "syzkaller-bugs" , "David Miller" > , "Eric Dumazet" > , "LKML" , "Networking" > , "sunlianwen" > > Sent: Monday, September 24, 2018 11:39:08 AM > Subject: Re: KMSAN: uninit-value in memcmp (2) > > On Mon, Sep 24, 2018 at 8:53 AM, Alexander Potapenko > wrote: > > On Mon, Sep 24, 2018 at 12:09 AM Vladis Dronov wrote: > >> > >> Hello, Dmirty, > >> > >> Thank you for the reply. Can we please, discuss this further? > > Hi Vladis, > >> > You can see on dashboard that the last crash > >> > for the second version (2) happened just few days ago. So this is a > >> > different bug. > > FWIW I've just double-checked that the reproducer provided by > > syzkaller in the original message still triggers the report from the > > original message in the latest KMSAN tree (which already contains the > > __hw_addr_add_ex() fix from April). > >> Well... yes and no. When I was looking at this bug (bug?id=088efeac32fd) I > >> was looking > >> at the report at "2018/05/09 18:55" > >> (https://syzkaller.appspot.com/text?tag=CrashReport=141b707b80), > >> since it was the only report with a reproducer. This was my error. > >> > >> The error and the call trace in this report are: > >> > >> >>> > >> BUG: KMSAN: uninit-value in memcmp+0x119/0x180 lib/string.c:861 > >> CPU: 0 PID: 38 Comm: kworker/0:1 Not tainted 4.17.0-rc3+ #88 > >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > >> Google 01/01/2011 > >> Workqueue: ipv6_addrconf addrconf_dad_work > >> Call Trace: > >> __dump_stack lib/dump_stack.c:77 [inline] > >> dump_stack+0x185/0x1d0 lib/dump_stack.c:113 > >> kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067 > >> __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683 > >> memcmp+0x119/0x180 lib/string.c:861 > >> __hw_addr_add_ex net/core/dev_addr_lists.c:61 [inline] > >> __dev_mc_add+0x1fc/0x900 net/core/dev_addr_lists.c:670 > >> dev_mc_add+0x6d/0x80 net/core/dev_addr_lists.c:687 > >> igmp6_group_added+0x2db/0xa00 net/ipv6/mcast.c:662 > >> ipv6_dev_mc_inc+0xe9e/0x1130 net/ipv6/mcast.c:914 > >> addrconf_join_solict net/ipv6/addrconf.c:2103 [inline] > >> addrconf_dad_begin net/ipv6/addrconf.c:3853 [inline] > >> addrconf_dad_work+0x462/0x2a20 net/ipv6/addrconf.c:3979 > >> process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145 > >> worker_thread+0x113c/0x24f0 kernel/workqueue.c:2279 > >> kthread+0x539/0x720 kernel/kthread.c:239 > >> ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:412 > >> > >> Local variable description: buf@igmp6_group_added > >> Variable was created at: > >> igmp6_group_added+0x4a/0xa00 net/ipv6/mcast.c:650 > >> ipv6_dev_mc_inc+0xe9e/0x1130 net/ipv6/mcast.c:914 > >> <<< > >> > >> It is the same like in bug?id=3887c0d99aecb27d085180c5222d245d08a30806 > >> which, after some more test, made me believe these bugs are duplicate > >> and are fixed by the same commit. > >> > >> But let's look at another report at "2018/09/12 21:00" > >> (https://syzkaller.appspot.com/text?tag=CrashReport=14f99b7140) > >> at the bug (bug?id=088efeac32fd), the one you've mentioned as > >> "the last crash for the second version (2) happened just few days ago". > >> > >> Its error and the call trace are completely different: > >> > >> >>> > >> BUG: KMS
[PATCH] xfrm: policy: check policy direction value
The 'dir' parameter in xfrm_migrate() is a user-controlled byte which is used
as an array index. This can lead to an out-of-bound access, kernel lockup and
DoS. Add a check for the 'dir' value.
This fixes CVE-2017-11600.
References: https://bugzilla.redhat.com/show_bug.cgi?id=1474928
Fixes: 80c9abaabf42 ("[XFRM]: Extension for dynamic update of endpoint
address(es)")
Cc: <sta...@vger.kernel.org> # v2.6.21-rc1
Reported-by: "bo Zhang" <zhangbo5891...@gmail.com>
Signed-off-by: Vladis Dronov <vdro...@redhat.com>
---
net/xfrm/xfrm_policy.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index ff61d85..6f5a0dad 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3308,9 +3308,15 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8
dir, u8 type,
struct xfrm_state *x_new[XFRM_MAX_DEPTH];
struct xfrm_migrate *mp;
+ /* Stage 0 - sanity checks */
if ((err = xfrm_migrate_check(m, num_migrate)) < 0)
goto out;
+ if (dir >= XFRM_POLICY_MAX) {
+ err = -EINVAL;
+ goto out;
+ }
+
/* Stage 1 - find policy */
if ((pol = xfrm_migrate_policy_find(sel, dir, type, net)) == NULL) {
err = -ENOENT;
--
2.9.4
[PATCH v2 net] ipv4: Fix misplaced EXPORT_SYMBOL_GPL(ping_hash) in net/ipv4/ping.c
Signed-off-by: Vladis Dronov <vdro...@redhat.com>
---
This is quite a smaill patch, please, feel free not to accept in separately,
but use as a part of any patch of yours.
net/ipv4/ping.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index ccfbce1..19f0b7b 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -71,7 +71,6 @@ static inline u32 ping_hashfn(const struct net *net, u32 num,
u32 mask)
pr_debug("hash(%u) = %u\n", num, res);
return res;
}
-EXPORT_SYMBOL_GPL(ping_hash);
static inline struct hlist_nulls_head *ping_hashslot(struct ping_table *table,
struct net *net, unsigned int num)
@@ -152,6 +151,7 @@ int ping_hash(struct sock *sk)
return 0;
}
+EXPORT_SYMBOL_GPL(ping_hash);
void ping_unhash(struct sock *sk)
{
--
2.9.3
[PATCH] misplaced EXPORT_SYMBOL_GPL(ping_hash) in net/ipv4/ping.c
Move misplaced EXPORT_SYMBOL_GPL(ping_hash) to a proper place.
Signed-off-by: Vladis Dronov <vdro...@redhat.com>
---
Actually, this is so small and unimportant (it just hurts my perfectionism),
so does not worth a separate patch. Please, feel free to make it a part of
some patch of yours.
net/ipv4/ping.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index ccfbce1..19f0b7b 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -71,7 +71,6 @@ static inline u32 ping_hashfn(const struct net *net, u32 num,
u32 mask)
pr_debug("hash(%u) = %u\n", num, res);
return res;
}
-EXPORT_SYMBOL_GPL(ping_hash);
static inline struct hlist_nulls_head *ping_hashslot(struct ping_table *table,
struct net *net, unsigned int num)
@@ -152,6 +151,7 @@ int ping_hash(struct sock *sk)
return 0;
}
+EXPORT_SYMBOL_GPL(ping_hash);
void ping_unhash(struct sock *sk)
{
--
2.9.3
Re: BUG() can be hit in tcp_collapse()
Hello, Eric, Marco, all, This is JFYI and a follow-up message. A further investigation was made to find out the Linux kernel commit which has introduced the flaw. It appeared that previous Linux kernel versions are vulnerable, down to v3.6-rc1. This fact was hidden by 'net.ipv4.tcp_fastopen' set to 0 by default, and now it is easier to notice since kernel v3.12 due to commit 0d41cca490 where the default was changed to 1. With 'net.ipv4.tcp_fastopen' set to 1, previous Linux kernels (including RHEL-7 ones) are also vulnerable. The bug is here since tcp-fastopen feature was introduced in kernel v3.6-rc1, the first commit when the reproducer starts to panic the kernel with net.ipv4.tcp_fastopen=1 set is cf60af03ca, which is a part of commit sequence 2100c8d2d9..67da22d23f introducing net-tcp-fastopen feature: $ git bisect bad cf60af03ca4e71134206809ea892e49b92a88896 cf60af03ca4e71134206809ea892e49b92a88896 is the first bad commit commit cf60af03ca4e71134206809ea892e49b92a88896 Author: Yuchung Cheng <ych...@google.com> Date: Thu Jul 19 06:43:09 2012 + So, ideally, the upstream commit ac6e780070 which fixes the bug should have "Fixes: cf60af03ca" statement, unfortunately, this investigation was not completed at the time the patch was accepted upstream. And unfortunately I do not see other way to add this information except making notes in a comment in the related code, which seems weird. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer
Re: BUG() can be hit in tcp_collapse()
Hello, Eric, > Another sk_filter() is used in tcp v6. > So the correct patch would be : Thank you much for your research. I'm happy my report has resulted as the proposed patch. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer
BUG() can be hit in tcp_collapse()
ing the BUG()). This
is why I'm emailing not only to stable@, but also to netdev@, asking to review
the
data above and probably develop a fix.
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer#ifndef __NR_mmap
#define __NR_mmap 9
#endif
#ifndef __NR_syz_fuse_mount
#define __NR_syz_fuse_mount 104
#endif
#ifndef __NR_syz_test
#define __NR_syz_test 101
#endif
#ifndef __NR_syz_open_dev
#define __NR_syz_open_dev 102
#endif
#ifndef __NR_syz_open_pts
#define __NR_syz_open_pts 103
#endif
#ifndef __NR_socket
#define __NR_socket 41
#endif
#ifndef __NR_bind
#define __NR_bind 49
#endif
#ifndef __NR_sendto
#define __NR_sendto 44
#endif
#ifndef __NR_setsockopt
#define __NR_setsockopt 54
#endif
#ifndef __NR_syz_fuseblk_mount
#define __NR_syz_fuseblk_mount 105
#endif
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
__thread int skip_segv;
__thread jmp_buf segv_env;
static void segv_handler(int sig, siginfo_t* info, void* uctx)
{
if (__atomic_load_n(_segv, __ATOMIC_RELAXED))
_longjmp(segv_env, 1);
exit(sig);
}
static void install_segv_handler()
{
struct sigaction sa;
memset(, 0, sizeof(sa));
sa.sa_sigaction = segv_handler;
sa.sa_flags = SA_NODEFER | SA_SIGINFO;
sigaction(SIGSEGV, , NULL);
sigaction(SIGBUS, , NULL);
}
#define NONFAILING(...)\
{\
__atomic_fetch_add(_segv, 1, __ATOMIC_SEQ_CST); \
if (_setjmp(segv_env) == 0) { \
__VA_ARGS__; \
} \
__atomic_fetch_sub(_segv, 1, __ATOMIC_SEQ_CST); \
}
static uintptr_t syz_open_dev(uintptr_t a0, uintptr_t a1, uintptr_t a2)
{
if (a0 == 0xc || a0 == 0xb) {
char buf[128];
sprintf(buf, "/dev/%s/%d:%d", a0 == 0xc ? "char" : "block",
(uint8_t)a1, (uint8_t)a2);
return open(buf, O_RDWR, 0);
} else {
char buf[1024];
char* hash;
strncpy(buf, (char*)a0, sizeof(buf));
buf[sizeof(buf) - 1] = 0;
while ((hash = strchr(buf, '#'))) {
*hash = '0' + (char)(a1 % 10);
a1 /= 10;
}
return open(buf, a2, 0);
}
}
static uintptr_t syz_open_pts(uintptr_t a0, uintptr_t a1)
{
int ptyno = 0;
if (ioctl(a0, TIOCGPTN, ))
return -1;
char buf[128];
sprintf(buf, "/dev/pts/%d", ptyno);
return open(buf, a1, 0);
}
static uintptr_t syz_fuse_mount(uintptr_t a0, uintptr_t a1,
uintptr_t a2, uintptr_t a3,
uintptr_t a4, uintptr_t a5)
{
uint64_t target = a0;
uint64_t mode = a1;
uint64_t uid = a2;
uint64_t gid = a3;
uint64_t maxread = a4;
uint64_t flags = a5;
int fd = open("/dev/fuse", O_RDWR);
if (fd == -1)
return fd;
char buf[1024];
sprintf(buf, "fd=%d,user_id=%ld,group_id=%ld,rootmode=0%o", fd,
(long)uid, (long)gid, (unsigned)mode & ~3u);
if (maxread != 0)
sprintf(buf + strlen(buf), ",max_read=%ld", (long)maxread);
if (mode & 1)
strcat(buf, ",default_permissions");
if (mode & 2)
strcat(buf, ",allow_other");
syscall(SYS_mount, "", target, "fuse", flags, buf);
return fd;
}
static uintptr_t syz_fuseblk_mount(uintptr_t a0, uintptr_t a1,
uintptr_t a2, uintptr_t a3,
uintptr_t a4, uintptr_t a5,
uintptr_t a6, uintptr_t a7)
{
uint64_t target = a0;
uint64_t blkdev = a1;
uint64_t mode = a2;
uint64_t uid = a3;
uint64_t gid = a4;
uint64_t maxread = a5;
uint64_t blksize = a6;
uint64_t flags = a7;
int fd = open("/dev/fuse", O_RDWR);
if (fd == -1)
return fd;
if (syscall(SYS_mknodat, AT_FDCWD, blkdev, S_IFBLK, makedev(7, 199)))
return fd;
char buf[256];
sprintf(buf, "fd=%d,user_id=%ld,group_id=%ld,rootmode=0%o", fd,
(long)uid, (long)gid, (unsigned)mode & ~3u);
if (maxread != 0)
sprintf(buf + strlen(buf), ",max_read=%ld", (long)maxread);
if (blksize != 0)
sprintf(buf + strlen(buf), ",blksize=%ld", (long)blksize);
if (mode & 1)
strcat(buf, ",default_permissions");
if (mode & 2)
strcat(buf, ",allow_other");
syscall(SYS_mount, blkdev, target, "fuseblk", flags, buf);
return fd;
}
static uintptr_t execute_syscall(int nr, uintptr_t a0, uintptr_t a1,
uintptr_t a2, uintptr_t a3,
uintptr_t a4, uintptr_t a5,
uintptr_t a6, uintptr_t a7,
uintptr_t a8)
{
switch (nr) {
