[PATCH net] net/packet: fix a race in packet_bind() and packet_notifier()

[PATCH net] net/packet: fix a race in packet_bind() and packet_notifier()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

syzbot reported crashes [1] and provided a C repro easing bug hunting.

When/if packet_do_bind() calls __unregister_prot_hook() and releases
po->bind_lock, another thread can run packet_notifier() and process an
NETDEV_UP event.

This calls register_prot_hook() and hook again the socket right before
first thread was able to grab again po->bind_lock.

Fixes this issue by adding po->frozen bit :

It is set and cleared by __unregister_prot_hook() if po->bind_lock
needs to be released temporarily.

It is tested in register_prot_hook() to prevent the race condition.

[1]
dev_remove_pack: ffff8801bf16fa80 not found
------------[ cut here ]------------
kernel BUG at net/core/dev.c:7945!  ( BUG_ON(!list_empty(&dev->ptype_all)); )
invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
device syz0 entered promiscuous mode
CPU: 0 PID: 3161 Comm: syzkaller404108 Not tainted 4.14.0+ #190
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
task: ffff8801cc57a500 task.stack: ffff8801cc588000
RIP: 0010:netdev_run_todo+0x772/0xae0 net/core/dev.c:7945
RSP: 0018:ffff8801cc58f598 EFLAGS: 00010293
RAX: ffff8801cc57a500 RBX: dffffc0000000000 RCX: ffffffff841f75b2
RDX: 0000000000000000 RSI: 1ffff100398b1ede RDI: ffff8801bf1f8810
device syz0 entered promiscuous mode
RBP: ffff8801cc58f898 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801bf1f8cd8
R13: ffff8801cc58f870 R14: ffff8801bf1f8780 R15: ffff8801cc58f7f0
FS:  0000000001716880(0000) GS:ffff8801db400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020b13000 CR3: 0000000005e25000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 rtnl_unlock+0xe/0x10 net/core/rtnetlink.c:106
 tun_detach drivers/net/tun.c:670 [inline]
 tun_chr_close+0x49/0x60 drivers/net/tun.c:2845
 __fput+0x333/0x7f0 fs/file_table.c:210
 ____fput+0x15/0x20 fs/file_table.c:244
 task_work_run+0x199/0x270 kernel/task_work.c:113
 exit_task_work include/linux/task_work.h:22 [inline]
 do_exit+0x9bb/0x1ae0 kernel/exit.c:865
 do_group_exit+0x149/0x400 kernel/exit.c:968
 SYSC_exit_group kernel/exit.c:979 [inline]
 SyS_exit_group+0x1d/0x20 kernel/exit.c:977
 entry_SYSCALL_64_fastpath+0x1f/0x96
RIP: 0033:0x44ad19


