[PATCH] net: tun: Fix use-after-free in tun_detach()

8 views
Skip to first unread message

Shigeru Yoshida

unread,
Nov 19, 2022, 2:56:25 AM11/19/22
to da...@davemloft.net, edum...@google.com, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, Shigeru Yoshida
syzbot reported use-after-free in tun_detach() [1]. This causes call
trace like below:

==================================================================
BUG: KASAN: use-after-free in notifier_call_chain+0x1da/0x1e0
...
Call Trace:
<TASK>
dump_stack_lvl+0x100/0x178
print_report+0x167/0x470
? __virt_addr_valid+0x5e/0x2d0
? __phys_addr+0xc6/0x140
? notifier_call_chain+0x1da/0x1e0
? notifier_call_chain+0x1da/0x1e0
kasan_report+0xbf/0x1e0
? notifier_call_chain+0x1da/0x1e0
notifier_call_chain+0x1da/0x1e0
call_netdevice_notifiers_info+0x83/0x130
netdev_run_todo+0xc33/0x11b0
? generic_xdp_install+0x490/0x490
? __tun_detach+0x1500/0x1500
tun_chr_close+0xe2/0x190
__fput+0x26a/0xa40
task_work_run+0x14d/0x240
? task_work_cancel+0x30/0x30
do_exit+0xb31/0x2a40
? reacquire_held_locks+0x4a0/0x4a0
? do_raw_spin_lock+0x12e/0x2b0
? mm_update_next_owner+0x7c0/0x7c0
? rwlock_bug.part.0+0x90/0x90
? lockdep_hardirqs_on_prepare+0x17f/0x410
do_group_exit+0xd4/0x2a0
__x64_sys_exit_group+0x3e/0x50
do_syscall_64+0x38/0xb0
entry_SYSCALL_64_after_hwframe+0x63/0xcd

The cause of the issue is that sock_put() from __tun_detach() drops
last reference count for struct net, and then notifier_call_chain()
from netdev_state_change() accesses that struct net.

This patch fixes the issue by calling sock_put() from tun_detach()
after all necessary accesses for the struct net has done.

Link: https://syzkaller.appspot.com/bug?id=96eb7f1ce75ef933697f24eeab928c4a716edefe [1]
Signed-off-by: Shigeru Yoshida <syos...@redhat.com>
---
drivers/net/tun.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 7a3ab3427369..ce9fcf4c8ef4 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -686,7 +686,6 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
if (tun)
xdp_rxq_info_unreg(&tfile->xdp_rxq);
ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
- sock_put(&tfile->sk);
}
}

@@ -702,6 +701,11 @@ static void tun_detach(struct tun_file *tfile, bool clean)
if (dev)
netdev_state_change(dev);
rtnl_unlock();
+
+ if (clean) {
+ synchronize_rcu();
+ sock_put(&tfile->sk);
+ }
}

static void tun_detach_all(struct net_device *dev)
--
2.38.1

Eric Dumazet

unread,
Nov 19, 2022, 1:31:51 PM11/19/22
to Shigeru Yoshida, da...@davemloft.net, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com
On Fri, Nov 18, 2022 at 11:56 PM Shigeru Yoshida <syos...@redhat.com> wrote:
>
> syzbot reported use-after-free in tun_detach() [1]. This causes call
> trace like below:
>
> ==================================================================
> BUG: KASAN: use-after-free in notifier_call_chain+0x1da/0x1e0
> ...
> Call Trace:

Please include a symbolic stack trace, I think syzbot has them.
Please add a Fixes: tag, once you identified which commit added this bug.

Shigeru Yoshida

unread,
Nov 20, 2022, 3:38:07 AM11/20/22
to edum...@google.com, da...@davemloft.net, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com
Hi Eric,

