[PATCH net] wireguard: fix race in wg_index_hashtable_replace()

[PATCH net] wireguard: fix race in wg_index_hashtable_replace()

[Eric Dumazet]

syzbot got a NULL dereference in wg_index_hashtable_replace() [1]

Issue here is that right after checking hlist_unhashed(&old->index_hash)
another cpu might have removed @old already from the hash.

Since we are dealing with a very unlikely case, we can simply
acquire the table lock earlier.

[1]
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
Modules linked in:
---[ end trace 0d737db78b72da84 ]---
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

Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: wireguard@lists.zx2c4.com
---
 drivers/net/wireguard/peerlookup.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireguard/peerlookup.c b/drivers/net/wireguard/peerlookup.c
index e4deb331476b3d67c1d24eb269b26f587798cbc2..d82eba263292474a7cdfb133b2ba1862a2e9352c 100644
--- a/drivers/net/wireguard/peerlookup.c
+++ b/drivers/net/wireguard/peerlookup.c
@@ -167,21 +167,25 @@ bool wg_index_hashtable_replace(struct index_hashtable *table,
 				struct index_hashtable_entry *old,
 				struct index_hashtable_entry *new)
 {
-	if (unlikely(hlist_unhashed(&old->index_hash)))
-		return false;
+
+	bool replaced = false;
+
 	spin_lock_bh(&table->lock);
-	new->index = old->index;
-	hlist_replace_rcu(&old->index_hash, &new->index_hash);
+	if (likely(!hlist_unhashed(&old->index_hash))) {
+		replaced = true;
+		new->index = old->index;
+		hlist_replace_rcu(&old->index_hash, &new->index_hash);
 
-	/* Calling init here NULLs out index_hash, and in fact after this
-	 * function returns, it's theoretically possible for this to get
-	 * reinserted elsewhere. That means the RCU lookup below might either
-	 * terminate early or jump between buckets, in which case the packet
-	 * simply gets dropped, which isn't terrible.
-	 */
-	INIT_HLIST_NODE(&old->index_hash);
+		/* Calling init here NULLs out index_hash, and in fact after this
+		 * function returns, it's theoretically possible for this to get
+		 * reinserted elsewhere. That means the RCU lookup below might either
+		 * terminate early or jump between buckets, in which case the packet
+		 * simply gets dropped, which isn't terrible.
+		 */
+		INIT_HLIST_NODE(&old->index_hash);
+	}
 	spin_unlock_bh(&table->lock);
-	return true;
+	return replaced;
 }
 
 void wg_index_hashtable_remove(struct index_hashtable *table,
-- 
2.28.0.526.ge36021eeef-goog

Re: [PATCH net] wireguard: fix race in wg_index_hashtable_replace()

[Jason A. Donenfeld]

Hey Eric,

On Tue, Sep 8, 2020 at 4:59 PM Eric Dumazet <edumazet@google.com> wrote:
>
> syzbot got a NULL dereference in wg_index_hashtable_replace() [1]
>
> Issue here is that right after checking hlist_unhashed(&old->index_hash)
> another cpu might have removed @old already from the hash.
>
> Since we are dealing with a very unlikely case, we can simply
> acquire the table lock earlier.

That's a nice bug. It looks like this is triggered by a teardown race,
when wg_index_hashtable_replace races with wg_index_hashtable_remove.

Since all the other hashtable mutator functions are protected by that
spinlock, it doesn't seem harmful to fix this by doing the same, even
if formally that spinlock is supposed to protect hash bucket heads
rather than entry pointers.

I'm playing with your patch and a variant of it, which I'll have
queued up in my tree in the next hour or so.

Thanks a lot for triaging this.

Jason