Date:   Fri, 10 Aug 2018 09:54:02 -0700
From:   Wei Wang <weiwan@...gle.com>
To:     g.nault@...halink.fr
Cc:     "David S . Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        David Ahern <dsahern@...il.com>,
        Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [PATCH net] l2tp: use sk_dst_check() to avoid race on sk->sk_dst_cache

On Fri, Aug 10, 2018 at 8:59 AM Guillaume Nault <g.nault@...halink.fr> wrote:
>
> On Thu, Aug 09, 2018 at 11:54:05AM -0700, Wei Wang wrote:
> > From: Wei Wang <weiwan@...gle.com>
> >
> > In l2tp code, if it is a L2TP_UDP_ENCAP tunnel, tunnel->sk points to a
> > UDP socket. User could call sendmsg() on both this tunnel and the UDP
> > socket itself concurrently. As l2tp_xmit_skb() holds socket lock and call
> > __sk_dst_check() to refresh sk->sk_dst_cache, while udpv6_sendmsg() is
> > lockless and call sk_dst_check() to refresh sk->sk_dst_cache, there
> > could be a race and cause the dst cache to be freed multiple times.
> > So we fix l2tp side code to always call sk_dst_check() to garantee
> > xchg() is called when refreshing sk->sk_dst_cache to avoid race
> > conditions.
> >
> > Syzkaller reported stack trace:
> > BUG: KASAN: use-after-free in atomic_read include/asm-generic/atomic-instrumented.h:21 [inline]
> > BUG: KASAN: use-after-free in atomic_fetch_add_unless include/linux/atomic.h:575 [inline]
> > BUG: KASAN: use-after-free in atomic_add_unless include/linux/atomic.h:597 [inline]
> > BUG: KASAN: use-after-free in dst_hold_safe include/net/dst.h:308 [inline]
> > BUG: KASAN: use-after-free in ip6_hold_safe+0xe6/0x670 net/ipv6/route.c:1029
> > Read of size 4 at addr ffff8801aea9a880 by task syz-executor129/4829
> >
> > CPU: 0 PID: 4829 Comm: syz-executor129 Not tainted 4.18.0-rc7-next-20180802+ #30
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > Call Trace:
> >  __dump_stack lib/dump_stack.c:77 [inline]
> >  dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
> >  print_address_description+0x6c/0x20b mm/kasan/report.c:256
> >  kasan_report_error mm/kasan/report.c:354 [inline]
> >  kasan_report.cold.7+0x242/0x30d mm/kasan/report.c:412
> >  check_memory_region_inline mm/kasan/kasan.c:260 [inline]
> >  check_memory_region+0x13e/0x1b0 mm/kasan/kasan.c:267
> >  kasan_check_read+0x11/0x20 mm/kasan/kasan.c:272
> >  atomic_read include/asm-generic/atomic-instrumented.h:21 [inline]
> >  atomic_fetch_add_unless include/linux/atomic.h:575 [inline]
> >  atomic_add_unless include/linux/atomic.h:597 [inline]
> >  dst_hold_safe include/net/dst.h:308 [inline]
> >  ip6_hold_safe+0xe6/0x670 net/ipv6/route.c:1029
> >  rt6_get_pcpu_route net/ipv6/route.c:1249 [inline]
> >  ip6_pol_route+0x354/0xd20 net/ipv6/route.c:1922
> >  ip6_pol_route_output+0x54/0x70 net/ipv6/route.c:2098
> >  fib6_rule_lookup+0x283/0x890 net/ipv6/fib6_rules.c:122
> >  ip6_route_output_flags+0x2c5/0x350 net/ipv6/route.c:2126
> >  ip6_dst_lookup_tail+0x1278/0x1da0 net/ipv6/ip6_output.c:978
> >  ip6_dst_lookup_flow+0xc8/0x270 net/ipv6/ip6_output.c:1079
> >  ip6_sk_dst_lookup_flow+0x5ed/0xc50 net/ipv6/ip6_output.c:1117
> >  udpv6_sendmsg+0x2163/0x36b0 net/ipv6/udp.c:1354
> >  inet_sendmsg+0x1a1/0x690 net/ipv4/af_inet.c:798
> >  sock_sendmsg_nosec net/socket.c:622 [inline]
> >  sock_sendmsg+0xd5/0x120 net/socket.c:632
> >  ___sys_sendmsg+0x51d/0x930 net/socket.c:2115
> >  __sys_sendmmsg+0x240/0x6f0 net/socket.c:2210
> >  __do_sys_sendmmsg net/socket.c:2239 [inline]
> >  __se_sys_sendmmsg net/socket.c:2236 [inline]
> >  __x64_sys_sendmmsg+0x9d/0x100 net/socket.c:2236
> >  do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
> >  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > RIP: 0033:0x446a29
> > Code: e8 ac b8 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 eb 08 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> > RSP: 002b:00007f4de5532db8 EFLAGS: 00000246 ORIG_RAX: 0000000000000133
> > RAX: ffffffffffffffda RBX: 00000000006dcc38 RCX: 0000000000446a29
> > RDX: 00000000000000b8 RSI: 0000000020001b00 RDI: 0000000000000003
> > RBP: 00000000006dcc30 R08: 00007f4de5533700 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000246 R12: 00000000006dcc3c
> > R13: 00007ffe2b830fdf R14: 00007f4de55339c0 R15: 0000000000000001
> >
> > Fixes: 71b1391a4128 ("l2tp: ensure sk->dst is still valid")
> > Reported-by: syzbot+05f840f3b04f211bad55@...kaller.appspotmail.com
> > Signed-off-by: Wei Wang <weiwan@...gle.com>
> > Signed-off-by: Martin KaFai Lau <kafai@...com>
> > Cc: David Ahern <dsahern@...il.com>
> > Cc: Cong Wang <xiyou.wangcong@...il.com>
> > ---
> >  net/l2tp/l2tp_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > index 40261cb68e83..7166b61338d4 100644
> > --- a/net/l2tp/l2tp_core.c
> > +++ b/net/l2tp/l2tp_core.c
> > @@ -1110,7 +1110,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
> >
> >       /* Get routing info from the tunnel socket */
> >       skb_dst_drop(skb);
> > -     skb_dst_set(skb, dst_clone(__sk_dst_check(sk, 0)));
> > +     skb_dst_set(skb, dst_clone(sk_dst_check(sk, 0)));
> >
> With sk_dst_check(), we're now holding an extra reference on the cached
> dst. How is it dropped? Shouldn't we remove dst_clone()?
>
Yes. You are right. dst_clone() should get removed because
sk_dst_check() already gets a reference. I will send a v2 to correct
this.

> Also, does syzbot have a reproducer? I haven't found the original
> report on the mailing list.
This bug is reported in google internally.
I attached the .config and repro.c here.

View attachment "repro.c" of type "text/x-csrc" (12572 bytes)

Download attachment ".config" of type "application/octet-stream" (141479 bytes)

Powered by blists - more mailing lists