On Sat, 19 Nov 2022 10:31:38 -0800, Eric Dumazet wrote:
> On Fri, Nov 18, 2022 at 11:56 PM Shigeru Yoshida <syos...@redhat.com> wrote:
>>
>> syzbot reported use-after-free in tun_detach() [1]. This causes call
>> trace like below:
>>
>> ==================================================================
>> BUG: KASAN: use-after-free in notifier_call_chain+0x1da/0x1e0
>> ...
>> Call Trace:
>
> Please include a symbolic stack trace, I think syzbot has them.

Thank you so much for your comment. I got it.
I got it too. I'll send v2 patch.

Thanks,
Shigeru

Shigeru Yoshida

unread,
Nov 20, 2022, 4:02:34 AM11/20/22
to da...@davemloft.net, edum...@google.com, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, Shigeru Yoshida, syzbot+106f9b...@syzkaller.appspotmail.com
syzbot reported use-after-free in tun_detach() [1]. This causes call
trace like below:

==================================================================
BUG: KASAN: use-after-free in notifier_call_chain+0x1ee/0x200 kernel/notifier.c:75
Read of size 8 at addr ffff88807324e2a8 by task syz-executor.0/3673

CPU: 0 PID: 3673 Comm: syz-executor.0 Not tainted 6.1.0-rc5-syzkaller-00044-gcc675d22e422 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd1/0x138 lib/dump_stack.c:106
print_address_description mm/kasan/report.c:284 [inline]
print_report+0x15e/0x461 mm/kasan/report.c:395
kasan_report+0xbf/0x1f0 mm/kasan/report.c:495
notifier_call_chain+0x1ee/0x200 kernel/notifier.c:75
call_netdevice_notifiers_info+0x86/0x130 net/core/dev.c:1942
call_netdevice_notifiers_extack net/core/dev.c:1983 [inline]
call_netdevice_notifiers net/core/dev.c:1997 [inline]
netdev_wait_allrefs_any net/core/dev.c:10237 [inline]
netdev_run_todo+0xbc6/0x1100 net/core/dev.c:10351
tun_detach drivers/net/tun.c:704 [inline]
tun_chr_close+0xe4/0x190 drivers/net/tun.c:3467
__fput+0x27c/0xa90 fs/file_table.c:320
task_work_run+0x16f/0x270 kernel/task_work.c:179
exit_task_work include/linux/task_work.h:38 [inline]
do_exit+0xb3d/0x2a30 kernel/exit.c:820
do_group_exit+0xd4/0x2a0 kernel/exit.c:950
get_signal+0x21b1/0x2440 kernel/signal.c:2858
arch_do_signal_or_restart+0x86/0x2300 arch/x86/kernel/signal.c:869
exit_to_user_mode_loop kernel/entry/common.c:168 [inline]
exit_to_user_mode_prepare+0x15f/0x250 kernel/entry/common.c:203
__syscall_exit_to_user_mode_work kernel/entry/common.c:285 [inline]
syscall_exit_to_user_mode+0x1d/0x50 kernel/entry/common.c:296
do_syscall_64+0x46/0xb0 arch/x86/entry/common.c:86
entry_SYSCALL_64_after_hwframe+0x63/0xcd

The cause of the issue is that sock_put() from __tun_detach() drops
last reference count for struct net, and then notifier_call_chain()
from netdev_state_change() accesses that struct net.

This patch fixes the issue by calling sock_put() from tun_detach()
after all necessary accesses for the struct net has done.

Fixes: 83c1f36f9880 ("tun: send netlink notification when the device is modified")
Reported-by: syzbot+106f9b...@syzkaller.appspotmail.com
---
v2:
- Include symbolic stack trace
- Add Fixes and Reported-by tags
v1: https://lore.kernel.org/all/20221119075615.7...@redhat.com/

Hillf Danton

unread,
Nov 20, 2022, 5:49:22 AM11/20/22
to Shigeru Yoshida, edum...@google.com, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
On 20 Nov 2022 18:02:13 +0900 Shigeru Yoshida <syos...@redhat.com>
Correct. IOW the race between netdev_run_todo() and cleanup_net() is behind
the uaf report from syzbot.

