Skip to contentSkip to conversation

bpf: Fix [::] -> [::1] rewrite in sys_sendmsg

PATCH Superseded Andrey Ignatov 1 participant 4 messages Jan 3, 2019 – Jan 4, 2019 1 message not in the archive

Subsystems (6)
Networking Network drivers Kselftest BPF +2 more subsystems

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Andrey Ignatov

author
#0 lore raw

cover bpf: Fix [::] -> [::1] rewrite in sys_sendmsg

The patch set fixes BSD'ism in sys_sendmsg to rewrite unspecified destination IPv6 for unconnected UDP sockets in sys_sendmsg with [::1] in case when either CONFIG_CGROUP_BPF is enabled or when sys_sendmsg BPF hook sets destination IPv6 to [::].

Patch 1 is the fix and provides more details.
Patch 2 adds two test cases to verify the fix.

Andrey Ignatov (2):
bpf: Fix [::] -> [::1] rewrite in sys_sendmsg
selftests/bpf: Test [::] -> [::1] rewrite in sys_sendmsg in test_sock_addr

 net/ipv6/udp.c                               |  8 +--
 tools/testing/selftests/bpf/test_sock_addr.c | 53 ++++++++++++++++++--
 2 files changed, 54 insertions(+), 7 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 1/2 bpf: Fix [::] -> [::1] rewrite in sys_sendmsg

sys_sendmsg has supported unspecified destination IPv6 (wildcard) for unconnected UDP sockets since 876c7f41. When [::] is passed by user as destination, sys_sendmsg rewrites it with [::1] to be consistent with BSD (see "BSD'ism" comment in the code).

This didn't work when cgroup-bpf was enabled though since the rewrite [::] -> [::1] happened before passing control to cgroup-bpf block where fl6.daddr was updated with passed by user sockaddr_in6.sin6_addr (that might or might not be changed by BPF program). That way if user passed [::] as dst IPv6 it was first rewritten with [::1] by original code from 876c7f41, but then rewritten back with [::] by cgroup-bpf block.

It happened even when BPF_CGROUP_UDP6_SENDMSG program was not present (CONFIG_CGROUP_BPF=y was enough).

The fix is to apply BSD'ism after cgroup-bpf block so that [::] is replaced with [::1] no matter where it came from: passed by user to sys_sendmsg or set by BPF_CGROUP_UDP6_SENDMSG program.

Fixes 1cedee13d25a ("bpf: Hooks for sys_sendmsg")
Reported-by Nitin Rawat <[email protected]>
Signed-off-by Andrey Ignatov <[email protected]>
---
 net/ipv6/udp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
net/ipv6/udp.c +4 -4
index 9cbf363172bd..cd563f985e1a 100644
@@ -1390,10 +1390,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	ipc6.opt = opt;
 
 	fl6.flowi6_proto = sk->sk_protocol;
-	if (!ipv6_addr_any(daddr))
-		fl6.daddr = *daddr;
-	else
-		fl6.daddr.s6_addr[15] = 0x1; /* :: means loopback (BSD'ism) */
+	fl6.daddr = *daddr;
 	if (ipv6_addr_any(&fl6.saddr) && !ipv6_addr_any(&np->saddr))
 		fl6.saddr = np->saddr;
 	fl6.fl6_sport = inet->inet_sport;

@@ -1421,6 +1418,9 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		}
 	}
 
+	if (ipv6_addr_any(fl6.daddr))
+		fl6.daddr.s6_addr[15] = 0x1; /* :: means loopback (BSD'ism) */
+
 	final_p = fl6_update_dst(&fl6, opt, &final);
 	if (final_p)
 		connected = false;
Signature · 1 line
2.17.1

A message that is not in the archive (sent off-list or not yet received): B09C58438338E249BE54C5BE772C588EBDD59B@BGSMSX106.gar.corp.intel.com

author
#3 lore raw

Rawat, Nitin <[email protected]> [Fri, 2019-01-04 00:35 -0800]:

Quoted 11 lines

Hi All,
Andrey,thanks for the fix .
I compiled the patch ½ provided by you , there was a compilation issue in your patch in the below line .

Original line : if (ipv6_addr_any(fl6.daddr))
Expected line:  if (ipv6_addr_any(&fl6.daddr))

I have made the above changes and test case was working fine for unconnected test case .
Please let me know your view on that.

Oops, I had this locally but missed it in the commit while baking the final patch somehow.
Good catch, thanks! I'll send v2 with the fix.

Quoted 13 lines

There is also a another way to solve this way by putting the check just inside the unconnected check , this will leave connected code path untouched Hence I feel would be simpler

(quoted diff) +3 -0
@@ -1343,6 +1343,11 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg,
 size_t len)
                        fl6.fl6_dport = sin6->sin6_port;
                        fl6.daddr = sin6->sin6_addr;
                }
