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

From: Eric Dumazet <[email protected]>

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 <[email protected]>

Reported-by syzbot <[email protected]>

Cc Francesco Ruggeri <[email protected]>

---
 net/packet/af_packet.c |    5 ++++-
 net/packet/internal.h  |    1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

net/packet/af_packet.c +0 -0

index 737092ca9b4eed464b6c0907d85b679ae4da6046..64382200ab9b0701a510f16a098257ccd7ac5ff5 100644

net/packet/af_packet.c +3 -0

                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;
 

net/packet/internal.h +1 -0

index 562fbc155006374862e5bfdd78b65a7f46210bea..e039af6e71d650e3beb2936af6cbdd167313499e 100644
@@ -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;

On Mon, Nov 27, 2017 at 8:00 PM, Eric Dumazet <[email protected]

Quoted 4 lines

wrote:
From: Eric Dumazet <[email protected]>

Quoted 5 lines

+++ b/net/packet/af_packet.c

net/packet/af_packet.c +0 -0

@@ -336,7 +336,7 @@ static void register_prot_hook(struct sock *sk)
  {
         struct packet_sock *po = pkt_sk(sk);
 

net/packet/af_packet.c static void register_prot_hook(struct sock *sk) · line 339

Quoted 2 lines

net/packet/af_packet.c +1 -1

-       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.

From: Eric Dumazet <[email protected]>
Date: Mon, 27 Nov 2017 20:00:52 -0800

Review static void __unregister_prot_hook(struct sock

Quoted 1 line

(quoted diff) +0 -0

@@ -368,9 +368,11 @@ static void __unregister_prot_hook(struct sock

*sk, bool sync)

Quoted 11 lines

net/packet/af_packet.c +0 -0

        __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.

From: Eric Dumazet <[email protected]>

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 <[email protected]>

Reported-by syzbot <[email protected]>

Cc Francesco Ruggeri <[email protected]>