>
> This patch fixes the issue by calling sock_put() from tun_detach()
> after all necessary accesses for the struct net has done.

Thanks for your fix.

But tun is not special wrt netdev_run_todo() and call_netdevice_notifiers(),
so the correct fix should be making netdev grab another hold on net and
invoking put_net() in the path of netdev_run_todo().

Hillf

Eric Dumazet

unread,
Nov 20, 2022, 11:04:26 AM11/20/22
to Hillf Danton, Shigeru Yoshida, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
Well, this is not going to work. Unless I am missing something.

1) A netns is destroyed when its refcount reaches zero.

if (refcount_dec_and_test(&net->ns.count))
__put_net(net);

This transition is final.

(It is illegal to attempt a refcount_inc() from this state)

2) All its netdevices are unregistered.

Trying to get a reference on netns in netdevice dismantle path
would immediately trigger a refcount_t warning.

Hillf Danton

unread,
Nov 20, 2022, 7:34:18 PM11/20/22
to Eric Dumazet, Shigeru Yoshida, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
On 20 Nov 2022 08:04:13 -0800 Eric Dumazet <edum...@google.com>
> On Sun, Nov 20, 2022 at 2:49 AM Hillf Danton <hda...@sina.com> wrote:
> > On 20 Nov 2022 18:02:13 +0900 Shigeru Yoshida <syos...@redhat.com>
> > >
> > > This patch fixes the issue by calling sock_put() from tun_detach()
> > > after all necessary accesses for the struct net has done.
> >
> > Thanks for your fix.
> >
> > But tun is not special wrt netdev_run_todo() and call_netdevice_notifiers(),
> > so the correct fix should be making netdev grab another hold on net and
> > invoking put_net() in the path of netdev_run_todo().
>
> Well, this is not going to work. Unless I am missing something.

Thanks for taking a look.

I mean bump up refcount for net when updating netdev->nd_net in a bid to
make dev_net() safe throught netdev's life span.

Hillf

Eric Dumazet

unread,
Nov 20, 2022, 7:53:50 PM11/20/22
to Hillf Danton, Shigeru Yoshida, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
This would prevent netns deletion, as the following sequence would
then no longer work as intended.

ip netns add foo
ip netns add ip link set lo up
ip netns del foo

When a netns is deleted ("ip netns del" and no more refcounted sockets),
we have callbacks to unregister all devices tied to it.

Hillf Danton

unread,
Nov 21, 2022, 7:02:09 AM11/21/22
to Eric Dumazet, Shigeru Yoshida, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
On 20 Nov 2022 16:53:38 -0800 Eric Dumazet <edum...@google.com>
IIUC the break can be walked around by putting a minor tweak in
dev_net_set() based on net->passive. Then it can be called through out
the life span of any netdev with &init_net as input, and in the callbacks
above as well.

The bonus is, in parallel to socket, netdev can pin net.