+               if (ipv6_addr_any(&fl6.daddr))
+                {
+                    fl6.daddr.s6_addr[15] = 0x1; /* :: means loopback

(BSD'ism) */

net/ipv6/udp.c int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) · line 1424

Quoted 5 lines
net/ipv6/udp.c +2 -0
+                }
+
        }
 
        final_p = fl6_update_dst(&fl6, opt, &final);

Yeah, it can be fixed like this but I intentionally didn't do it this way since it adds code duplication (copy/paste and checking ipv6_addr_any / setting [::1] twice).

Quoted 71 lines

I have tested second approach and it is working fine for unconnected case. Please let us know your views on the above fix

Regards,
Nitin
-----Original Message-----
From: Andrey Ignatov [mailto:[email protected]]
Sent: Friday, January 4, 2019 4:13 AM
To: [email protected]
Cc: Andrey Ignatov <[email protected]>; [email protected]; [email protected]; Rawat, Nitin <[email protected]>; [email protected]
Subject: [PATCH bpf 1/2] bpf: Fix [::] -> [::1] rewrite in sys_sendmsg

sys_sendmsg has supported unspecified destination IPv6 (wildcard) for unconnected UDP sockets since 876c7f41. When [::] is passed by user as destination, sys_sendmsg rewrites it with [::1] to be consistent with BSD (see "BSD'ism" comment in the code).

This didn't work when cgroup-bpf was enabled though since the rewrite [::] -> [::1] happened before passing control to cgroup-bpf block where fl6.daddr was updated with passed by user sockaddr_in6.sin6_addr (that might or might not be changed by BPF program). That way if user passed [::] as dst IPv6 it was first rewritten with [::1] by original code from 876c7f41, but then rewritten back with [::] by cgroup-bpf block.

It happened even when BPF_CGROUP_UDP6_SENDMSG program was not present (CONFIG_CGROUP_BPF=y was enough).

The fix is to apply BSD'ism after cgroup-bpf block so that [::] is replaced with [::1] no matter where it came from: passed by user to sys_sendmsg or set by BPF_CGROUP_UDP6_SENDMSG program.

Fixes 1cedee13d25a ("bpf: Hooks for sys_sendmsg")
Reported-by Nitin Rawat <[email protected]>
Signed-off-by Andrey Ignatov <[email protected]>

---
net/ipv6/udp.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)
net/ipv6/udp.c index 9cbf363172bd..cd563f985e1a renamed from net/ipv6/udp.c +0 -0

100644

net/ipv6/udp.c +1 -4
@@ -1390,10 +1390,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg,
 size_t len)
         ipc6.opt = opt;
 
         fl6.flowi6_proto = sk->sk_protocol;
-       if (!ipv6_addr_any(daddr))
-               fl6.daddr = *daddr;
-       else
-               fl6.daddr.s6_addr[15] = 0x1; /* :: means loopback (BSD'ism) */
+       fl6.daddr = *daddr;
         if (ipv6_addr_any(&fl6.saddr) && !ipv6_addr_any(&np->saddr))
                 fl6.saddr = np->saddr;

         fl6.fl6_sport = inet->inet_sport;
net/ipv6/udp.c +3 -0
@@ -1421,6 +1418,9 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg,
 size_t len)
                 }
         }
 
+       if (ipv6_addr_any(fl6.daddr))
+               fl6.daddr.s6_addr[15] = 0x1; /* :: means loopback (BSD'ism) */
+
         final_p = fl6_update_dst(&fl6, opt, &final);
         if (final_p)

                 connected = false;

--
2.17.1

Signature · 1 line
Andrey Ignatov
author
#4 lore raw

PATCH 2/2 selftests/bpf: Test [::] -> [::1] rewrite in sys_sendmsg in test_sock_addr

In reply to Andrey Ignatov

Test that sys_sendmsg BPF hook doesn't break sys_sendmsg behaviour to rewrite destination IPv6 = [::] with [::1] (BSD'ism).

Two test cases are added:

1) User passes dst IPv6 = [::] and BPF_CGROUP_UDP6_SENDMSG program doesn't touch it.

2) User passes dst IPv6 != [::], but BPF_CGROUP_UDP6_SENDMSG program rewrites it with [::].

In both cases [::1] is used by sys_sendmsg code eventually and datagram is sent successfully for unconnected UDP socket.

Example of relevant output:
Test case: sendmsg6: set dst IP = [::] (BSD'ism) .. [PASS] Test case: sendmsg6: preserve dst IP = [::] (BSD'ism) .. [PASS]

Signed-off-by Andrey Ignatov <[email protected]>
---
 tools/testing/selftests/bpf/test_sock_addr.c | 53 ++++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)