Fixes: 30f7ea1c2b5f ("packet: race condition in packet_bind")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Francesco Ruggeri <fruggeri@aristanetworks.com>
---
 net/packet/af_packet.c |    5 ++++-
 net/packet/internal.h  |    1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 737092ca9b4eed464b6c0907d85b679ae4da6046..64382200ab9b0701a510f16a098257ccd7ac5ff5 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -336,7 +336,7 @@ static void register_prot_hook(struct sock *sk)
 {
 	struct packet_sock *po = pkt_sk(sk);
 
-	if (!po->running) {
+	if (!po->running && !po->frozen) {
 		if (po->fanout)
 			__fanout_link(sk, po);
 		else
@@ -368,9 +368,11 @@ static void __unregister_prot_hook(struct sock *sk, bool sync)
 	__sock_put(sk);
 
 	if (sync) {
+		po->frozen = 1;
 		spin_unlock(&po->bind_lock);
 		synchronize_net();
 		spin_lock(&po->bind_lock);
+		po->frozen = 0;
 	}
 }
 
@@ -3105,6 +3107,7 @@ static int packet_do_bind(struct sock *sk, const char *name, int ifindex,
 								 dev->ifindex);
 		}
 
+		BUG_ON(po->running);
 		po->num = proto;
 		po->prot_hook.type = proto;
 
diff --git a/net/packet/internal.h b/net/packet/internal.h
index 562fbc155006374862e5bfdd78b65a7f46210bea..e039af6e71d650e3beb2936af6cbdd167313499e 100644
--- a/net/packet/internal.h
+++ b/net/packet/internal.h
@@ -114,6 +114,7 @@ struct packet_sock {
 	spinlock_t		bind_lock;
 	struct mutex		pg_vec_lock;
 	unsigned int		running:1,	/* prot_hook is attached*/
+				frozen:1,
 				auxdata:1,
 				origdev:1,
 				has_vnet_hdr:1;

Re: [PATCH net] net/packet: fix a race in packet_bind() and packet_notifier()

[Francesco Ruggeri]

On Mon, Nov 27, 2017 at 8:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> syzbot reported crashes [1] and provided a C repro easing bug hunting.
>
> When/if packet_do_bind() calls __unregister_prot_hook() and releases
> po->bind_lock, another thread can run packet_notifier() and process an
> NETDEV_UP event.
>
> This calls register_prot_hook() and hook again the socket right before
> first thread was able to grab again po->bind_lock.
>
> Fixes this issue by adding po->frozen bit :
>
> It is set and cleared by __unregister_prot_hook() if po->bind_lock
> needs to be released temporarily.
>
> It is tested in register_prot_hook() to prevent the race condition.
>
> [1]
> dev_remove_pack: ffff8801bf16fa80 not found
> ------------[ cut here ]------------
> kernel BUG at net/core/dev.c:7945!  ( BUG_ON(!list_empty(&dev->ptype_all)); )
> invalid opcode: 0000 [#1] SMP KASAN
> Dumping ftrace buffer:
>    (ftrace buffer empty)
> Modules linked in:
> device syz0 entered promiscuous mode
> CPU: 0 PID: 3161 Comm: syzkaller404108 Not tainted 4.14.0+ #190
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> task: ffff8801cc57a500 task.stack: ffff8801cc588000
> RIP: 0010:netdev_run_todo+0x772/0xae0 net/core/dev.c:7945
> RSP: 0018:ffff8801cc58f598 EFLAGS: 00010293
> RAX: ffff8801cc57a500 RBX: dffffc0000000000 RCX: ffffffff841f75b2
> RDX: 0000000000000000 RSI: 1ffff100398b1ede RDI: ffff8801bf1f8810
> device syz0 entered promiscuous mode
> RBP: ffff8801cc58f898 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801bf1f8cd8
> R13: ffff8801cc58f870 R14: ffff8801bf1f8780 R15: ffff8801cc58f7f0
> FS:  0000000001716880(0000) GS:ffff8801db400000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000020b13000 CR3: 0000000005e25000 CR4: 00000000001406f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  rtnl_unlock+0xe/0x10 net/core/rtnetlink.c:106
>  tun_detach drivers/net/tun.c:670 [inline]
>  tun_chr_close+0x49/0x60 drivers/net/tun.c:2845
>  __fput+0x333/0x7f0 fs/file_table.c:210
>  ____fput+0x15/0x20 fs/file_table.c:244
>  task_work_run+0x199/0x270 kernel/task_work.c:113
>  exit_task_work include/linux/task_work.h:22 [inline]
>  do_exit+0x9bb/0x1ae0 kernel/exit.c:865
>  do_group_exit+0x149/0x400 kernel/exit.c:968
>  SYSC_exit_group kernel/exit.c:979 [inline]
>  SyS_exit_group+0x1d/0x20 kernel/exit.c:977
>  entry_SYSCALL_64_fastpath+0x1f/0x96
> RIP: 0033:0x44ad19
>
>
> Fixes: 30f7ea1c2b5f ("packet: race condition in packet_bind")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Francesco Ruggeri <fruggeri@aristanetworks.com>
> ---
>  net/packet/af_packet.c |    5 ++++-
>  net/packet/internal.h  |    1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 737092ca9b4eed464b6c0907d85b679ae4da6046..64382200ab9b0701a510f16a098257ccd7ac5ff5 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -336,7 +336,7 @@ static void register_prot_hook(struct sock *sk)
>  {
>         struct packet_sock *po = pkt_sk(sk);
>
> -       if (!po->running) {
> +       if (!po->running && !po->frozen) {

Would it make sense to move the check for po->frozen to
packet_notifier(NETDEV_UP)?
As far as I can tell that is the only case today that can cause this
race condition, and if new cases come up in the future an error code
may be required rather than silently turning register_prot_hook() into
a noop.
Otherwise it looks fine to me.

Thanks,

Acked-by: Francesco Ruggeri <fruggeri@arista.com>

>                 if (po->fanout)
>                         __fanout_link(sk, po);
>                 else
> @@ -368,9 +368,11 @@ static void __unregister_prot_hook(struct sock *sk, bool sync)
>         __sock_put(sk);
>
>         if (sync) {
> +               po->frozen = 1;
>                 spin_unlock(&po->bind_lock);
>                 synchronize_net();
>                 spin_lock(&po->bind_lock);
> +               po->frozen = 0;
>         }
>  }
>
> @@ -3105,6 +3107,7 @@ static int packet_do_bind(struct sock *sk, const char *name, int ifindex,
>                                                                  dev->ifindex);
>                 }
>
> +               BUG_ON(po->running);
>                 po->num = proto;
>                 po->prot_hook.type = proto;
>
> diff --git a/net/packet/internal.h b/net/packet/internal.h
> index 562fbc155006374862e5bfdd78b65a7f46210bea..e039af6e71d650e3beb2936af6cbdd167313499e 100644
> --- a/net/packet/internal.h
> +++ b/net/packet/internal.h
> @@ -114,6 +114,7 @@ struct packet_sock {
>         spinlock_t              bind_lock;
>         struct mutex            pg_vec_lock;
>         unsigned int            running:1,      /* prot_hook is attached*/
> +                               frozen:1,
>                                 auxdata:1,
>                                 origdev:1,
>                                 has_vnet_hdr:1;

Re: [PATCH net] net/packet: fix a race in packet_bind() and packet_notifier()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 27 Nov 2017 20:00:52 -0800

> @@ -368,9 +368,11 @@ static void __unregister_prot_hook(struct sock *sk, bool sync)
>  	__sock_put(sk);
>  
>  	if (sync) {
> +		po->frozen = 1;
>  		spin_unlock(&po->bind_lock);
>  		synchronize_net();
>  		spin_lock(&po->bind_lock);
> +		po->frozen = 0;
>  	}
>  }
>  

Ugh.

Maybe you can just set po->num to zero in the bind code path which causes
this problem.  That will prevent this situation entirely.

Re: [PATCH net] net/packet: fix a race in packet_bind() and packet_notifier()

[Eric Dumazet]

On Tue, 2017-11-28 at 02:23 -0800, Francesco Ruggeri wrote:
> On Mon, Nov 27, 2017 at 8:00 PM, Eric Dumazet <eric.dumazet@gmail.com
> > wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > 
...
> > +++ b/net/packet/af_packet.c
> > @@ -336,7 +336,7 @@ static void register_prot_hook(struct sock *sk)
> >  {
> >         struct packet_sock *po = pkt_sk(sk);
> > 
> > -       if (!po->running) {
> > +       if (!po->running && !po->frozen) {
> 
> Would it make sense to move the check for po->frozen to
> packet_notifier(NETDEV_UP)?
> As far as I can tell that is the only case today that can cause this
> race condition, and if new cases come up in the future an error code
> may be required rather than silently turning register_prot_hook()
> into
> a noop.
> Otherwise it looks fine to me.
> 

Whatever works for me is fine, I have no strong opinion on this.

Note that frozen is only set in the case we know that
register_prot_hook() is going to be called by us.

Re: [PATCH net] net/packet: fix a race in packet_bind() and packet_notifier()

[Eric Dumazet]

On Tue, 2017-11-28 at 09:48 -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 27 Nov 2017 20:00:52 -0800
> 
> > @@ -368,9 +368,11 @@ static void __unregister_prot_hook(struct sock
> *sk, bool sync)
> >       __sock_put(sk);
> >  
> >       if (sync) {
> > +             po->frozen = 1;
> >               spin_unlock(&po->bind_lock);
> >               synchronize_net();
> >               spin_lock(&po->bind_lock);
> > +             po->frozen = 0;
> >       }
> >  }
> >  
> 
> Ugh.
> 
> Maybe you can just set po->num to zero in the bind code path which
> causes
> this problem.  That will prevent this situation entirely.

Yes, I can submit a V2 with this idea implemented, thanks.

[PATCH v2 net] net/packet: fix a race in packet_bind() and packet_notifier()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

syzbot reported crashes [1] and provided a C repro easing bug hunting.

When/if packet_do_bind() calls __unregister_prot_hook() and releases
po->bind_lock, another thread can run packet_notifier() and process an
NETDEV_UP event.

This calls register_prot_hook() and hooks again the socket right before
first thread is able to grab again po->bind_lock.

Fixes this issue by temporarily setting po->num to 0, as suggested by
David Miller.

[1]
dev_remove_pack: ffff8801bf16fa80 not found
------------[ cut here ]------------
kernel BUG at net/core/dev.c:7945!  ( BUG_ON(!list_empty(&dev->ptype_all)); )
invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
device syz0 entered promiscuous mode
CPU: 0 PID: 3161 Comm: syzkaller404108 Not tainted 4.14.0+ #190
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
task: ffff8801cc57a500 task.stack: ffff8801cc588000
RIP: 0010:netdev_run_todo+0x772/0xae0 net/core/dev.c:7945
RSP: 0018:ffff8801cc58f598 EFLAGS: 00010293
RAX: ffff8801cc57a500 RBX: dffffc0000000000 RCX: ffffffff841f75b2
RDX: 0000000000000000 RSI: 1ffff100398b1ede RDI: ffff8801bf1f8810
device syz0 entered promiscuous mode
RBP: ffff8801cc58f898 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801bf1f8cd8
R13: ffff8801cc58f870 R14: ffff8801bf1f8780 R15: ffff8801cc58f7f0
FS:  0000000001716880(0000) GS:ffff8801db400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020b13000 CR3: 0000000005e25000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 rtnl_unlock+0xe/0x10 net/core/rtnetlink.c:106
 tun_detach drivers/net/tun.c:670 [inline]
 tun_chr_close+0x49/0x60 drivers/net/tun.c:2845
 __fput+0x333/0x7f0 fs/file_table.c:210
 ____fput+0x15/0x20 fs/file_table.c:244
 task_work_run+0x199/0x270 kernel/task_work.c:113
 exit_task_work include/linux/task_work.h:22 [inline]
 do_exit+0x9bb/0x1ae0 kernel/exit.c:865
 do_group_exit+0x149/0x400 kernel/exit.c:968
 SYSC_exit_group kernel/exit.c:979 [inline]
 SyS_exit_group+0x1d/0x20 kernel/exit.c:977
 entry_SYSCALL_64_fastpath+0x1f/0x96
RIP: 0033:0x44ad19


Fixes: 30f7ea1c2b5f ("packet: race condition in packet_bind")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Francesco Ruggeri <fruggeri@aristanetworks.com>
---
 net/packet/af_packet.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 737092ca9b4eed464b6c0907d85b679ae4da6046..cd454ca583d93841dcad4e1d83a660f15731c5fb 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3097,6 +3097,10 @@ static int packet_do_bind(struct sock *sk, const char *name, int ifindex,
 	if (need_rehook) {
 		if (po->running) {
 			rcu_read_unlock();
+			/* prevents packet_notifier() from calling
+			 * register_prot_hook()
+			 */
+			po->num = 0;
 			__unregister_prot_hook(sk, true);
 			rcu_read_lock();
 			dev_curr = po->prot_hook.dev;
@@ -3105,6 +3109,7 @@ static int packet_do_bind(struct sock *sk, const char *name, int ifindex,
 								 dev->ifindex);
 		}
 
+		BUG_ON(po->running);
 		po->num = proto;
 		po->prot_hook.type = proto;
 

Re: [PATCH v2 net] net/packet: fix a race in packet_bind() and packet_notifier()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 28 Nov 2017 08:03:30 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> syzbot reported crashes [1] and provided a C repro easing bug hunting.
> 
> When/if packet_do_bind() calls __unregister_prot_hook() and releases
> po->bind_lock, another thread can run packet_notifier() and process an
> NETDEV_UP event.
> 
> This calls register_prot_hook() and hooks again the socket right before
> first thread is able to grab again po->bind_lock.
> 
> Fixes this issue by temporarily setting po->num to 0, as suggested by
> David Miller.
> 
> [1]
 ...
> Fixes: 30f7ea1c2b5f ("packet: race condition in packet_bind")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Francesco Ruggeri <fruggeri@aristanetworks.com>

Applied and queued up for -stable.