Message-ID: <CANn89iLXk-=Zh9va4eKM+35vx4=sn8p_Jvu5xAkHphOyh07B7w@mail.gmail.com>
Date: Thu, 3 Aug 2023 09:11:40 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, socketcan@...tkopp.net, davem@...emloft.net, 
	kuba@...nel.org, pabeni@...hat.com, linux-can@...r.kernel.org, 
	netdev@...r.kernel.org, penguin-kernel@...ove.sakura.ne.jp
Subject: Re: [PATCH net v3] can: raw: fix receiver memory leak

On Thu, Aug 3, 2023 at 2:44 AM Ziyang Xuan (William)
<william.xuanziyang@...wei.com> wrote:
>
> >>> On Wed, Jul 19, 2023 at 6:41 AM Ziyang Xuan (William)
> >>> <william.xuanziyang@...wei.com> wrote:
> >>>>
> >>>>> On Mon, Jul 17, 2023 at 9:27 AM Marc Kleine-Budde <mkl@...gutronix.de> wrote:
> >>>>>>
> >>>>>> On 11.07.2023 09:17:37, Ziyang Xuan wrote:
> >>>>>>> Got kmemleak errors with the following ltp can_filter testcase:
> >>>>>>>
> >>>>>>> for ((i=1; i<=100; i++))
> >>>>>>> do
> >>>>>>>         ./can_filter &
> >>>>>>>         sleep 0.1
> >>>>>>> done
> >>>>>>>
> >>>>>>> ==============================================================
> >>>>>>> [<00000000db4a4943>] can_rx_register+0x147/0x360 [can]
> >>>>>>> [<00000000a289549d>] raw_setsockopt+0x5ef/0x853 [can_raw]
> >>>>>>> [<000000006d3d9ebd>] __sys_setsockopt+0x173/0x2c0
> >>>>>>> [<00000000407dbfec>] __x64_sys_setsockopt+0x61/0x70
> >>>>>>> [<00000000fd468496>] do_syscall_64+0x33/0x40
> >>>>>>> [<00000000b7e47d51>] entry_SYSCALL_64_after_hwframe+0x61/0xc6
> >>>>>>>
> >>>>>>> It's a bug in the concurrent scenario of unregister_netdevice_many()
> >>>>>>> and raw_release() as following:
> >>>>>>>
> >>>>>>>              cpu0                                        cpu1
> >>>>>>> unregister_netdevice_many(can_dev)
> >>>>>>>   unlist_netdevice(can_dev) // dev_get_by_index() return NULL after this
> >>>>>>>   net_set_todo(can_dev)
> >>>>>>>                                               raw_release(can_socket)
> >>>>>>>                                                 dev = dev_get_by_index(, ro->ifindex); // dev == NULL
> >>>>>>>                                                 if (dev) { // receivers in dev_rcv_lists not free because dev is NULL
> >>>>>>>                                                   raw_disable_allfilters(, dev, );
> >>>>>>>                                                   dev_put(dev);
> >>>>>>>                                                 }
> >>>>>>>                                                 ...
> >>>>>>>                                                 ro->bound = 0;
> >>>>>>>                                                 ...
> >>>>>>>
> >>>>>>> call_netdevice_notifiers(NETDEV_UNREGISTER, )
> >>>>>>>   raw_notify(, NETDEV_UNREGISTER, )
> >>>>>>>     if (ro->bound) // invalid because ro->bound has been set 0
> >>>>>>>       raw_disable_allfilters(, dev, ); // receivers in dev_rcv_lists will never be freed
> >>>>>>>
> >>>>>>> Add a net_device pointer member in struct raw_sock to record bound can_dev,
> >>>>>>> and use rtnl_lock to serialize raw_socket members between raw_bind(), raw_release(),
> >>>>>>> raw_setsockopt() and raw_notify(). Use ro->dev to decide whether to free receivers in
> >>>>>>> dev_rcv_lists.
> >>>>>>>
> >>>>>>> Fixes: 8d0caedb7596 ("can: bcm/raw/isotp: use per module netdevice notifier")
> >>>>>>> Signed-off-by: Ziyang Xuan <william.xuanziyang@...wei.com>
> >>>>>>> Reviewed-by: Oliver Hartkopp <socketcan@...tkopp.net>
> >>>>>>> Acked-by: Oliver Hartkopp <socketcan@...tkopp.net>
> >>>>>>
> >>>>>> Added to linux-can/testing.
> >>>>>>
> >>>>>
> >>>>> This patch causes three syzbot LOCKDEP reports so far.
> >>>>
> >>>> Hello Eric,
> >>>>
> >>>> Is there reproducer? I want to understand the specific root cause.
> >>>>
> >>>
> >>> No repro yet, but simply look at other functions in net/can/raw.c
> >>>
> >>> You must always take locks in the same order.
> >>>
> >>> raw_bind(), raw_setsockopt() use:
> >>>
> >>> rtnl_lock();
> >>> lock_sock(sk);
> >>>
> >>> Therefore, raw_release() must _also_ use the same order, or risk deadlock.
> >>>
> >>> Please build a LOCKDEP enabled kernel, and run your tests ?
> >>
> >> I know now. This needs raw_bind() and raw_setsockopt() concurrent with raw_release().
> >> And there is not the scenario in my current testcase. I did not get it. I will try to
> >> reproduce it and add the testcase.
> >>
> >> Thank you for your patient explanation.
> >
> > Another syzbot report is firing because of your patch
> >
> > Apparently we store in ro->dev a pointer to a netdev without holding a
> > refcount on it.
> > .
> Hello Eric,
>
> Is there a syzbot link or reproducer can be provided?
>

