Development discussion of WireGuard
 help / color / mirror / Atom feed
* [PATCH net 0/2] wireguard: device: fix metadata_dst xmit null pointer dereference
@ 2022-04-14 10:44 Nikolay Aleksandrov
  2022-04-14 10:44 ` [PATCH net 1/2] " Nikolay Aleksandrov
  2022-04-14 10:44 ` [PATCH net 2/2] wireguard: selftests: add metadata_dst xmit selftest Nikolay Aleksandrov
  0 siblings, 2 replies; 8+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-14 10:44 UTC (permalink / raw)
  To: netdev
  Cc: Daniel Borkmann, Martynas Pumputis, Jason A . Donenfeld,
	wireguard, kuba, davem, Nikolay Aleksandrov

Hi,
Patch 01 fixes a null pointer dereference in wireguard's xmit path when
transmitting skbs with metadata_dst attached. Patch 02 adds a selftest for
that case using bpf_skb_set_tunnel_key on wg device's egress path.

Thanks,
 Nik

Nikolay Aleksandrov (2):
  wireguard: device: fix metadata_dst xmit null pointer dereference
  wireguard: selftests: add metadata_dst xmit selftest

 drivers/net/wireguard/device.c             |  3 +-
 tools/testing/selftests/wireguard/netns.sh | 63 ++++++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net 1/2] wireguard: device: fix metadata_dst xmit null pointer dereference
  2022-04-14 10:44 [PATCH net 0/2] wireguard: device: fix metadata_dst xmit null pointer dereference Nikolay Aleksandrov
@ 2022-04-14 10:44 ` Nikolay Aleksandrov
  2022-04-14 11:28   ` Daniel Borkmann
  2022-04-14 11:58   ` Jason A. Donenfeld
  2022-04-14 10:44 ` [PATCH net 2/2] wireguard: selftests: add metadata_dst xmit selftest Nikolay Aleksandrov
  1 sibling, 2 replies; 8+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-14 10:44 UTC (permalink / raw)
  To: netdev
  Cc: Daniel Borkmann, Martynas Pumputis, Jason A . Donenfeld,
	wireguard, kuba, davem, Nikolay Aleksandrov, stable

When we try to transmit an skb with md_dst attached through wireguard
we hit a null pointer dereference[1] in wg_xmit() due to the use of
dst_mtu() which calls into dst_blackhole_mtu() which in turn tries to
dereference dst->dev. Since wireguard doesn't use md_dsts we should use
skb_valid_dst() which checks for DST_METADATA flag and if it's set then
fallback to wireguard's device mtu. That gives us the best chance of
transmitting the packet, otherwise if the blackhole netdev is used we'd
get ETH_MIN_MTU.

