Re: [syzbot] linux-next test error: WARNING in set_peer

[Jason A. Donenfeld]

I think I'll open code it like below. I'll include this in my next push
to net.

>From 19fb26af00a8266df65b706dc02727c6a608b1b6 Mon Sep 17 00:00:00 2001
From: "Jason A. Donenfeld" 
Date: Wed, 14 Sep 2022 18:42:00 +0100
Subject: [PATCH] wireguard: netlink: avoid variable-sized memcpy on sockaddr

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
"" 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 
Signed-off-by: Jason A. Donenfeld 
---
 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(, 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, );
+   } else if (len == sizeof(struct sockaddr_in6) && 
addr->sa_family == AF_INET6) {
+   endpoint.addr6 = *(struct sockaddr_in6 *)addr;
wg_socket_set_peer_endpoint(peer, );
}
}
--
2.37.3

Re: [syzbot] linux-next test error: WARNING in set_peer

[Kees Cook]

On Tue, Sep 13, 2022 at 12:51:42PM -0700, syzbot wrote:
> memcpy: detected field-spanning write (size 28) of single field 
> "" at drivers/net/wireguard/netlink.c:446 (size 16)

This is one way to fix it:

diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
index 0c0644e762e5..dbbeba216530 100644
--- a/drivers/net/wireguard/netlink.c
+++ b/drivers/net/wireguard/netlink.c
@@ -434,16 +434,16 @@ 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]);
+   struct endpoint *raw = nla_data(attrs[WGPEER_A_ENDPOINT]);
size_t len = nla_len(attrs[WGPEER_A_ENDPOINT]);
 
if ((len == sizeof(struct sockaddr_in) &&
-addr->sa_family == AF_INET) ||
+raw->addr.sa_family == AF_INET) ||
(len == sizeof(struct sockaddr_in6) &&
-addr->sa_family == AF_INET6)) {
+raw->addr.sa_family == AF_INET6)) {
struct endpoint endpoint = { { { 0 } } };
 
-   memcpy(, addr, len);
+   memcpy(, >addrs, len);
wg_socket_set_peer_endpoint(peer, );
}
}
diff --git a/drivers/net/wireguard/peer.h b/drivers/net/wireguard/peer.h
index 76e4d3128ad4..4fbe7940828b 100644
--- a/drivers/net/wireguard/peer.h
+++ b/drivers/net/wireguard/peer.h
@@ -19,11 +19,13 @@
 struct wg_device;
 
 struct endpoint {
-   union {
-   struct sockaddr addr;
-   struct sockaddr_in addr4;
-   struct sockaddr_in6 addr6;
-   };
+   struct_group(addrs,
+   union {
+   struct sockaddr addr;
+   struct sockaddr_in addr4;
+   struct sockaddr_in6 addr6;
+   };
+   );
union {
struct {
struct in_addr src4;


diffoscope shows the bounds check gets updated to the full union size:
│ - cmp$0x11,%edx
│ + cmp$0x1d,%edx

and the field name changes in the warning:
$ strings clang/drivers/net/wireguard/netlink.o.after | grep ^field
field "" at drivers/net/wireguard/netlink.c:446


-- 
Kees Cook