Not yet, but really we should not cache a dev pointer without making
sure it does not disappear later.

Caching the ifindex is fine though.

BUG: KASAN: use-after-free in read_pnet include/net/net_namespace.h:383 [inline]
BUG: KASAN: use-after-free in dev_net include/linux/netdevice.h:2590 [inline]
BUG: KASAN: use-after-free in raw_release+0x960/0x9b0 net/can/raw.c:395
Read of size 8 at addr ffff88802b9b8670 by task syz-executor.1/7705

CPU: 0 PID: 7705 Comm: syz-executor.1 Not tainted
6.5.0-rc3-syzkaller-00176-g13d2618b48f1 #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 07/12/2023
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106
print_address_description mm/kasan/report.c:364 [inline]
print_report+0xc4/0x620 mm/kasan/report.c:475
kasan_report+0xda/0x110 mm/kasan/report.c:588
read_pnet include/net/net_namespace.h:383 [inline]
dev_net include/linux/netdevice.h:2590 [inline]
raw_release+0x960/0x9b0 net/can/raw.c:395
__sock_release+0xcd/0x290 net/socket.c:654
sock_close+0x1c/0x20 net/socket.c:1386
__fput+0x3fd/0xac0 fs/file_table.c:384
task_work_run+0x14d/0x240 kernel/task_work.c:179
resume_user_mode_work include/linux/resume_user_mode.h:49 [inline]
exit_to_user_mode_loop kernel/entry/common.c:171 [inline]
exit_to_user_mode_prepare+0x210/0x240 kernel/entry/common.c:204
__syscall_exit_to_user_mode_work kernel/entry/common.c:286 [inline]
syscall_exit_to_user_mode+0x1d/0x50 kernel/entry/common.c:297
do_syscall_64+0x44/0xb0 arch/x86/entry/common.c:86
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fb7e927b9da
Code: 48 3d 00 f0 ff ff 77 48 c3 0f 1f 80 00 00 00 00 48 83 ec 18 89
7c 24 0c e8 03 7f 02 00 8b 7c 24 0c 89 c2 b8 03 00 00 00 0f 05 <48> 3d
00 f0 ff ff 77 36 89 d7 89 44 24 0c e8 63 7f 02 00 8b 44 24
RSP: 002b:00007ffd15e54c90 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
RAX: 0000000000000000 RBX: 000000000000000a RCX: 00007fb7e927b9da
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000009
RBP: 00007fb7e939d980 R08: 0000001b2fb20000 R09: 000000000000040e
R10: 000000008190df57 R11: 0000000000000293 R12: 000000000011244a
R13: ffffffffffffffff R14: 00007fb7e8e00000 R15: 0000000000112109
</TASK>