[1] calltrace:
 [  263.693506] BUG: kernel NULL pointer dereference, address: 00000000000000e0
 [  263.693908] #PF: supervisor read access in kernel mode
 [  263.694174] #PF: error_code(0x0000) - not-present page
 [  263.694424] PGD 0 P4D 0
 [  263.694653] Oops: 0000 [#1] PREEMPT SMP NOPTI
 [  263.694876] CPU: 5 PID: 951 Comm: mausezahn Kdump: loaded Not tainted 5.18.0-rc1+ #522
 [  263.695190] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014
 [  263.695529] RIP: 0010:dst_blackhole_mtu+0x17/0x20
 [  263.695770] Code: 00 00 00 0f 1f 44 00 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 8b 47 10 48 83 e0 fc 8b 40 04 85 c0 75 09 48 8b 07 <8b> 80 e0 00 00 00 c3 66 90 0f 1f 44 00 00 48 89 d7 be 01 00 00 00
 [  263.696339] RSP: 0018:ffffa4a4422fbb28 EFLAGS: 00010246
 [  263.696600] RAX: 0000000000000000 RBX: ffff8ac9c3553000 RCX: 0000000000000000
 [  263.696891] RDX: 0000000000000401 RSI: 00000000fffffe01 RDI: ffffc4a43fb48900
 [  263.697178] RBP: ffffa4a4422fbb90 R08: ffffffff9622635e R09: 0000000000000002
 [  263.697469] R10: ffffffff9b69a6c0 R11: ffffa4a4422fbd0c R12: ffff8ac9d18b1a00
 [  263.697766] R13: ffff8ac9d0ce1840 R14: ffff8ac9d18b1a00 R15: ffff8ac9c3553000
 [  263.698054] FS:  00007f3704c337c0(0000) GS:ffff8acaebf40000(0000) knlGS:0000000000000000
 [  263.698470] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 [  263.698826] CR2: 00000000000000e0 CR3: 0000000117a5c000 CR4: 00000000000006e0
 [  263.699214] Call Trace:
 [  263.699505]  <TASK>
 [  263.699759]  wg_xmit+0x411/0x450
 [  263.700059]  ? bpf_skb_set_tunnel_key+0x46/0x2d0
 [   263.700382]  ? dev_queue_xmit_nit+0x31/0x2b0
 [  263.700719]  dev_hard_start_xmit+0xd9/0x220
 [  263.701047]  __dev_queue_xmit+0x8b9/0xd30
 [  263.701344]  __bpf_redirect+0x1a4/0x380
 [  263.701664]  __dev_queue_xmit+0x83b/0xd30
 [  263.701961]  ? packet_parse_headers+0xb4/0xf0
 [  263.702275]  packet_sendmsg+0x9a8/0x16a0
 [  263.702596]  ? _raw_spin_unlock_irqrestore+0x23/0x40
 [  263.702933]  sock_sendmsg+0x5e/0x60
 [  263.703239]  __sys_sendto+0xf0/0x160
 [  263.703549]  __x64_sys_sendto+0x20/0x30
 [  263.703853]  do_syscall_64+0x3b/0x90
 [  263.704162]  entry_SYSCALL_64_after_hwframe+0x44/0xae
 [  263.704494] RIP: 0033:0x7f3704d50506
 [  263.704789] Code: 48 c7 c0 ff ff ff ff eb b7 66 2e 0f 1f 84 00 00 00 00 00 90 41 89 ca 64 8b 04 25 18 00 00 00 85 c0 75 11 b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 72 c3 90 55 48 83 ec 30 44 89 4c 24 2c 4c 89
 [  263.705652] RSP: 002b:00007ffe954b0b88 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
 [  263.706141] RAX: ffffffffffffffda RBX: 0000558bb259b490 RCX: 00007f3704d50506
 [  263.706544] RDX: 000000000000004a RSI: 0000558bb259b7b2 RDI: 0000000000000003
 [  263.706952] RBP: 0000000000000000 R08: 00007ffe954b0b90 R09: 0000000000000014
 [  263.707339] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe954b0b90
 [  263.707735] R13: 000000000000004a R14: 0000558bb259b7b2 R15: 0000000000000001
 [  263.708132]  </TASK>
 [  263.708398] Modules linked in: bridge netconsole bonding [last unloaded: bridge]
 [  263.708942] CR2: 00000000000000e0

CC: stable@vger.kernel.org
CC: wireguard@lists.zx2c4.com
CC: Jason A. Donenfeld <Jason@zx2c4.com>
CC: Daniel Borkmann <daniel@iogearbox.net>
CC: Martynas Pumputis <m@lambda.lt>
Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
Reported-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 drivers/net/wireguard/device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
index 0fad1331303c..aa9a7a5970fd 100644
--- a/drivers/net/wireguard/device.c
+++ b/drivers/net/wireguard/device.c
@@ -19,6 +19,7 @@
 #include <linux/if_arp.h>
 #include <linux/icmp.h>
 #include <linux/suspend.h>
+#include <net/dst_metadata.h>
 #include <net/icmp.h>
 #include <net/rtnetlink.h>
 #include <net/ip_tunnels.h>
@@ -167,7 +168,7 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto err_peer;
 	}
 
