[PATCH net] net/packet: fix a race in packet_bind() and packet_notifier()
Subsystems:
networking [general] , packet sockets , the rest
3183d
7 messages,
3 authors,
2017-11-28 · open the first message on its own page
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 <redacted>
Cc: Francesco Ruggeri <redacted>
---
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 ;
On Mon, Nov 27, 2017 at 8:00 PM, Eric Dumazet [off-list ref] wrote: quoted hunk 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 <redacted>
Cc: Francesco Ruggeri <redacted>
---
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 <redacted>
quoted hunk 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 ;
From: Eric Dumazet <redacted>
Date: Mon, 27 Nov 2017 20:00:52 -0800
quoted hunk @@ -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.
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 quoted wrote:
From: Eric Dumazet <edumazet@google.com>
... quoted +++ 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.
On Tue, 2017-11-28 at 09:48 -0500, David Miller wrote: From: Eric Dumazet <redacted>
Date: Mon, 27 Nov 2017 20:00:52 -0800
quoted @@ -368,9 +368,11 @@ static void __unregister_prot_hook(struct sock *sk, bool sync) quoted __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.
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 <redacted>
Cc: Francesco Ruggeri <redacted>
---
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 ;
From: Eric Dumazet <redacted>
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 <redacted>
Cc: Francesco Ruggeri <redacted>
Applied and queued up for -stable.