+++ b/include/linux/netdevice.h
@@ -2513,6 +2513,14 @@ struct net *dev_net(const struct net_dev
static inline
void dev_net_set(struct net_device *dev, struct net *net)
{
+ struct net *old = dev_net(dev);
+
+ if (old && old != &init_net)
+ net_free(old);
+
+ if (net && net != &init_net)
+ refcount_inc(&net->passive);
+
write_pnet(&dev->nd_net, net);
}

Eric Dumazet

unread,
Nov 21, 2022, 11:47:30 AM11/21/22
to Shigeru Yoshida, da...@davemloft.net, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
Would you mind explaining (a comment would be nice) why this barrier is needed ?

Thanks.

Shigeru Yoshida

unread,
Nov 22, 2022, 1:10:14 PM11/22/22
to edum...@google.com, da...@davemloft.net, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
Hi Eric,
I thought that tfile is accessed with rcu_lock(), so I put
synchronize_rcu() here. Please let me know if I misunderstand the
concept of rcu (I'm losing my confidence...).

Thanks,
Shigeru

Eric Dumazet

unread,
Nov 22, 2022, 1:47:13 PM11/22/22
to Shigeru Yoshida, Jason Wang, da...@davemloft.net, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
Addin Jason for comments.

If an RCU grace period was needed before commit 83c1f36f9880 ("tun:
send netlink notification when the device is modified"),
would we need another patch ?

Also sock_flag(sk, SOCK_RCU_FREE) would probably be better than adding
a synchronize_rcu() (if again a grace period is needed)

Jason Wang

unread,
Nov 22, 2022, 11:20:59 PM11/22/22
to Eric Dumazet, Shigeru Yoshida, da...@davemloft.net, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
I think we don't need another synchronization here. __tun_detach() has
already done the necessary synchronization when it tries to modify
tun->tfiles array and tfile->tun.

Thanks

Shigeru Yoshida

unread,
Nov 23, 2022, 11:08:42 AM11/23/22
to jaso...@redhat.com, edum...@google.com, da...@davemloft.net, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, syzbot+106f9b...@syzkaller.appspotmail.com
Hi Jason,
Thank you so much for your comment. I'll prepare v3 patch to remove
calling synchronize_rcu().

Thanks,
Shigeru

Shigeru Yoshida

unread,
Nov 24, 2022, 12:51:54 PM11/24/22
to da...@davemloft.net, edum...@google.com, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, jaso...@redhat.com, Shigeru Yoshida, syzbot+106f9b...@syzkaller.appspotmail.com
v3:
- Remove redundant synchronize_rcu()
v2:
- Include symbolic stack trace
- Add Fixes and Reported-by tags
v1: https://lore.kernel.org/all/20221119075615.7...@redhat.com/
---
drivers/net/tun.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 7a3ab3427369..24001112c323 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -686,7 +686,6 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
if (tun)
xdp_rxq_info_unreg(&tfile->xdp_rxq);
ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
- sock_put(&tfile->sk);
}
}

@@ -702,6 +701,9 @@ static void tun_detach(struct tun_file *tfile, bool clean)
if (dev)
netdev_state_change(dev);
rtnl_unlock();
+
+ if (clean)
+ sock_put(&tfile->sk);

Hillf Danton

unread,
Nov 24, 2022, 11:39:12 PM11/24/22
to Shigeru Yoshida, edum...@google.com, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, jaso...@redhat.com, syzbot+106f9b...@syzkaller.appspotmail.com
On 25 Nov 2022 02:51:34 +0900 Shigeru Yoshida <syos...@redhat.com>
If this is a correct fix, could you specify the reasons why
call_netdevice_notifiers() is not independent of socket, given netdev,
sock and netns are different things, and sock can pin netns?

Hillf

patchwork-b...@kernel.org

unread,
Nov 29, 2022, 6:30:16 AM11/29/22
to Shigeru Yoshida, da...@davemloft.net, edum...@google.com, ku...@kernel.org, pab...@redhat.com, net...@vger.kernel.org, linux-...@vger.kernel.org, syzkall...@googlegroups.com, jaso...@redhat.com, syzbot+106f9b...@syzkaller.appspotmail.com
Hello:

This patch was applied to netdev/net.git (master)
by Paolo Abeni <pab...@redhat.com>:

On Fri, 25 Nov 2022 02:51:34 +0900 you wrote:
> syzbot reported use-after-free in tun_detach() [1]. This causes call
> trace like below:
>
> ==================================================================
> BUG: KASAN: use-after-free in notifier_call_chain+0x1ee/0x200 kernel/notifier.c:75
> Read of size 8 at addr ffff88807324e2a8 by task syz-executor.0/3673
>
> [...]

Here is the summary with links:
- [v3] net: tun: Fix use-after-free in tun_detach()
https://git.kernel.org/netdev/net/c/5daadc86f27e

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


Reply all
Reply to author
Forward
0 new messages