-	mtu = skb_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu;
+	mtu = skb_valid_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu;
 
 	__skb_queue_head_init(&packets);
 	if (!skb_is_gso(skb)) {
-- 
2.35.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net 2/2] wireguard: selftests: add metadata_dst xmit selftest
  2022-04-14 10:44 [PATCH net 0/2] wireguard: device: fix metadata_dst xmit null pointer dereference Nikolay Aleksandrov
  2022-04-14 10:44 ` [PATCH net 1/2] " Nikolay Aleksandrov
@ 2022-04-14 10:44 ` Nikolay Aleksandrov
  2022-04-14 12:06   ` Jason A. Donenfeld
  1 sibling, 1 reply; 8+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-14 10:44 UTC (permalink / raw)
  To: netdev
  Cc: Daniel Borkmann, Martynas Pumputis, Jason A . Donenfeld,
	wireguard, kuba, davem, Nikolay Aleksandrov

Add a selftest for transmitting skb with md_dst attached. It is done via
a bpf program which uses bpf_skb_set_tunnel_key on wireguard's egress
path. It requires clang and tc to be installed. If the test finishes
without a crash it is considered successful.

CC: wireguard@lists.zx2c4.com
CC: Jason A. Donenfeld <Jason@zx2c4.com>
CC: Daniel Borkmann <daniel@iogearbox.net>
CC: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
Executed the prep compilation commands with n1 to make them visible.

 tools/testing/selftests/wireguard/netns.sh | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/tools/testing/selftests/wireguard/netns.sh b/tools/testing/selftests/wireguard/netns.sh
index 8a9461aa0878..b492dbb94245 100755
--- a/tools/testing/selftests/wireguard/netns.sh
+++ b/tools/testing/selftests/wireguard/netns.sh
@@ -156,6 +156,67 @@ tests() {
 	done
 }
 
+md_dst_test_cleanup() {
+	rm -rf /tmp/test_wg_tun.c /tmp/test_wg_tun.ll /tmp/test_wg_tun.o
+	n1 tc qdisc del dev wg0 clsact
+}
+
+# test for md dst on wireguard's egress path
+md_dst_test() {
+	# clang is required for the test
+	if [[ ! -x "$(command -v "clang")" ]]; then
+		return
+	fi
+
+	# attach md dst to the skb on egress using bpf_skb_set_tunnel_key
+	n1 cat > /tmp/test_wg_tun.c <<EOF
+#include <linux/bpf.h>
+
+#ifndef TC_ACT_OK
+# define TC_ACT_OK 0
+#endif
+
+static long (*bpf_skb_set_tunnel_key)(struct __sk_buff *skb, struct bpf_tunnel_key *key, __u32 size, __u64 flags) = (void *) 21;
+
+__attribute__((section("egress"), used))
+int tc_egress(struct __sk_buff *skb)
+{
+	struct bpf_tunnel_key key = {};
+
+        bpf_skb_set_tunnel_key(skb, &key, sizeof(key), 0);
+
+	return TC_ACT_OK;
+}
+
+char __license[] __attribute__((section("license"), used)) = "GPL";
+EOF
+
+	n1 clang -O2 -emit-llvm -c /tmp/test_wg_tun.c -o /tmp/test_wg_tun.ll
+	if [[ ! -f "/tmp/test_wg_tun.ll" ]]; then
+		md_dst_test_cleanup
+		return
+	fi
+	n1 llc -march=bpf -filetype=obj -o /tmp/test_wg_tun.o /tmp/test_wg_tun.ll
+	if [[ ! -f "/tmp/test_wg_tun.o" ]]; then
+		md_dst_test_cleanup
+		return
+	fi
+
+	n1 tc qdisc add dev wg0 clsact
+	if [[ $? -ne 0 ]]; then
+		md_dst_test_cleanup
+		return
+	fi
+	n1 tc filter add dev wg0 egress basic action bpf obj /tmp/test_wg_tun.o sec egress
+	if [[ $? -ne 0 ]]; then
+		md_dst_test_cleanup
+		return
+	fi
+	n1 ping -c 2 -f -W 1 192.168.241.2
+	# if we reach here without a crash the test passed
+	md_dst_test_cleanup
+}
+
 [[ $(ip1 link show dev wg0) =~ mtu\ ([0-9]+) ]] && orig_mtu="${BASH_REMATCH[1]}"
 big_mtu=$(( 34816 - 1500 + $orig_mtu ))
 
@@ -175,6 +236,8 @@ read _ rx_bytes tx_bytes < <(n1 wg show wg0 transfer)
 read _ timestamp < <(n1 wg show wg0 latest-handshakes)
 (( timestamp != 0 ))
 
+md_dst_test
+
 tests
 ip1 link set wg0 mtu $big_mtu
 ip2 link set wg0 mtu $big_mtu
-- 
2.35.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 1/2] wireguard: device: fix metadata_dst xmit null pointer dereference
  2022-04-14 10:44 ` [PATCH net 1/2] " Nikolay Aleksandrov
