diff options
author | Eric Dumazet <edumazet@google.com> | 2019-12-07 23:10:34 +0100 |
---|---|---|
committer | David S. Miller <davem@davemloft.net> | 2019-12-09 18:49:07 +0100 |
commit | 9e99bfefdbce2e23ef37487a3bcb4adf90a791d1 (patch) | |
tree | 58af530fd317f74788573ae540f2b634235d6c18 /drivers/net/bonding/bond_main.c | |
parent | neighbour: remove neigh_cleanup() method (diff) | |
download | linux-9e99bfefdbce2e23ef37487a3bcb4adf90a791d1.tar.xz linux-9e99bfefdbce2e23ef37487a3bcb4adf90a791d1.zip |
bonding: fix bond_neigh_init()
1) syzbot reported an uninit-value in bond_neigh_setup() [1]
bond_neigh_setup() uses a temporary on-stack 'struct neigh_parms parms',
but only clears parms.neigh_setup field.
A stacked bonding device would then enter bond_neigh_setup()
and read garbage from parms->dev.
If we get really unlucky and garbage is matching @dev, then we
could recurse and eventually crash.
Let's make sure the whole structure is cleared to avoid surprises.
2) bond_neigh_setup() can be called while another cpu manipulates
the master device, removing or adding a slave.
We need at least rcu protection to prevent use-after-free.
Note: Prior code does not support a stack of bonding devices,
this patch does not attempt to fix this, and leave a comment instead.
[1]
BUG: KMSAN: uninit-value in bond_neigh_setup+0xa4/0x110 drivers/net/bonding/bond_main.c:3655
CPU: 0 PID: 11256 Comm: syz-executor.0 Not tainted 5.4.0-rc8-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
<IRQ>
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1c9/0x220 lib/dump_stack.c:118
kmsan_report+0x128/0x220 mm/kmsan/kmsan_report.c:108
__msan_warning+0x57/0xa0 mm/kmsan/kmsan_instr.c:245
bond_neigh_setup+0xa4/0x110 drivers/net/bonding/bond_main.c:3655
bond_neigh_init+0x216/0x4b0 drivers/net/bonding/bond_main.c:3626
___neigh_create+0x169e/0x2c40 net/core/neighbour.c:613
__neigh_create+0xbd/0xd0 net/core/neighbour.c:674
ip6_finish_output2+0x149a/0x2670 net/ipv6/ip6_output.c:113
__ip6_finish_output+0x83d/0x8f0 net/ipv6/ip6_output.c:142
ip6_finish_output+0x2db/0x420 net/ipv6/ip6_output.c:152
NF_HOOK_COND include/linux/netfilter.h:294 [inline]
ip6_output+0x5d3/0x720 net/ipv6/ip6_output.c:175
dst_output include/net/dst.h:436 [inline]
NF_HOOK include/linux/netfilter.h:305 [inline]
mld_sendpack+0xebd/0x13d0 net/ipv6/mcast.c:1682
mld_send_cr net/ipv6/mcast.c:1978 [inline]
mld_ifc_timer_expire+0x116b/0x1680 net/ipv6/mcast.c:2477
call_timer_fn+0x232/0x530 kernel/time/timer.c:1404
expire_timers kernel/time/timer.c:1449 [inline]
__run_timers+0xd60/0x1270 kernel/time/timer.c:1773
run_timer_softirq+0x2d/0x50 kernel/time/timer.c:1786
__do_softirq+0x4a1/0x83a kernel/softirq.c:293
invoke_softirq kernel/softirq.c:375 [inline]
irq_exit+0x230/0x280 kernel/softirq.c:416
exiting_irq+0xe/0x10 arch/x86/include/asm/apic.h:536
smp_apic_timer_interrupt+0x48/0x70 arch/x86/kernel/apic/apic.c:1138
apic_timer_interrupt+0x2e/0x40 arch/x86/entry/entry_64.S:835
</IRQ>
RIP: 0010:kmsan_free_page+0x18d/0x1c0 mm/kmsan/kmsan_shadow.c:439
Code: 4c 89 ff 44 89 f6 e8 82 0d ee ff 65 ff 0d 9f 26 3b 60 65 8b 05 98 26 3b 60 85 c0 75 24 e8 5b f6 35 ff 4c 89 6d d0 ff 75 d0 9d <48> 83 c4 10 5b 41 5c 41 5d 41 5e 41 5f 5d c3 0f 0b 0f 0b 0f 0b 0f
RSP: 0018:ffffb328034af818 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
RAX: 0000000000000000 RBX: ffffe2d7471f8360 RCX: 0000000000000000
RDX: ffffffffadea7000 RSI: 0000000000000004 RDI: ffff93496fcda104
RBP: ffffb328034af850 R08: ffff934a47e86d00 R09: ffff93496fc41900
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
R13: 0000000000000246 R14: 0000000000000000 R15: ffffe2d7472225c0
free_pages_prepare mm/page_alloc.c:1138 [inline]
free_pcp_prepare mm/page_alloc.c:1230 [inline]
free_unref_page_prepare+0x1d9/0x770 mm/page_alloc.c:3025
free_unref_page mm/page_alloc.c:3074 [inline]
free_the_page mm/page_alloc.c:4832 [inline]
__free_pages+0x154/0x230 mm/page_alloc.c:4840
__vunmap+0xdac/0xf20 mm/vmalloc.c:2277
__vfree mm/vmalloc.c:2325 [inline]
vfree+0x7c/0x170 mm/vmalloc.c:2355
copy_entries_to_user net/ipv6/netfilter/ip6_tables.c:883 [inline]
get_entries net/ipv6/netfilter/ip6_tables.c:1041 [inline]
do_ip6t_get_ctl+0xfa4/0x1030 net/ipv6/netfilter/ip6_tables.c:1709
nf_sockopt net/netfilter/nf_sockopt.c:104 [inline]
nf_getsockopt+0x481/0x4e0 net/netfilter/nf_sockopt.c:122
ipv6_getsockopt+0x264/0x510 net/ipv6/ipv6_sockglue.c:1400
tcp_getsockopt+0x1c6/0x1f0 net/ipv4/tcp.c:3688
sock_common_getsockopt+0x13f/0x180 net/core/sock.c:3110
__sys_getsockopt+0x533/0x7b0 net/socket.c:2129
__do_sys_getsockopt net/socket.c:2144 [inline]
__se_sys_getsockopt+0xe1/0x100 net/socket.c:2141
__x64_sys_getsockopt+0x62/0x80 net/socket.c:2141
do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:291
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x45d20a
Code: b8 34 01 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 8d 8b fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 49 89 ca b8 37 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 83 6a 8b fb ff c3 66 0f 1f 84 00 00 00 00 00
RSP: 002b:0000000000a6f618 EFLAGS: 00000212 ORIG_RAX: 0000000000000037
RAX: ffffffffffffffda RBX: 0000000000a6f640 RCX: 000000000045d20a
RDX: 0000000000000041 RSI: 0000000000000029 RDI: 0000000000000003
RBP: 0000000000717cc0 R08: 0000000000a6f63c R09: 0000000000004000
R10: 0000000000a6f740 R11: 0000000000000212 R12: 0000000000000003
R13: 0000000000000000 R14: 0000000000000029 R15: 0000000000715b00
Local variable description: ----parms@bond_neigh_init
Variable was created at:
bond_neigh_init+0x8c/0x4b0 drivers/net/bonding/bond_main.c:3617
bond_neigh_init+0x8c/0x4b0 drivers/net/bonding/bond_main.c:3617
Fixes: 9918d5bf329d ("bonding: modify only neigh_parms owned by us")
Fixes: 234bcf8a499e ("net/bonding: correctly proxy slave neigh param setup ndo function")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Veaceslav Falico <vfalico@gmail.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: David S. Miller <davem@davemloft.net>
Diffstat (limited to 'drivers/net/bonding/bond_main.c')
-rw-r--r-- | drivers/net/bonding/bond_main.c | 31 |
1 files changed, 21 insertions, 10 deletions
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 6c72623e48e5..041aa9649dfc 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -3702,24 +3702,35 @@ static int bond_neigh_init(struct neighbour *n) const struct net_device_ops *slave_ops; struct neigh_parms parms; struct slave *slave; - int ret; + int ret = 0; - slave = bond_first_slave(bond); + rcu_read_lock(); + slave = bond_first_slave_rcu(bond); if (!slave) - return 0; + goto out; slave_ops = slave->dev->netdev_ops; if (!slave_ops->ndo_neigh_setup) - return 0; + goto out; - parms.neigh_setup = NULL; + /* TODO: find another way [1] to implement this. + * Passing a zeroed structure is fragile, + * but at least we do not pass garbage. + * + * [1] One way would be that ndo_neigh_setup() never touch + * struct neigh_parms, but propagate the new neigh_setup() + * back to ___neigh_create() / neigh_parms_alloc() + */ + memset(&parms, 0, sizeof(parms)); ret = slave_ops->ndo_neigh_setup(slave->dev, &parms); - if (ret) - return ret; - if (!parms.neigh_setup) - return 0; + if (ret) + goto out; - return parms.neigh_setup(n); + if (parms.neigh_setup) + ret = parms.neigh_setup(n); +out: + rcu_read_unlock(); + return ret; } /* The bonding ndo_neigh_setup is called at init time beofre any |