tools/testing/selftests/bpf/test_sock_addr.c +50 -3
index 73b7493d4120..d94336cbd8bd 100644
@@ -44,6 +44,7 @@
 #define SERV6_V4MAPPED_IP	"::ffff:192.168.0.4"
 #define SRC6_IP			"::1"
 #define SRC6_REWRITE_IP		"::6"
+#define WILDCARD6_IP		"::"
 #define SERV6_PORT		6060
 #define SERV6_REWRITE_PORT	6666
 

@@ -85,12 +86,14 @@ static int bind4_prog_load(const struct sock_addr_test *test);
 static int bind6_prog_load(const struct sock_addr_test *test);
 static int connect4_prog_load(const struct sock_addr_test *test);
 static int connect6_prog_load(const struct sock_addr_test *test);
+static int sendmsg_allow_prog_load(const struct sock_addr_test *test);
 static int sendmsg_deny_prog_load(const struct sock_addr_test *test);
 static int sendmsg4_rw_asm_prog_load(const struct sock_addr_test *test);
 static int sendmsg4_rw_c_prog_load(const struct sock_addr_test *test);
 static int sendmsg6_rw_asm_prog_load(const struct sock_addr_test *test);
 static int sendmsg6_rw_c_prog_load(const struct sock_addr_test *test);
 static int sendmsg6_rw_v4mapped_prog_load(const struct sock_addr_test *test);
+static int sendmsg6_rw_wildcard_prog_load(const struct sock_addr_test *test);
 
 static struct sock_addr_test tests[] = {
 	/* bind */

@@ -462,6 +465,34 @@ static struct sock_addr_test tests[] = {
 		SRC6_REWRITE_IP,
 		SYSCALL_ENOTSUPP,
 	},
+	{
+		"sendmsg6: set dst IP = [::] (BSD'ism)",
+		sendmsg6_rw_wildcard_prog_load,
+		BPF_CGROUP_UDP6_SENDMSG,
+		BPF_CGROUP_UDP6_SENDMSG,
+		AF_INET6,
+		SOCK_DGRAM,
+		SERV6_IP,
+		SERV6_PORT,
+		SERV6_REWRITE_IP,
+		SERV6_REWRITE_PORT,
+		SRC6_REWRITE_IP,
+		SUCCESS,
+	},
+	{
+		"sendmsg6: preserve dst IP = [::] (BSD'ism)",
+		sendmsg_allow_prog_load,
+		BPF_CGROUP_UDP6_SENDMSG,
+		BPF_CGROUP_UDP6_SENDMSG,
+		AF_INET6,
+		SOCK_DGRAM,
+		WILDCARD6_IP,
+		SERV6_PORT,
+		SERV6_REWRITE_IP,
+		SERV6_PORT,
+		SRC6_IP,
+		SUCCESS,
+	},
 	{
 		"sendmsg6: deny call",
 		sendmsg_deny_prog_load,

@@ -734,16 +765,27 @@ static int connect6_prog_load(const struct sock_addr_test *test)
 	return load_path(test, CONNECT6_PROG_PATH);
 }
 
-static int sendmsg_deny_prog_load(const struct sock_addr_test *test)
+static int sendmsg_ret_only_prog_load(const struct sock_addr_test *test,
+				      int32_t rc)
 {
 	struct bpf_insn insns[] = {
-		/* return 0 */
-		BPF_MOV64_IMM(BPF_REG_0, 0),
+		/* return rc */
+		BPF_MOV64_IMM(BPF_REG_0, rc),
 		BPF_EXIT_INSN(),
 	};
 	return load_insns(test, insns, sizeof(insns) / sizeof(struct bpf_insn));
 }
 
+static int sendmsg_allow_prog_load(const struct sock_addr_test *test)
+{
+	return sendmsg_ret_only_prog_load(test, /*rc*/ 1);
+}
+
+static int sendmsg_deny_prog_load(const struct sock_addr_test *test)
+{
+	return sendmsg_ret_only_prog_load(test, /*rc*/ 0);
+}
+
 static int sendmsg4_rw_asm_prog_load(const struct sock_addr_test *test)
 {
 	struct sockaddr_in dst4_rw_addr;

@@ -864,6 +906,11 @@ static int sendmsg6_rw_v4mapped_prog_load(const struct sock_addr_test *test)
 	return sendmsg6_rw_dst_asm_prog_load(test, SERV6_V4MAPPED_IP);
 }
 
+static int sendmsg6_rw_wildcard_prog_load(const struct sock_addr_test *test)
+{
+	return sendmsg6_rw_dst_asm_prog_load(test, WILDCARD6_IP);
+}
+
 static int sendmsg6_rw_c_prog_load(const struct sock_addr_test *test)
 {
 	return load_path(test, SENDMSG6_PROG_PATH);
Signature · 1 line
2.17.1