Re: [PATCH 4.14 43/43] tipc: pass tunnel dev as NULL to udp_tunnel(6)_xmit_skb

[Rantala, Tommi T. (Nokia - FI/Espoo)]

On Tue, 2019-07-02 at 10:02 +0200, Greg Kroah-Hartman wrote:
> From: Xin Long <lucien.xin@xxxxxxxxx>
> 
> commit c3bcde026684c62d7a2b6f626dc7cf763833875c upstream.
> 
> udp_tunnel(6)_xmit_skb() called by tipc_udp_xmit() expects a tunnel
> device
> to count packets on dev->tstats, a perpcu variable. However, TIPC is
> using
> udp tunnel with no tunnel device, and pass the lower dev, like veth
> device
> that only initializes dev->lstats(a perpcu variable) when creating
> it.

Hi,

This tipc patch added in 4.14.132 is triggering a crash for me, revert
fixes it.

Anyone have ideas if some other commits missing in 4.14.x to make this
work...?


# modprobe tipc
# tipc node set addr 1.1.2
# tipc bearer enable media udp name UDP1 localip 192.168.1.15

[  143.105529] Own node address <1.1.2>, network identity 4711
[  172.087098] BUG: unable to handle kernel NULL pointer dereference at
00000000000004f0
[  172.088375] IP: iptunnel_xmit+0x15e/0x1e0
[  172.089072] PGD 8000000231306067 P4D 8000000231306067 PUD 2356e1067
PMD 0
[  172.090094] Oops: 0000 [#1] SMP PTI
[  172.090610] Modules linked in: tipc ip6_udp_tunnel udp_tunnel isofs
kvm_intel kvm irqbypass sch_fq_codel pcbc aesni_intel aes_x86_64
crypto_simd cryptd glue_helper ata_piix dm_mirror dm_region_hash dm_log
dm_mod dax autofs4
[  172.093293] CPU: 1 PID: 747 Comm: tipc Not tainted 4.14.134-1.x86_64 
#1
[  172.094448] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.12.0-2.fc30 04/01/2014
[  172.095703] task: ffff8b99f12c0000 task.stack: ffff9ab481198000
[  172.096731] RIP: 0010:iptunnel_xmit+0x15e/0x1e0
[  172.097460] RSP: 0018:ffff9ab48119ba00 EFLAGS: 00010202
[  172.098214] RAX: 0000000000000000 RBX: ffffffffbf4d8140 RCX:
000000000000008c
[  172.099320] RDX: 0000000000000001 RSI: 00000000fffffe01 RDI:
ffffffffbe944d62
[  172.100392] RBP: ffff8b99f1e7ed00 R08: ffff8b99ffc64520 R09:
0000000000000000
[  172.101451] R10: 000000023426d000 R11: 0000000000000002 R12:
0000000000000000
[  172.102607] R13: 0000000000000040 R14: 0000000000000000 R15:
ffff8b99f426e0e8
[  172.103728] FS:  00007efc82b96800(0000) GS:ffff8b99ffc40000(0000)
knlGS:0000000000000000
[  172.104976] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  172.105821] CR2: 00000000000004f0 CR3: 0000000234250001 CR4:
00000000003606e0
[  172.106981] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[  172.108120] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[  172.109386] Call Trace:
[  172.109808]  tipc_udp_xmit.isra.18+0x1a7/0x1c0 [tipc]
[  172.110687]  ? __internal_add_timer+0x1a/0x50
[  172.111369]  ? __skb_clone+0x29/0x130
[  172.111999]  tipc_bearer_xmit_skb+0x4d/0x80 [tipc]
[  172.112845]  tipc_enable_bearer+0x2b9/0x3c0 [tipc]
[  172.113637]  ? __nla_put+0xc/0x20
[  172.114213]  tipc_nl_bearer_enable+0xca/0x100 [tipc]
[  172.114952]  genl_family_rcv_msg+0x190/0x390
[  172.115748]  genl_rcv_msg+0x47/0x90
[  172.116287]  ? __alloc_skb+0x72/0x1b0
[  172.116898]  ? genl_family_rcv_msg+0x390/0x390
[  172.117669]  netlink_rcv_skb+0x3d/0x100
[  172.118361]  genl_rcv+0x24/0x40
[  172.119005]  netlink_unicast+0x16d/0x230
[  172.119777]  netlink_sendmsg+0x1ae/0x3c0
[  172.120525]  SYSC_sendto+0xe6/0x140
[  172.121248]  ? SYSC_getsockname+0x81/0xa0
[  172.121989]  ? sock_alloc_file+0x97/0x120
[  172.122645]  ? sock_map_fd+0x3d/0x60
[  172.123278]  do_syscall_64+0x74/0x190
[  172.123911]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[  172.124716] RIP: 0033:0x7efc82d6ac6b
[  172.125368] RSP: 002b:00007fff40411ae8 EFLAGS: 00000246 ORIG_RAX:
000000000000002c
[  172.126486] RAX: ffffffffffffffda RBX: 0000000001dfca20 RCX:
00007efc82d6ac6b
[  172.127632] RDX: 0000000000000054 RSI: 00007fff40411b60 RDI:
0000000000000003
[  172.128765] RBP: 00007fff40411b50 R08: 00007efc82e36000 R09:
000000000000000c
[  172.129793] R10: 0000000000000000 R11: 0000000000000246 R12:
00007fff40411b60
[  172.130799] R13: 00007fff40412d10 R14: 000000000040bb44 R15:
0000000000000000
[  172.131868] Code: 01 00 00 00 85 d2 0f 44 d0 e8 1f f3 fa ff 48 8b 74
24 08 4c 89 fa 48 89 df e8 9f 94 fb ff 83 e0 fd 75 35 8b 4c 24 1c 85 c9
7e 2b <49> 8b 84 24 f0 04 00 00 65 48 03 05 aa 29 68 41 48 83 40 10 01
[  172.134773] RIP: iptunnel_xmit+0x15e/0x1e0 RSP: ffff9ab48119ba00
[  172.135697] CR2: 00000000000004f0
[  172.136305] ---[ end trace 27f7522ade26797f ]---