@ 2022-04-14 11:28   ` Daniel Borkmann
  2022-04-14 11:58   ` Jason A. Donenfeld
  1 sibling, 0 replies; 8+ messages in thread
From: Daniel Borkmann @ 2022-04-14 11:28 UTC (permalink / raw)
  To: Nikolay Aleksandrov, netdev
  Cc: Martynas Pumputis, Jason A . Donenfeld, wireguard, kuba, davem, stable

On 4/14/22 12:44 PM, Nikolay Aleksandrov wrote:
> When we try to transmit an skb with md_dst attached through wireguard
> we hit a null pointer dereference[1] in wg_xmit() due to the use of
> dst_mtu() which calls into dst_blackhole_mtu() which in turn tries to
> dereference dst->dev. Since wireguard doesn't use md_dsts we should use
> skb_valid_dst() which checks for DST_METADATA flag and if it's set then
> fallback to wireguard's device mtu. That gives us the best chance of
> transmitting the packet, otherwise if the blackhole netdev is used we'd
> get ETH_MIN_MTU.
> 
[...]
> 
> CC: stable@vger.kernel.org
> CC: wireguard@lists.zx2c4.com
> CC: Jason A. Donenfeld <Jason@zx2c4.com>
> CC: Daniel Borkmann <daniel@iogearbox.net>
> CC: Martynas Pumputis <m@lambda.lt>
> Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
> Reported-by: Martynas Pumputis <m@lambda.lt>
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>

Looks good to me, thanks Nik!

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 1/2] wireguard: device: fix metadata_dst xmit null pointer dereference
  2022-04-14 10:44 ` [PATCH net 1/2] " Nikolay Aleksandrov
  2022-04-14 11:28   ` Daniel Borkmann
@ 2022-04-14 11:58   ` Jason A. Donenfeld
  1 sibling, 0 replies; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-04-14 11:58 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: Netdev, Daniel Borkmann, Martynas Pumputis,
	WireGuard mailing list, Jakub Kicinski, David Miller, stable

Hi Nikolay,

On Thu, Apr 14, 2022 at 12:45 PM Nikolay Aleksandrov
<razor@blackwall.org> wrote:
> When we try to transmit an skb with md_dst attached through wireguard
> we hit a null pointer dereference[1] in wg_xmit() due to the use of
> dst_mtu() which calls into dst_blackhole_mtu() which in turn tries to
> dereference dst->dev. Since wireguard doesn't use md_dsts we should use
> skb_valid_dst() which checks for DST_METADATA flag and if it's set then
> fallback to wireguard's device mtu. That gives us the best chance of
> transmitting the packet, otherwise if the blackhole netdev is used we'd
> get ETH_MIN_MTU.

Thanks for the patch. Will queue up this patch #1 in the wireguard
tree and send it out to net.git not before too long.

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/2] wireguard: selftests: add metadata_dst xmit selftest
  2022-04-14 10:44 ` [PATCH net 2/2] wireguard: selftests: add metadata_dst xmit selftest Nikolay Aleksandrov
