From patchwork Wed Sep 9 11:58:14 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Jason A. Donenfeld" X-Patchwork-Id: 249459 Delivered-To: patch@linaro.org Received: by 2002:a92:5b9c:0:0:0:0:0 with SMTP id c28csp503605ilg; Wed, 9 Sep 2020 08:11:05 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxHQsuFKkn3Ls3bPHRTo+0/U3Oe2P1CA0lDqg3rK+pkcCs337qzWQG5V9GlnIbu04ltSIfF X-Received: by 2002:aa7:d6c6:: with SMTP id x6mr4615175edr.338.1599664265127; Wed, 09 Sep 2020 08:11:05 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1599664265; cv=none; d=google.com; s=arc-20160816; b=sk2u5h3a0XUlxKlwOyHvVD7RE7APAA+GLM+ku36hqSXcswkXWTe9adwfedrOLai84s eCkMrirDdN0fQvkOGEIaQacV499ifNUXAABNo0Oxo68B2Fq2lIzjtL6BHG7cI1+yAgGT 3g2GRJRB46wWK8LfHOvQCX/AM8redIzKk9ngnu6FMWVI/aBlmq0/mnUr82zTpV3hYvlL TRfRSn5nElcyCrqivX5EbtgryWyC1B+vP5tFidGyhYDNHtt566vRvWQrTO3mf2E9T150 c66tYsHs3paNUydrLVqlE8Bzux4Bc4kDtvNBJgwane75fkMVTHZhciH2v9qMUabGxm/f XMoA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=HGxEmaKBC/kjxueovbmulhL0g9y1GjANEmnFmgWWJ6Y=; b=D9vUPdYcf5DhF/kN7B+jfSvvM2LS+MmddgfEfENXNTNpuHoVprXJ5/0JrxSEtifl5+ 1uISjnDN3KYEncytpO8nf/o2ZR/QlkuHDaRhdk5KUd4QWM2+5siJjekgDbZRCAjMQ2v0 aK+kE1vVa69YLZEjIGPJOcO8Szn8+EEE0MpozsIf/Eu7tb4F9VvjiyjFBGYEk1SERm00 kBf7rX9B08QOw0TGG9SYgdPQK7CoBIuyXhjCNCp8mgPsw3g+f+YnLf7JMBew9nYkCQOh 30hj3l4MMOWoVdAwAj8cppQqDavn46Pw4I611sf6jOsv73e+zmr9CeNGg7kmGuIeDUMP pJAg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@zx2c4.com header.s=mail header.b=Ff5SFrAm; spf=pass (google.com: domain of netdev-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=netdev-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=zx2c4.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id d6si1688153ejj.92.2020.09.09.08.11.04; Wed, 09 Sep 2020 08:11:05 -0700 (PDT) Received-SPF: pass (google.com: domain of netdev-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@zx2c4.com header.s=mail header.b=Ff5SFrAm; spf=pass (google.com: domain of netdev-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=netdev-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=zx2c4.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729789AbgIIPKy (ORCPT + 9 others); Wed, 9 Sep 2020 11:10:54 -0400 Received: from mail.zx2c4.com ([192.95.5.64]:57309 "EHLO mail.zx2c4.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730083AbgIIL73 (ORCPT ); Wed, 9 Sep 2020 07:59:29 -0400 Received: by mail.zx2c4.com (ZX2C4 Mail Server) with ESMTP id 0002fbc2; Wed, 9 Sep 2020 11:29:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=zx2c4.com; h=from:to:cc :subject:date:message-id:in-reply-to:references:mime-version :content-transfer-encoding; s=mail; bh=WRPTJHH8GASfujm1rHt7Zug89 kw=; b=Ff5SFrAmLcBdJNdQC+SiUKJqTzN7xS6uEhEtNA5wpVp/+GHXL/b8m6dIM 23slzrCxN9CqE4ddt1is/YyRJQOg2PsPk3lG21Ode164a/H5CXKERY0W13hLvkyS iPqsAo8A0vKNwqs61b1e5Wm48L0r+FkVAQjhfkCiOBwzn6A4FubJRSwsJO9TOJSX dIKVln3GptJyuM4wt3s1TE0j1xVuOv+siMed5bW5oAybwNaqLyFh0b6YHD+XdXWm LntG9yirjpqalruGEbstmuxfHUDAnWZiF1aaDGzU5Bblj/8T7AXohL2iNL/8xpBP OYNB0HYkPJhaVmiMpqShmM4aIyRCg== Received: by mail.zx2c4.com (ZX2C4 Mail Server) with ESMTPSA id 795dd0f3 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Wed, 9 Sep 2020 11:29:29 +0000 (UTC) From: "Jason A. Donenfeld" To: netdev@vger.kernel.org, davem@davemloft.net Cc: "Jason A. Donenfeld" , syzbot , Eric Dumazet Subject: [PATCH net 1/2] wireguard: noise: take lock when removing handshake entry from table Date: Wed, 9 Sep 2020 13:58:14 +0200 Message-Id: <20200909115815.522168-2-Jason@zx2c4.com> In-Reply-To: <20200909115815.522168-1-Jason@zx2c4.com> References: <20200909115815.522168-1-Jason@zx2c4.com> MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 Reported-by: Eric Dumazet 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 --- drivers/net/wireguard/noise.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) -- 2.28.0 diff --git a/drivers/net/wireguard/noise.c b/drivers/net/wireguard/noise.c index 3dd3b76790d0..c0cfd9b36c0b 100644 --- a/drivers/net/wireguard/noise.c +++ b/drivers/net/wireguard/noise.c @@ -87,15 +87,12 @@ static void handshake_zero(struct noise_handshake *handshake) 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) From patchwork Wed Sep 9 11:58:15 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Jason A. Donenfeld" X-Patchwork-Id: 249458 Delivered-To: patch@linaro.org Received: by 2002:a92:5b9c:0:0:0:0:0 with SMTP id c28csp501562ilg; Wed, 9 Sep 2020 08:08:47 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzNCK4LEV2auIO5DpdgHO9qI+lcPmAYqMc+umh7BCTqo3EtwyKAeqGbpAsrjNM4Vwgfh5dB X-Received: by 2002:aa7:d6ce:: with SMTP id x14mr4587439edr.359.1599664126905; Wed, 09 Sep 2020 08:08:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1599664126; cv=none; d=google.com; s=arc-20160816; b=FZW4HaW2+gvKOhSZoJGK3O0sO9HlTGfZu18/cLTI9ZKvb90SQkHKUkCqiRR7DK25LP qiCjpoRsYkbWKPw6QCfIHBbEM+GXnWtdx0Q4PubZcuMx/UJLwzt/VlxHLgRbX+wcA56r 0relB+vHKo+1yhi049VZbn/OwxiVPOTMQ99iceXp7reZJyBVOz9wlEQSgegFpih4QMij n95iLfGpRfTnZFP2Pf2GjurU5E85QhSdhEoJghKTHaOy6E8yySmBRvV16o8Ikm4r1Djy b2a/B2z55XHaO23GCBZQz7h2r9im+9Fi0V2V37mdbDvVQE9QZBiHaSX/dxwDQK3wx6E5 eczQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=vhYEJ4RfuH1AgOSP5pYrgQpp8UEhpSdhTwJ9Sxva3ek=; b=zYbH7lXD6fZVHFw9TJKzfTBwLQ1yd4fxv4C7QFIsk5pPinUAAAd8OPYG5VJIAaarFV 0PmedY95J3Mxe5tKodYJltz0R1k6JWZeH0nTWqi7p2o6PEsgMPkALC7RM5paYVGQDjgr JvTa6WRRQiMDTkvbh2ywc2Rz9LUkKQ2+E+wK6550Lfrc6crar4AUPGVs96/KL/tJmkys uRYoXVbXXfJkt8d7pu3jXxmbblr/RwGHHPBBNcro8G5uakezu8y7r+Voc2ZIr5K58Ogh YAaOgE6eOw7KJsVQZvVRdIW4SV/X5SQsciNwM9F58jw9fv5IURYavI4nWu5E/Qfh79sk RjVw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@zx2c4.com header.s=mail header.b=scWw2wrE; spf=pass (google.com: domain of netdev-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=netdev-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=zx2c4.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id w11si1664474ejy.632.2020.09.09.08.08.46; Wed, 09 Sep 2020 08:08:46 -0700 (PDT) Received-SPF: pass (google.com: domain of netdev-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@zx2c4.com header.s=mail header.b=scWw2wrE; spf=pass (google.com: domain of netdev-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=netdev-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=zx2c4.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726714AbgIIPH6 (ORCPT + 9 others); Wed, 9 Sep 2020 11:07:58 -0400 Received: from mail.zx2c4.com ([192.95.5.64]:44203 "EHLO mail.zx2c4.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730108AbgIIL7r (ORCPT ); Wed, 9 Sep 2020 07:59:47 -0400 Received: by mail.zx2c4.com (ZX2C4 Mail Server) with ESMTP id 95377b77; Wed, 9 Sep 2020 11:29:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=zx2c4.com; h=from:to:cc :subject:date:message-id:in-reply-to:references:mime-version :content-transfer-encoding; s=mail; bh=WW7WTo+4ZuqjauDDxeTu5QNSG m8=; b=scWw2wrECL47SQ9w+IMlWrHSFf0Yns2mnBP10l9R59+s6Iz3M3juB6gPg APjjwdvcWFRO5nHIHcf8qt5BunZe9AtzbEavJM/bv9z/ZW63W1iecdAhdoKgn+GE os3mglNAx3uqtJNZQS0WPtdcPwrrmg6HxgCYdpad3xCrj0ZMPv4tqjGqDsVqSHus 74eJaL7dQvKB8QL/RyBHcCM4+tOfIBSqarsb7v0blqjUBXwOlUCwCzucruRL4mZ0 TpNtf+H17CiXOHeGSpJ5zXLHKHQADf1lUbkKZg9KAO7d2iDReSox+73waEsrkM+a u1U7k8EACiH8DWNZXhI+SucTq6v5g== Received: by mail.zx2c4.com (ZX2C4 Mail Server) with ESMTPSA id 81b3e82f (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Wed, 9 Sep 2020 11:29:31 +0000 (UTC) From: "Jason A. Donenfeld" To: netdev@vger.kernel.org, davem@davemloft.net Cc: "Jason A. Donenfeld" , Eric Dumazet Subject: [PATCH net 2/2] wireguard: peerlookup: take lock before checking hash in replace operation Date: Wed, 9 Sep 2020 13:58:15 +0200 Message-Id: <20200909115815.522168-3-Jason@zx2c4.com> In-Reply-To: <20200909115815.522168-1-Jason@zx2c4.com> References: <20200909115815.522168-1-Jason@zx2c4.com> MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Eric's suggested fix for the previous commit's mentioned race condition was to simply take the table->lock in wg_index_hashtable_replace(). The table->lock of the hash table is supposed to protect the bucket heads, not the entires, but actually, since all the mutator functions are already taking it, it makes sense to take it too for the test to hlist_unhashed, as a defense in depth measure, so that it no longer races with deletions, regardless of what other locks are protecting individual entries. This is sensible from a performance perspective because, as Eric pointed out, the case of being unhashed is already the unlikely case, so this won't add common contention. And comparing instructions, this basically doesn't make much of a difference other than pushing and popping %r13, used by the new `bool ret`. More generally, I like the idea of locking consistency across table mutator functions, and this might let me rest slightly easier at night. Suggested-by: Eric Dumazet 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 --- drivers/net/wireguard/peerlookup.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) -- 2.28.0 diff --git a/drivers/net/wireguard/peerlookup.c b/drivers/net/wireguard/peerlookup.c index e4deb331476b..f2783aa7a88f 100644 --- a/drivers/net/wireguard/peerlookup.c +++ b/drivers/net/wireguard/peerlookup.c @@ -167,9 +167,13 @@ 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 ret; + spin_lock_bh(&table->lock); + ret = !hlist_unhashed(&old->index_hash); + if (unlikely(!ret)) + goto out; + new->index = old->index; hlist_replace_rcu(&old->index_hash, &new->index_hash); @@ -180,8 +184,9 @@ bool wg_index_hashtable_replace(struct index_hashtable *table, * simply gets dropped, which isn't terrible. */ INIT_HLIST_NODE(&old->index_hash); +out: spin_unlock_bh(&table->lock); - return true; + return ret; } void wg_index_hashtable_remove(struct index_hashtable *table,