mirror of
https://github.com/openwrt/openwrt.git
synced 2024-12-29 10:08:59 +00:00
c0cb86e1d5
Rather than using the clunky, old, slower wireguard-linux-compat out of tree module, this commit does a patch-by-patch backport of upstream's wireguard to 5.4. This specific backport is in widespread use, being part of SUSE's enterprise kernel, Oracle's enterprise kernel, Google's Android kernel, Gentoo's distro kernel, and probably more I've forgotten about. It's definately the "more proper" way of adding wireguard to a kernel than the ugly compat.h hell of the wireguard-linux-compat repo. And most importantly for OpenWRT, it allows using the same module configuration code for 5.10 as for 5.4, with no need for bifurcation. These patches are from the backport tree which is maintained in the open here: https://git.zx2c4.com/wireguard-linux/log/?h=backport-5.4.y I'll be sending PRs to update this as needed. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> (cherry picked from commit3888fa7880) (cherry picked from commitd540725871) (cherry picked from commit196f3d586f) (cherry picked from commit3500fd7938) (cherry picked from commit23b801d3ba) (cherry picked from commit0c0cb97da7) (cherry picked from commit2a27f6f90a) Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
128 lines
6.1 KiB
Diff
128 lines
6.1 KiB
Diff
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)
|