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

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 0caac1da9949 Add linux-next specific files for 20220913
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=172d78d8880000
kernel config: https://syzkaller.appspot.com/x/.config?x=2fd6142ea1cf631c
dashboard link: https://syzkaller.appspot.com/bug?extid=a448cda4dba2dac50de5
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/4916ab25f774/disk-0caac1da.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/16dace3b273b/vmlinux-0caac1da.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+a448cd...@syzkaller.appspotmail.com

netdevsim netdevsim0 netdevsim1: renamed from eth1
netdevsim netdevsim0 netdevsim2: renamed from eth2
netdevsim netdevsim0 netdevsim3: renamed from eth3
------------[ cut here ]------------
memcpy: detected field-spanning write (size 28) of single field "&endpoint.addr" at drivers/net/wireguard/netlink.c:446 (size 16)
WARNING: CPU: 0 PID: 3616 at drivers/net/wireguard/netlink.c:446 set_peer+0x991/0x10c0 drivers/net/wireguard/netlink.c:446
Modules linked in:
CPU: 0 PID: 3616 Comm: syz-executor.0 Not tainted 6.0.0-rc5-next-20220913-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/26/2022
RIP: 0010:set_peer+0x991/0x10c0 drivers/net/wireguard/netlink.c:446
Code: 00 e8 63 30 b3 fc b9 10 00 00 00 48 c7 c2 00 4c 72 8a be 1c 00 00 00 48 c7 c7 60 4c 72 8a c6 05 d0 e7 02 09 01 e8 f1 d7 74 04 <0f> 0b e9 03 04 00 00 e8 33 30 b3 fc 89 ee 44 89 ef e8 79 2c b3 fc
RSP: 0018:ffffc90003d4f540 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffffc90003d4f6d8 RCX: 0000000000000000
RDX: ffff888072ed57c0 RSI: ffffffff81611eb8 RDI: fffff520007a9e9a
RBP: ffffc90003d4f5e8 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 7720676e696e6e6d R12: 000000000000001c
R13: 0000000000000000 R14: ffff888072f1d104 R15: ffff888024cb0960
FS: 000055555616b400(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fa5644d32c0 CR3: 000000006e43c000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
wg_set_device+0x8d7/0x11b0 drivers/net/wireguard/netlink.c:589
genl_family_rcv_msg_doit+0x228/0x320 net/netlink/genetlink.c:731
genl_family_rcv_msg net/netlink/genetlink.c:778 [inline]
genl_rcv_msg+0x3b7/0x630 net/netlink/genetlink.c:795
netlink_rcv_skb+0x153/0x420 net/netlink/af_netlink.c:2540
genl_rcv+0x24/0x40 net/netlink/genetlink.c:806
netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
netlink_unicast+0x543/0x7f0 net/netlink/af_netlink.c:1345
netlink_sendmsg+0x917/0xe10 net/netlink/af_netlink.c:1921
sock_sendmsg_nosec net/socket.c:714 [inline]
sock_sendmsg+0xcf/0x120 net/socket.c:734
__sys_sendto+0x236/0x340 net/socket.c:2117
__do_sys_sendto net/socket.c:2129 [inline]
__se_sys_sendto net/socket.c:2125 [inline]
__x64_sys_sendto+0xdd/0x1b0 net/socket.c:2125
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fa56343c18c
Code: fa fa ff ff 44 8b 4c 24 2c 4c 8b 44 24 20 89 c5 44 8b 54 24 28 48 8b 54 24 18 b8 2c 00 00 00 48 8b 74 24 10 8b 7c 24 08 0f 05 <48> 3d 00 f0 ff ff 77 34 89 ef 48 89 44 24 08 e8 20 fb ff ff 48 8b
RSP: 002b:00007ffe4bc97580 EFLAGS: 00000293 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 00007fa5644d4320 RCX: 00007fa56343c18c
RDX: 0000000000000170 RSI: 00007fa5644d4370 RDI: 0000000000000005
RBP: 0000000000000000 R08: 00007ffe4bc975d4 R09: 000000000000000c
R10: 0000000000000000 R11: 0000000000000293 R12: 0000000000000000
R13: 00007fa5644d4370 R14: 0000000000000005 R15: 0000000000000000
</TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

[Kees Cook]

On Tue, Sep 13, 2022 at 12:51:42PM -0700, syzbot wrote:
> memcpy: detected field-spanning write (size 28) of single field "&endpoint.addr" 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(&endpoint.addr, addr, len);
+ memcpy(&endpoint.addrs, &raw->addrs, len);
wg_socket_set_peer_endpoint(peer, &endpoint);
}
}
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 "&endpoint.addrs" at drivers/net/wireguard/netlink.c:446


--
Kees Cook

[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" <Ja...@zx2c4.com>
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
"&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 <kees...@chromium.org>
Signed-off-by: Jason A. Donenfeld <Ja...@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