diff options
Diffstat (limited to 'target/linux/generic/backport-5.4/080-wireguard-0114-wireguard-noise-take-lock-when-removing-handshake-en.patch')
| -rw-r--r-- | target/linux/generic/backport-5.4/080-wireguard-0114-wireguard-noise-take-lock-when-removing-handshake-en.patch | 127 |
1 files changed, 127 insertions, 0 deletions
diff --git a/target/linux/generic/backport-5.4/080-wireguard-0114-wireguard-noise-take-lock-when-removing-handshake-en.patch b/target/linux/generic/backport-5.4/080-wireguard-0114-wireguard-noise-take-lock-when-removing-handshake-en.patch new file mode 100644 index 0000000000..e80528c91b --- /dev/null +++ b/target/linux/generic/backport-5.4/080-wireguard-0114-wireguard-noise-take-lock-when-removing-handshake-en.patch @@ -0,0 +1,127 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: "Jason A. Donenfeld" <Jason@zx2c4.com> +Date: Wed, 9 Sep 2020 13:58:14 +0200 +Subject: [PATCH] wireguard: noise: take lock when removing handshake entry + from table + +commit 9179ba31367bcf481c3c79b5f028c94faad9f30a upstream. + +Eric reported that syzkaller found a race of this variety: + +CPU 1 CPU 2 +-------------------------------------------|--------------------------------------- +wg_index_hashtable_replace(old, ...) | + if (hlist_unhashed(&old->index_hash)) | + | wg_index_hashtable_remove(old) + | hlist_del_init_rcu(&old->index_hash) + | old->index_hash.pprev = NULL + hlist_replace_rcu(&old->index_hash, ...) | + *old->index_hash.pprev | + +Syzbot wasn't actually able to reproduce this more than once or create a +reproducer, because the race window between checking "hlist_unhashed" and +calling "hlist_replace_rcu" is just so small. Adding an mdelay(5) or +similar there helps make this demonstrable using this simple script: + + #!/bin/bash + set -ex + trap 'kill $pid1; kill $pid2; ip link del wg0; ip link del wg1' EXIT + ip link add wg0 type wireguard + ip link add wg1 type wireguard + wg set wg0 private-key <(wg genkey) listen-port 9999 + wg set wg1 private-key <(wg genkey) peer $(wg show wg0 public-key) endpoint 127.0.0.1:9999 persistent-keepalive 1 + wg set wg0 peer $(wg show wg1 public-key) + ip link set wg0 up + yes link set wg1 up | ip -force -batch - & + pid1=$! + yes link set wg1 down | ip -force -batch - & + pid2=$! + wait + +The fundumental underlying problem is that we permit calls to wg_index_ +hashtable_remove(handshake.entry) without requiring the caller to take +the handshake mutex that is intended to protect members of handshake +during mutations. This is consistently the case with calls to wg_index_ +hashtable_insert(handshake.entry) and wg_index_hashtable_replace( +handshake.entry), but it's missing from a pertinent callsite of wg_ +index_hashtable_remove(handshake.entry). So, this patch makes sure that +mutex is taken. + +The original code was a little bit funky though, in the form of: + + remove(handshake.entry) + lock(), memzero(handshake.some_members), unlock() + remove(handshake.entry) + +The original intention of that double removal pattern outside the lock +appears to be some attempt to prevent insertions that might happen while +locks are dropped during expensive crypto operations, but actually, all +callers of wg_index_hashtable_insert(handshake.entry) take the write +lock and then explicitly check handshake.state, as they should, which +the aforementioned memzero clears, which means an insertion should +already be impossible. And regardless, the original intention was +necessarily racy, since it wasn't guaranteed that something else would +run after the unlock() instead of after the remove(). So, from a +soundness perspective, it seems positive to remove what looks like a +hack at best. + +The crash from both syzbot and from the script above is as follows: + + general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN + KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] + CPU: 0 PID: 7395 Comm: kworker/0:3 Not tainted 5.9.0-rc4-syzkaller #0 + Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 + Workqueue: wg-kex-wg1 wg_packet_handshake_receive_worker + RIP: 0010:hlist_replace_rcu include/linux/rculist.h:505 [inline] + RIP: 0010:wg_index_hashtable_replace+0x176/0x330 drivers/net/wireguard/peerlookup.c:174 + Code: 00 fc ff df 48 89 f9 48 c1 e9 03 80 3c 01 00 0f 85 44 01 00 00 48 b9 00 00 00 00 00 fc ff df 48 8b 45 10 48 89 c6 48 c1 ee 03 <80> 3c 0e 00 0f 85 06 01 00 00 48 85 d2 4c 89 28 74 47 e8 a3 4f b5 + RSP: 0018:ffffc90006a97bf8 EFLAGS: 00010246 + RAX: 0000000000000000 RBX: ffff888050ffc4f8 RCX: dffffc0000000000 + RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88808e04e010 + RBP: ffff88808e04e000 R08: 0000000000000001 R09: ffff8880543d0000 + R10: ffffed100a87a000 R11: 000000000000016e R12: ffff8880543d0000 + R13: ffff88808e04e008 R14: ffff888050ffc508 R15: ffff888050ffc500 + FS: 0000000000000000(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000 + CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 + CR2: 00000000f5505db0 CR3: 0000000097cf7000 CR4: 00000000001526f0 + DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 + DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 + Call Trace: + wg_noise_handshake_begin_session+0x752/0xc9a drivers/net/wireguard/noise.c:820 + wg_receive_handshake_packet drivers/net/wireguard/receive.c:183 [inline] + wg_packet_handshake_receive_worker+0x33b/0x730 drivers/net/wireguard/receive.c:220 + process_one_work+0x94c/0x1670 kernel/workqueue.c:2269 + worker_thread+0x64c/0x1120 kernel/workqueue.c:2415 + kthread+0x3b5/0x4a0 kernel/kthread.c:292 + ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 + +Reported-by: syzbot <syzkaller@googlegroups.com> +Reported-by: Eric Dumazet <edumazet@google.com> +Link: https://lore.kernel.org/wireguard/20200908145911.4090480-1-edumazet@google.com/ +Fixes: e7096c131e51 ("net: WireGuard secure network tunnel") +Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> +Signed-off-by: David S. Miller <davem@davemloft.net> +Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> +--- + drivers/net/wireguard/noise.c | 5 +---- + 1 file changed, 1 insertion(+), 4 deletions(-) + +--- a/drivers/net/wireguard/noise.c ++++ b/drivers/net/wireguard/noise.c +@@ -87,15 +87,12 @@ static void handshake_zero(struct noise_ + + void wg_noise_handshake_clear(struct noise_handshake *handshake) + { ++ down_write(&handshake->lock); + wg_index_hashtable_remove( + handshake->entry.peer->device->index_hashtable, + &handshake->entry); +- down_write(&handshake->lock); + handshake_zero(handshake); + up_write(&handshake->lock); +- wg_index_hashtable_remove( +- handshake->entry.peer->device->index_hashtable, +- &handshake->entry); + } + + static struct noise_keypair *keypair_create(struct wg_peer *peer) |