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

  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+106f9b687cd64ee70cd1@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=96eb7f1ce75ef933697f24eeab928c4a716edefe [1]
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
v2:
- Include symbolic stack trace
- Add Fixes and Reported-by tags
v1: https://lore.kernel.org/all/20221119075615.723290-1-syoshida@redhat.com/
---
 drivers/net/tun.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
  

On Sun, Nov 20, 2022 at 2:49 AM Hillf Danton <hdanton@sina.com> wrote:
>
> On 20 Nov 2022 18:02:13 +0900 Shigeru Yoshida <syoshida@redhat.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.
>
> 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().

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.
  

On Sun, Nov 20, 2022 at 4:34 PM Hillf Danton <hdanton@sina.com> wrote:
>
> On 20 Nov 2022 08:04:13 -0800 Eric Dumazet <edumazet@google.com>
> > On Sun, Nov 20, 2022 at 2:49 AM Hillf Danton <hdanton@sina.com> wrote:
> > > On 20 Nov 2022 18:02:13 +0900 Shigeru Yoshida <syoshida@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.

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.
  

On Sun, Nov 20, 2022 at 1:02 AM Shigeru Yoshida <syoshida@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+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+106f9b687cd64ee70cd1@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=96eb7f1ce75ef933697f24eeab928c4a716edefe [1]
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> ---
> v2:
> - Include symbolic stack trace
> - Add Fixes and Reported-by tags
> v1: https://lore.kernel.org/all/20221119075615.723290-1-syoshida@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) {

Would you mind explaining (a comment would be nice) why this barrier is needed ?

Thanks.

> +               synchronize_rcu();
> +               sock_put(&tfile->sk);
> +       }
>  }
>
>  static void tun_detach_all(struct net_device *dev)
> --
> 2.38.1
>
  

Hi Eric,

On Mon, 21 Nov 2022 08:47:17 -0800, Eric Dumazet wrote:
> On Sun, Nov 20, 2022 at 1:02 AM Shigeru Yoshida <syoshida@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+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+106f9b687cd64ee70cd1@syzkaller.appspotmail.com
>> Link: https://syzkaller.appspot.com/bug?id=96eb7f1ce75ef933697f24eeab928c4a716edefe [1]
>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>> ---
>> v2:
>> - Include symbolic stack trace
>> - Add Fixes and Reported-by tags
>> v1: https://lore.kernel.org/all/20221119075615.723290-1-syoshida@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) {
> 
> Would you mind explaining (a comment would be nice) why this barrier is needed ?

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

> 
> Thanks.
> 
>> +               synchronize_rcu();
>> +               sock_put(&tfile->sk);
>> +       }
>>  }
>>
>>  static void tun_detach_all(struct net_device *dev)
>> --
>> 2.38.1
>>
>
  

On Tue, Nov 22, 2022 at 10:10 AM Shigeru Yoshida <syoshida@redhat.com> wrote:
>
> Hi Eric,
>
> On Mon, 21 Nov 2022 08:47:17 -0800, Eric Dumazet wrote:
> > On Sun, Nov 20, 2022 at 1:02 AM Shigeru Yoshida <syoshida@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+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+106f9b687cd64ee70cd1@syzkaller.appspotmail.com
> >> Link: https://syzkaller.appspot.com/bug?id=96eb7f1ce75ef933697f24eeab928c4a716edefe [1]
> >> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> >> ---
> >> v2:
> >> - Include symbolic stack trace
> >> - Add Fixes and Reported-by tags
> >> v1: https://lore.kernel.org/all/20221119075615.723290-1-syoshida@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) {
> >
> > Would you mind explaining (a comment would be nice) why this barrier is needed ?
>
> 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...).
>

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)



> Thanks,
> Shigeru
>
> >
> > Thanks.
> >
> >> +               synchronize_rcu();
> >> +               sock_put(&tfile->sk);
> >> +       }
> >>  }
> >>
> >>  static void tun_detach_all(struct net_device *dev)
> >> --
> >> 2.38.1
> >>
> >
>
  

在 2022/11/23 02:47, Eric Dumazet 写道:
> On Tue, Nov 22, 2022 at 10:10 AM Shigeru Yoshida <syoshida@redhat.com> wrote:
>> Hi Eric,
>>
>> On Mon, 21 Nov 2022 08:47:17 -0800, Eric Dumazet wrote:
>>> On Sun, Nov 20, 2022 at 1:02 AM Shigeru Yoshida <syoshida@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+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+106f9b687cd64ee70cd1@syzkaller.appspotmail.com
>>>> Link: https://syzkaller.appspot.com/bug?id=96eb7f1ce75ef933697f24eeab928c4a716edefe [1]
>>>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>>>> ---
>>>> v2:
>>>> - Include symbolic stack trace
>>>> - Add Fixes and Reported-by tags
>>>> v1: https://lore.kernel.org/all/20221119075615.723290-1-syoshida@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) {
>>> Would you mind explaining (a comment would be nice) why this barrier is needed ?
>> 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...).
>>
> 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 ?


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


>
> Also sock_flag(sk, SOCK_RCU_FREE) would probably be better than adding
> a synchronize_rcu() (if again a grace period is needed)
>
>
>
>> Thanks,
>> Shigeru
>>
>>> Thanks.
>>>
>>>> +               synchronize_rcu();
>>>> +               sock_put(&tfile->sk);
>>>> +       }
>>>>   }
>>>>
>>>>   static void tun_detach_all(struct net_device *dev)
>>>> --
>>>> 2.38.1
>>>>
  

Hi Jason,

On Wed, 23 Nov 2022 12:20:47 +0800, Jason Wang wrote:
> 
> 在 2022/11/23 02:47, Eric Dumazet 写道:
>> On Tue, Nov 22, 2022 at 10:10 AM Shigeru Yoshida <syoshida@redhat.com>
>> wrote:
>>> Hi Eric,
>>>
>>> On Mon, 21 Nov 2022 08:47:17 -0800, Eric Dumazet wrote:
>>>> On Sun, Nov 20, 2022 at 1:02 AM Shigeru Yoshida <syoshida@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+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+106f9b687cd64ee70cd1@syzkaller.appspotmail.com
>>>>> Link:
>>>>> https://syzkaller.appspot.com/bug?id=96eb7f1ce75ef933697f24eeab928c4a716edefe
>>>>> [1]
>>>>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>>>>> ---
>>>>> v2:
>>>>> - Include symbolic stack trace
>>>>> - Add Fixes and Reported-by tags
>>>>> v1:
>>>>> https://lore.kernel.org/all/20221119075615.723290-1-syoshida@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) {
>>>> Would you mind explaining (a comment would be nice) why this barrier
>>>> is needed ?
>>> 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...).
>>>
>> 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 ?
> 
> 
> 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.

Thank you so much for your comment.  I'll prepare v3 patch to remove
calling synchronize_rcu().

Thanks,
Shigeru

> Thanks
> 
> 
>>
>> Also sock_flag(sk, SOCK_RCU_FREE) would probably be better than adding
>> a synchronize_rcu() (if again a grace period is needed)
>>
>>
>>
>>> Thanks,
>>> Shigeru
>>>
>>>> Thanks.
>>>>
>>>>> +               synchronize_rcu();
>>>>> +               sock_put(&tfile->sk);
>>>>> +       }
>>>>>   }
>>>>>
>>>>>   static void tun_detach_all(struct net_device *dev)
>>>>> --
>>>>> 2.38.1
>>>>>
>