The buggy address belongs to the physical page:
page:ffffea0000ae6e00 refcount:0 mapcount:0 mapping:0000000000000000
index:0x0 pfn:0x2b9b8
flags: 0xfff00000000000(node=0|zone=1|lastcpupid=0x7ff)
page_type: 0xffffffff()
raw: 00fff00000000000 ffffea0001e32208 ffffea0000ae7c08 0000000000000000
raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as freed
page last allocated via order 3, migratetype Unmovable, gfp_mask
0x446dc0(GFP_KERNEL_ACCOUNT|__GFP_NOWARN|__GFP_RETRY_MAYFAIL|__GFP_COMP|__GFP_ZERO),
pid 5067, tgid 5067 (syz-executor.1), ts 153775196806, free_ts
1123753306319
set_page_owner include/linux/page_owner.h:31 [inline]
post_alloc_hook+0x2d2/0x350 mm/page_alloc.c:1570
prep_new_page mm/page_alloc.c:1577 [inline]
get_page_from_freelist+0x10a9/0x31e0 mm/page_alloc.c:3221
__alloc_pages+0x1d0/0x4a0 mm/page_alloc.c:4477
__alloc_pages_node include/linux/gfp.h:237 [inline]
alloc_pages_node include/linux/gfp.h:260 [inline]
__kmalloc_large_node+0x87/0x1c0 mm/slab_common.c:1126
__do_kmalloc_node mm/slab_common.c:973 [inline]
__kmalloc_node.cold+0x5/0xdd mm/slab_common.c:992
kmalloc_node include/linux/slab.h:602 [inline]
kvmalloc_node+0x6f/0x1a0 mm/util.c:604
kvmalloc include/linux/slab.h:720 [inline]
kvzalloc include/linux/slab.h:728 [inline]
alloc_netdev_mqs+0x9b/0x1240 net/core/dev.c:10594
rtnl_create_link+0xc9c/0xfd0 net/core/rtnetlink.c:3336
rtnl_newlink_create net/core/rtnetlink.c:3462 [inline]
__rtnl_newlink+0x1075/0x18c0 net/core/rtnetlink.c:3689
rtnl_newlink+0x67/0xa0 net/core/rtnetlink.c:3702
rtnetlink_rcv_msg+0x439/0xd30 net/core/rtnetlink.c:6428
netlink_rcv_skb+0x16b/0x440 net/netlink/af_netlink.c:2549
netlink_unicast_kernel net/netlink/af_netlink.c:1339 [inline]
netlink_unicast+0x539/0x800 net/netlink/af_netlink.c:1365
netlink_sendmsg+0x93c/0xe30 net/netlink/af_netlink.c:1914
sock_sendmsg_nosec net/socket.c:725 [inline]
sock_sendmsg+0xd9/0x180 net/socket.c:748
__sys_sendto+0x255/0x340 net/socket.c:2134
page last free stack trace:
reset_page_owner include/linux/page_owner.h:24 [inline]
free_pages_prepare mm/page_alloc.c:1161 [inline]
free_unref_page_prepare+0x508/0xb90 mm/page_alloc.c:2348
free_unref_page+0x33/0x3b0 mm/page_alloc.c:2443
kvfree+0x47/0x50 mm/util.c:650
device_release+0xa1/0x240 drivers/base/core.c:2484
kobject_cleanup lib/kobject.c:682 [inline]
kobject_release lib/kobject.c:713 [inline]
kref_put include/linux/kref.h:65 [inline]
kobject_put+0x1f7/0x5b0 lib/kobject.c:730
netdev_run_todo+0x7dd/0x11d0 net/core/dev.c:10366
rtnl_unlock net/core/rtnetlink.c:151 [inline]
rtnetlink_rcv_msg+0x446/0xd30 net/core/rtnetlink.c:6429
netlink_rcv_skb+0x16b/0x440 net/netlink/af_netlink.c:2549
netlink_unicast_kernel net/netlink/af_netlink.c:1339 [inline]
netlink_unicast+0x539/0x800 net/netlink/af_netlink.c:1365
netlink_sendmsg+0x93c/0xe30 net/netlink/af_netlink.c:1914
sock_sendmsg_nosec net/socket.c:725 [inline]
sock_sendmsg+0xd9/0x180 net/socket.c:748
____sys_sendmsg+0x6ac/0x940 net/socket.c:2494
___sys_sendmsg+0x135/0x1d0 net/socket.c:2548
__sys_sendmsg+0x117/0x1e0 net/socket.c:2577
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

Memory state around the buggy address:
ffff88802b9b8500: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802b9b8580: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>ffff88802b9b8600: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
^
ffff88802b9b8680: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802b9b8700: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
==================================================================

Powered by blists - more mailing lists