[PATCH net 0/3] wireguard patches for 6.0-rc6

[PATCH net 0/3] wireguard patches for 6.0-rc6

[Jason A. Donenfeld]

Hey guys,

Sorry we didn't get a chance to talk at Plumbers. I saw some of you very
briefly in passing but never had the opportunity to chat. Next time, I
hope.

Please pull the following fixes:

1) The ratelimiter timing test doesn't help outside of development, yet
   it is currently preventing the module from being inserted on some
   kernels when it flakes at insertion time. So we disable it.

2) A fix for a build error on UML, caused by a recent change in a
   different tree.

3) A WARN_ON() is triggered by Kees' new fortified memcpy() patch, due
   to memcpy()ing over a sockaddr pointer with the size of a
   sockaddr_in[6]. The type safe fix is pretty simple. Given how classic
   of a thing sockaddr punning is, I suspect this may be the first in a
   few patches like this throughout the net tree, once Kees' fortify
   series is more widely deployed (current it's just in next).

Thanks,
Jason

Jason A. Donenfeld (3):
  wireguard: ratelimiter: disable timings test by default
  wireguard: selftests: do not install headers on UML
  wireguard: netlink: avoid variable-sized memcpy on sockaddr

 drivers/net/wireguard/netlink.c               | 13 +++++-----
 drivers/net/wireguard/selftest/ratelimiter.c  | 25 ++++++++-----------
 .../testing/selftests/wireguard/qemu/Makefile |  2 ++
 3 files changed, 18 insertions(+), 22 deletions(-)

-- 
2.37.3

[PATCH net 1/3] wireguard: ratelimiter: disable timings test by default

[Jason A. Donenfeld]

A previous commit tried to make the ratelimiter timings test more
reliable but in the process made it less reliable on other
configurations. This is an impossible problem to solve without
increasingly ridiculous heuristics. And it's not even a problem that
actually needs to be solved in any comprehensive way, since this is only
ever used during development. So just cordon this off with a DEBUG_
ifdef, just like we do for the trie's randomized tests, so it can be
enabled while hacking on the code, and otherwise disabled in CI. In the
process we also revert 151c8e499f47.

Fixes: 151c8e499f47 ("wireguard: ratelimiter: use hrtimer in selftest")
Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/wireguard/selftest/ratelimiter.c | 25 ++++++++------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireguard/selftest/ratelimiter.c b/drivers/net/wireguard/selftest/ratelimiter.c
index ba87d294604f..d4bb40a695ab 100644
--- a/drivers/net/wireguard/selftest/ratelimiter.c
+++ b/drivers/net/wireguard/selftest/ratelimiter.c
@@ -6,29 +6,28 @@
 #ifdef DEBUG
 
 #include <linux/jiffies.h>
-#include <linux/hrtimer.h>
 
 static const struct {
 	bool result;
-	u64 nsec_to_sleep_before;
+	unsigned int msec_to_sleep_before;
 } expected_results[] __initconst = {
 	[0 ... PACKETS_BURSTABLE - 1] = { true, 0 },
 	[PACKETS_BURSTABLE] = { false, 0 },
-	[PACKETS_BURSTABLE + 1] = { true, NSEC_PER_SEC / PACKETS_PER_SECOND },
+	[PACKETS_BURSTABLE + 1] = { true, MSEC_PER_SEC / PACKETS_PER_SECOND },
 	[PACKETS_BURSTABLE + 2] = { false, 0 },
-	[PACKETS_BURSTABLE + 3] = { true, (NSEC_PER_SEC / PACKETS_PER_SECOND) * 2 },
+	[PACKETS_BURSTABLE + 3] = { true, (MSEC_PER_SEC / PACKETS_PER_SECOND) * 2 },
 	[PACKETS_BURSTABLE + 4] = { true, 0 },
 	[PACKETS_BURSTABLE + 5] = { false, 0 }
 };
 
 static __init unsigned int maximum_jiffies_at_index(int index)
 {
-	u64 total_nsecs = 2 * NSEC_PER_SEC / PACKETS_PER_SECOND / 3;
+	unsigned int total_msecs = 2 * MSEC_PER_SEC / PACKETS_PER_SECOND / 3;
 	int i;
 
 	for (i = 0; i <= index; ++i)
-		total_nsecs += expected_results[i].nsec_to_sleep_before;
-	return nsecs_to_jiffies(total_nsecs);
+		total_msecs += expected_results[i].msec_to_sleep_before;
+	return msecs_to_jiffies(total_msecs);
 }
 
 static __init int timings_test(struct sk_buff *skb4, struct iphdr *hdr4,
@@ -43,12 +42,8 @@ static __init int timings_test(struct sk_buff *skb4, struct iphdr *hdr4,
 	loop_start_time = jiffies;
 
 	for (i = 0; i < ARRAY_SIZE(expected_results); ++i) {
-		if (expected_results[i].nsec_to_sleep_before) {
-			ktime_t timeout = ktime_add(ktime_add_ns(ktime_get_coarse_boottime(), TICK_NSEC * 4 / 3),
-						    ns_to_ktime(expected_results[i].nsec_to_sleep_before));
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			schedule_hrtimeout_range_clock(&timeout, 0, HRTIMER_MODE_ABS, CLOCK_BOOTTIME);
-		}
+		if (expected_results[i].msec_to_sleep_before)
+			msleep(expected_results[i].msec_to_sleep_before);
 
 		if (time_is_before_jiffies(loop_start_time +
 					   maximum_jiffies_at_index(i)))
@@ -132,7 +127,7 @@ bool __init wg_ratelimiter_selftest(void)
 	if (IS_ENABLED(CONFIG_KASAN) || IS_ENABLED(CONFIG_UBSAN))
 		return true;
 
-	BUILD_BUG_ON(NSEC_PER_SEC % PACKETS_PER_SECOND != 0);
+	BUILD_BUG_ON(MSEC_PER_SEC % PACKETS_PER_SECOND != 0);
 
 	if (wg_ratelimiter_init())
 		goto out;
@@ -172,7 +167,7 @@ bool __init wg_ratelimiter_selftest(void)
 	++test;
 #endif
 
-	for (trials = TRIALS_BEFORE_GIVING_UP;;) {
+	for (trials = TRIALS_BEFORE_GIVING_UP; IS_ENABLED(DEBUG_RATELIMITER_TIMINGS);) {
 		int test_count = 0, ret;
 
 		ret = timings_test(skb4, hdr4, skb6, hdr6, &test_count);
-- 
2.37.3

[PATCH net 2/3] wireguard: selftests: do not install headers on UML

[Jason A. Donenfeld]

Since 1b620d539ccc ("kbuild: disable header exports for UML in a
straightforward way"), installing headers fails on UML, so just disable
installing them, since they're not needed anyway on the architecture.

Fixes: b438b3b8d6e6 ("wireguard: selftests: support UML")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 tools/testing/selftests/wireguard/qemu/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/wireguard/qemu/Makefile b/tools/testing/selftests/wireguard/qemu/Makefile
index fda76282d34b..e95bd56b332f 100644
--- a/tools/testing/selftests/wireguard/qemu/Makefile
+++ b/tools/testing/selftests/wireguard/qemu/Makefile
@@ -343,8 +343,10 @@ $(KERNEL_BZIMAGE): $(TOOLCHAIN_PATH)/.installed $(KERNEL_BUILD_PATH)/.config $(B
 .PHONY: $(KERNEL_BZIMAGE)
 
 $(TOOLCHAIN_PATH)/$(CHOST)/include/linux/.installed: | $(KERNEL_BUILD_PATH)/.config $(TOOLCHAIN_PATH)/.installed
+ifneq ($(ARCH),um)
 	rm -rf $(TOOLCHAIN_PATH)/$(CHOST)/include/linux
 	$(MAKE) -C $(KERNEL_PATH) O=$(KERNEL_BUILD_PATH) INSTALL_HDR_PATH=$(TOOLCHAIN_PATH)/$(CHOST) ARCH=$(KERNEL_ARCH) CROSS_COMPILE=$(CROSS_COMPILE) headers_install
+endif
 	touch $@
 
 $(TOOLCHAIN_PATH)/.installed: $(TOOLCHAIN_TAR)
-- 
2.37.3

[PATCH net 3/3] wireguard: netlink: avoid variable-sized memcpy on sockaddr

[Jason A. Donenfeld]

Doing a variable-sized memcpy is slower, and the compiler isn't smart
enough to turn this into a constant-size assignment.

Further, Kees' latest fortified memcpy will actually bark, because the
destination pointer is type sockaddr, not explicitly sockaddr_in or
sockaddr_in6, so it thinks there's an overflow:

    memcpy: detected field-spanning write (size 28) of single field
    "&endpoint.addr" at drivers/net/wireguard/netlink.c:446 (size 16)

Fix this by just assigning by using explicit casts for each checked
case.

Cc: Kees Cook <keescook@chromium.org>
Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/wireguard/netlink.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
index d0f3b6d7f408..5c804bcabfe6 100644
--- a/drivers/net/wireguard/netlink.c
+++ b/drivers/net/wireguard/netlink.c
@@ -436,14 +436,13 @@ static int set_peer(struct wg_device *wg, struct nlattr **attrs)
 	if (attrs[WGPEER_A_ENDPOINT]) {
 		struct sockaddr *addr = nla_data(attrs[WGPEER_A_ENDPOINT]);
 		size_t len = nla_len(attrs[WGPEER_A_ENDPOINT]);
+		struct endpoint endpoint = { { { 0 } } };
 
-		if ((len == sizeof(struct sockaddr_in) &&
-		     addr->sa_family == AF_INET) ||
-		    (len == sizeof(struct sockaddr_in6) &&
-		     addr->sa_family == AF_INET6)) {
-			struct endpoint endpoint = { { { 0 } } };
-
-			memcpy(&endpoint.addr, addr, len);
+		if (len == sizeof(struct sockaddr_in) && addr->sa_family == AF_INET) {
+			endpoint.addr4 = *(struct sockaddr_in *)addr;
+			wg_socket_set_peer_endpoint(peer, &endpoint);
+		} else if (len == sizeof(struct sockaddr_in6) && addr->sa_family == AF_INET6) {
+			endpoint.addr6 = *(struct sockaddr_in6 *)addr;
 			wg_socket_set_peer_endpoint(peer, &endpoint);
 		}
 	}
-- 
2.37.3

Re: [PATCH net 3/3] wireguard: netlink: avoid variable-sized memcpy on sockaddr

[Kees Cook]

On Fri, Sep 16, 2022 at 03:37:40PM +0100, Jason A. Donenfeld wrote:
> Doing a variable-sized memcpy is slower, and the compiler isn't smart
> enough to turn this into a constant-size assignment.
> 
> Further, Kees' latest fortified memcpy will actually bark, because the
> destination pointer is type sockaddr, not explicitly sockaddr_in or
> sockaddr_in6, so it thinks there's an overflow:
> 
>     memcpy: detected field-spanning write (size 28) of single field
>     "&endpoint.addr" at drivers/net/wireguard/netlink.c:446 (size 16)
> 
> Fix this by just assigning by using explicit casts for each checked
> case.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/net/wireguard/netlink.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
> index d0f3b6d7f408..5c804bcabfe6 100644
> --- a/drivers/net/wireguard/netlink.c
> +++ b/drivers/net/wireguard/netlink.c
> @@ -436,14 +436,13 @@ static int set_peer(struct wg_device *wg, struct nlattr **attrs)
>  	if (attrs[WGPEER_A_ENDPOINT]) {
>  		struct sockaddr *addr = nla_data(attrs[WGPEER_A_ENDPOINT]);
>  		size_t len = nla_len(attrs[WGPEER_A_ENDPOINT]);
> +		struct endpoint endpoint = { { { 0 } } };

FWIW, this is equivalent[1] on all our compiler versions now:

+		struct endpoint endpoint = { };

>  
> -		if ((len == sizeof(struct sockaddr_in) &&
> -		     addr->sa_family == AF_INET) ||
> -		    (len == sizeof(struct sockaddr_in6) &&
> -		     addr->sa_family == AF_INET6)) {
> -			struct endpoint endpoint = { { { 0 } } };
> -
> -			memcpy(&endpoint.addr, addr, len);
> +		if (len == sizeof(struct sockaddr_in) && addr->sa_family == AF_INET) {
> +			endpoint.addr4 = *(struct sockaddr_in *)addr;
> +			wg_socket_set_peer_endpoint(peer, &endpoint);
> +		} else if (len == sizeof(struct sockaddr_in6) && addr->sa_family == AF_INET6) {
> +			endpoint.addr6 = *(struct sockaddr_in6 *)addr;
>  			wg_socket_set_peer_endpoint(peer, &endpoint);
>  		}
>  	}

Ah, sneaky! I like it. :)

Reviewed-by: Kees Cook <keescook@chromium.org>


I wonder if we need a "converter" struct to help with this -- this isn't
the only place this code pattern exists.

struct sockaddr_decode {
	union {
		struct sockaddr		addr;
		struct sockaddr_in	addr4;
		struct sockaddr_in6	addr6;
		DECLARE_FLEX_ARRAY(u8,	content);
	};
};

	struct sockaddr_decode *addr = nla_data(attrs[WGPEER_A_ENDPOINT]);
	...
	if (len == sizeof(addr->addr4) && addr->addr.sa_family == AF_INET) {
		endpoint.addr4 = addr->addr4;
	...


This looks a lot like these open issues we've had for a while:
https://github.com/KSPP/linux/issues/169
https://github.com/KSPP/linux/issues/140

-Kees

[1] https://lore.kernel.org/lkml/20210910225207.3272766-1-keescook@chromium.org/

-- 
Kees Cook

Re: [PATCH net 3/3] wireguard: netlink: avoid variable-sized memcpy on sockaddr

[Kees Cook]

On Fri, Sep 16, 2022 at 03:37:40PM +0100, Jason A. Donenfeld wrote:
> Doing a variable-sized memcpy is slower, and the compiler isn't smart
> enough to turn this into a constant-size assignment.
> 
> Further, Kees' latest fortified memcpy will actually bark, because the
> destination pointer is type sockaddr, not explicitly sockaddr_in or
> sockaddr_in6, so it thinks there's an overflow:
> 
>     memcpy: detected field-spanning write (size 28) of single field
>     "&endpoint.addr" at drivers/net/wireguard/netlink.c:446 (size 16)
> 
> Fix this by just assigning by using explicit casts for each checked
> case.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Oh, also, please include reporter details:

Reported-by: syzbot+a448cda4dba2dac50de5@syzkaller.appspotmail.com


-- 
Kees Cook

Re: [PATCH net 0/3] wireguard patches for 6.0-rc6

[Jakub Kicinski]

On Fri, 16 Sep 2022 15:37:37 +0100 Jason A. Donenfeld wrote:
> Sorry we didn't get a chance to talk at Plumbers. I saw some of you very
> briefly in passing but never had the opportunity to chat. Next time, I
> hope.

Indeed!

> Please pull the following fixes:

You say pull but you mean apply, right?

Re: [PATCH net 0/3] wireguard patches for 6.0-rc6

[Jason A. Donenfeld]

On Mon, Sep 19, 2022 at 06:20:53PM -0700, Jakub Kicinski wrote:
> On Fri, 16 Sep 2022 15:37:37 +0100 Jason A. Donenfeld wrote:
> > Sorry we didn't get a chance to talk at Plumbers. I saw some of you very
> > briefly in passing but never had the opportunity to chat. Next time, I
> > hope.
> 
> Indeed!
> 
> > Please pull the following fixes:
> 
> You say pull but you mean apply, right?

Yes, sorry.

Jason