> Later iptunnel_xmit_stats() called by ip(6)tunnel_xmit() thinks the
> dev as
> a tunnel device, and uses dev->tstats instead of dev->lstats. tstats'
> each
> pointer points to a bigger struct than lstats, so when tstats-
> >tx_bytes is
> increased, other percpu variable's members could be overwritten.
> 
> syzbot has reported quite a few crashes due to fib_nh_common percpu
> member
> 'nhc_pcpu_rth_output' overwritten, call traces are like:
> 
>   BUG: KASAN: slab-out-of-bounds in rt_cache_valid+0x158/0x190
>   net/ipv4/route.c:1556
>     rt_cache_valid+0x158/0x190 net/ipv4/route.c:1556
>     __mkroute_output net/ipv4/route.c:2332 [inline]
>     ip_route_output_key_hash_rcu+0x819/0x2d50 net/ipv4/route.c:2564
>     ip_route_output_key_hash+0x1ef/0x360 net/ipv4/route.c:2393
>     __ip_route_output_key include/net/route.h:125 [inline]
>     ip_route_output_flow+0x28/0xc0 net/ipv4/route.c:2651
>     ip_route_output_key include/net/route.h:135 [inline]
>   ...
> 
> or:
> 
>   kasan: GPF could be caused by NULL-ptr deref or user memory access
>   RIP: 0010:dst_dev_put+0x24/0x290 net/core/dst.c:168
>     <IRQ>
>     rt_fibinfo_free_cpus net/ipv4/fib_semantics.c:200 [inline]
>     free_fib_info_rcu+0x2e1/0x490 net/ipv4/fib_semantics.c:217
>     __rcu_reclaim kernel/rcu/rcu.h:240 [inline]
>     rcu_do_batch kernel/rcu/tree.c:2437 [inline]
>     invoke_rcu_callbacks kernel/rcu/tree.c:2716 [inline]
>     rcu_process_callbacks+0x100a/0x1ac0 kernel/rcu/tree.c:2697
>   ...
> 
> The issue exists since tunnel stats update is moved to iptunnel_xmit
> by
> Commit 039f50629b7f ("ip_tunnel: Move stats update to
> iptunnel_xmit()"),
> and here to fix it by passing a NULL tunnel dev to
> udp_tunnel(6)_xmit_skb
> so that the packets counting won't happen on dev->tstats.
> 
> Reported-by: syzbot+9d4c12bfd45a58738d0a@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+a9e23ea2aa21044c2798@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+c4c4b2bb358bb936ad7e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+0290d2290a607e035ba1@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+a43d8d4e7e8a7a9e149e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+a47c5f4c6c00fc1ed16e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 039f50629b7f ("ip_tunnel: Move stats update to
> iptunnel_xmit()")
> Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> 
> ---
>  net/tipc/udp_media.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> --- a/net/tipc/udp_media.c
> +++ b/net/tipc/udp_media.c
> @@ -174,7 +174,6 @@ static int tipc_udp_xmit(struct net *net
>  			goto tx_error;
>  		}
>  
> -		skb->dev = rt->dst.dev;
>  		ttl = ip4_dst_hoplimit(&rt->dst);
>  		udp_tunnel_xmit_skb(rt, ub->ubsock->sk, skb, src-
> >ipv4.s_addr,
>  				    dst->ipv4.s_addr, 0, ttl, 0, src-
> >port,
> @@ -193,10 +192,9 @@ static int tipc_udp_xmit(struct net *net
>  		if (err)
>  			goto tx_error;
>  		ttl = ip6_dst_hoplimit(ndst);
> -		err = udp_tunnel6_xmit_skb(ndst, ub->ubsock->sk, skb,
> -					   ndst->dev, &src->ipv6,
> -					   &dst->ipv6, 0, ttl, 0, src-
> >port,
> -					   dst->port, false);
> +		err = udp_tunnel6_xmit_skb(ndst, ub->ubsock->sk, skb,
> NULL,
> +					   &src->ipv6, &dst->ipv6, 0,
> ttl, 0,
> +					   src->port, dst->port,
> false);
>  #endif
>  	}
>  	return err;
> 
>