@ 2022-04-14 12:06   ` Jason A. Donenfeld
  2022-04-14 12:12     ` Nikolay Aleksandrov
  0 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-04-14 12:06 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: Netdev, Daniel Borkmann, Martynas Pumputis,
	WireGuard mailing list, Jakub Kicinski, David Miller

Hi Nikolay,

These tests need to run in the minimal fast-to-compile test harness
inside of tools/testing/selftests/wireguard/qemu, which you can try
out with:

$ make -C tools/testing/selftests/wireguard/qemu -j$(nproc)

Currently iproute2 is built, but only ip(8) is used in the image, so
you'll need to add tc(8) to there. Clang, however, seems a bit
heavyweight. I suspect it'd make more sense to just base64 up that
object file and include it as a string in the file? Or, alternatively,
we could just not move ahead with this rather niche test, and revisit
the issue if we wind up wanting to test for lots of bpf things.
Thoughts on that?

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/2] wireguard: selftests: add metadata_dst xmit selftest
  2022-04-14 12:06   ` Jason A. Donenfeld
@ 2022-04-14 12:12     ` Nikolay Aleksandrov
  2022-04-14 12:24       ` Jason A. Donenfeld
  0 siblings, 1 reply; 8+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-14 12:12 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Netdev, Daniel Borkmann, Martynas Pumputis,
	WireGuard mailing list, Jakub Kicinski, David Miller

On 14/04/2022 15:06, Jason A. Donenfeld wrote:
> Hi Nikolay,
> 
> These tests need to run in the minimal fast-to-compile test harness
> inside of tools/testing/selftests/wireguard/qemu, which you can try
> out with:
> 
> $ make -C tools/testing/selftests/wireguard/qemu -j$(nproc)
> 
> Currently iproute2 is built, but only ip(8) is used in the image, so
> you'll need to add tc(8) to there. Clang, however, seems a bit
> heavyweight. I suspect it'd make more sense to just base64 up that
> object file and include it as a string in the file? Or, alternatively,
> we could just not move ahead with this rather niche test, and revisit
> the issue if we wind up wanting to test for lots of bpf things.
> Thoughts on that?
> 
> Jason

Hi Jason,
My bad, I completely missed the qemu part. I'll look into including the
ready object file. If it works out, looks compact and well
I'll post v2.

Thanks,
 Nik

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/2] wireguard: selftests: add metadata_dst xmit selftest
  2022-04-14 12:12     ` Nikolay Aleksandrov
@ 2022-04-14 12:24       ` Jason A. Donenfeld
  0 siblings, 0 replies; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-04-14 12:24 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: Netdev, Daniel Borkmann, Martynas Pumputis,
	WireGuard mailing list, Jakub Kicinski, David Miller

On Thu, Apr 14, 2022 at 2:12 PM Nikolay Aleksandrov <razor@blackwall.org> wrote:
> My bad, I completely missed the qemu part.

If you're curious, by the way, there's this running all those tests
for every commit https://www.wireguard.com/build-status/

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-04-22  0:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-14 10:44 [PATCH net 0/2] wireguard: device: fix metadata_dst xmit null pointer dereference Nikolay Aleksandrov
2022-04-14 10:44 ` [PATCH net 1/2] " Nikolay Aleksandrov
2022-04-14 11:28   ` Daniel Borkmann
2022-04-14 11:58   ` Jason A. Donenfeld
2022-04-14 10:44 ` [PATCH net 2/2] wireguard: selftests: add metadata_dst xmit selftest Nikolay Aleksandrov
2022-04-14 12:06   ` Jason A. Donenfeld
2022-04-14 12:12     ` Nikolay Aleksandrov
2022-04-14 12:24       ` Jason A. Donenfeld

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).