| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87r0c2nai8.fsf@cloudflare.com>
Date: Tue, 09 Jul 2024 12:08:15 +0200
From: Jakub Sitnicki <jakub@...udflare.com>
To: Michal Luczaj <mhal@...x.co>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
john.fastabend@...il.com, kuniyu@...zon.com, Rao.Shoaib@...cle.com,
cong.wang@...edance.com
Subject: Re: [PATCH bpf v3 4/4] selftest/bpf: Test sockmap redirect for
AF_UNIX MSG_OOB
On Sun, Jul 07, 2024 at 11:28 PM +02, Michal Luczaj wrote:
> Verify that out-of-band packets are silently dropped before they reach the
> redirection logic. Attempt to recv() stale data that might have been
> erroneously left reachable from the original socket.
>
> The idea is to test with a 2 byte long send(). Should a MSG_OOB flag be in
> use, only the last byte will be treated as out-of-band. Test fails if
> verd_mapfd indicates a wrong number of packets processed (e.g. if OOB data
> wasn't dropped at the source) or if it was still somehow possble to recv()
Nit: typo s/possble/possible
Something like below will catch these for you:
$ cat ~/src/linux/.git/hooks/post-commit
exec git show --format=email HEAD | ./scripts/checkpatch.pl --strict --codespell
> OOB from the mapped socket.
>
> Signed-off-by: Michal Luczaj <mhal@...x.co>
> ---
> .../selftests/bpf/prog_tests/sockmap_listen.c | 26 ++++++++++++++++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> index 59e16f8f2090..878fcca36a55 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -1397,10 +1397,10 @@ static void __pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
> return;
> }
>
> - n = xsend(cli1, "a", 1, send_flags);
> - if (n == 0)
> + n = xsend(cli1, "ab", 2, send_flags);
> + if (n >= 0 && n < 2)
> FAIL("%s: incomplete send", log_prefix);
> - if (n < 1)
> + if (n < 2)
> return;
>
> key = SK_PASS;
> @@ -1415,6 +1415,19 @@ static void __pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
> FAIL_ERRNO("%s: recv_timeout", log_prefix);
> if (n == 0)
> FAIL("%s: incomplete recv", log_prefix);
> +
> + if (send_flags & MSG_OOB) {
> + key = 0;
> + xbpf_map_delete_elem(sock_mapfd, &key);
> + key = 1;
> + xbpf_map_delete_elem(sock_mapfd, &key);
> +
> + n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
> + if (n > 0)
> + FAIL("%s: recv(MSG_OOB) succeeded", log_prefix);
> + if (n == 0)
> + FAIL("%s: recv(MSG_OOB) returned 0", log_prefix);
> + }
> }
>
> static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
> @@ -1883,6 +1896,10 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
> unix_inet_redir_to_connected(family, SOCK_STREAM,
> sock_map, nop_map, verdict_map,
> REDIR_EGRESS);
> + __unix_inet_redir_to_connected(family, SOCK_STREAM,
> + sock_map, nop_map, verdict_map,
> + REDIR_EGRESS, MSG_OOB);
> +
> skel->bss->test_ingress = true;
> unix_inet_redir_to_connected(family, SOCK_DGRAM,
> sock_map, -1, verdict_map,
> @@ -1897,6 +1914,9 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
> unix_inet_redir_to_connected(family, SOCK_STREAM,
> sock_map, nop_map, verdict_map,
> REDIR_INGRESS);
> + __unix_inet_redir_to_connected(family, SOCK_STREAM,
> + sock_map, nop_map, verdict_map,
> + REDIR_INGRESS, MSG_OOB);
>
> xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
> }
This AF_UNIX MSG_OOB use case is super exotic, IMO. TBH, I've just
learned about it. Hence, I think we could use some more comments for the
future readers.
Also, it seems like we only need to remove peer1 from sockmap to test
the behavior. If so, I'd stick to just what is needed to set up the
test. Extra stuff makes you wonder what was the authors intention.
I'd also be more direct about checking return value & error. These
selftests often serve as the only example / API documentation out there.
--8<--
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 25938e66a3c1..1e30e6861805 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1399,6 +1399,7 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
return;
}
+ /* Last byte is OOB data when send_flags has MSG_OOB bit set */
n = xsend(cli1, "ab", 2, send_flags);
if (n >= 0 && n < 2)
FAIL("%s: incomplete send", log_prefix);
@@ -1419,16 +1420,22 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
FAIL("%s: incomplete recv", log_prefix);
if (send_flags & MSG_OOB) {
- key = 0;
- xbpf_map_delete_elem(sock_mapfd, &key);
- key = 1;
- xbpf_map_delete_elem(sock_mapfd, &key);
+ /* Check that we can't read OOB while in sockmap */
+ errno = 0;
+ n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
+ if (n != -1 || errno != EOPNOTSUPP)
+ FAIL("%s: recv(MSG_OOB): expected EOPNOTSUPP: retval=%d errno=%d",
+ log_prefix, n, errno);
+
+ /* Remove peer1 from sockmap */
+ xbpf_map_delete_elem(sock_mapfd, &(int){ 1 });
+ /* Check that OOB was dropped on redirect */
+ errno = 0;
n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
- if (n > 0)
- FAIL("%s: recv(MSG_OOB) succeeded", log_prefix);
- if (n == 0)
- FAIL("%s: recv(MSG_OOB) returned 0", log_prefix);
+ if (n != -1 || errno != EINVAL)
+ FAIL("%s: recv(MSG_OOB): expected EINVAL: retval=%d errno=%d",
+ log_prefix, n, errno);
}
}
@@ -1882,9 +1889,11 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
unix_inet_redir_to_connected(family, SOCK_STREAM,
sock_map, nop_map, verdict_map,
REDIR_EGRESS, NO_FLAGS);
- __unix_inet_redir_to_connected(family, SOCK_STREAM,
- sock_map, nop_map, verdict_map,
- REDIR_EGRESS, MSG_OOB);
+
+ /* MSG_OOB not supported by AF_UNIX SOCK_DGRAM */
+ unix_inet_redir_to_connected(family, SOCK_STREAM,
+ sock_map, nop_map, verdict_map,
+ REDIR_EGRESS, MSG_OOB);
skel->bss->test_ingress = true;
unix_inet_redir_to_connected(family, SOCK_DGRAM,
@@ -1900,9 +1909,11 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
unix_inet_redir_to_connected(family, SOCK_STREAM,
sock_map, nop_map, verdict_map,
REDIR_INGRESS, NO_FLAGS);
- __unix_inet_redir_to_connected(family, SOCK_STREAM,
- sock_map, nop_map, verdict_map,
- REDIR_INGRESS, MSG_OOB);
+
+ /* MSG_OOB not supported by AF_UNIX SOCK_DGRAM */
+ unix_inet_redir_to_connected(family, SOCK_STREAM,
+ sock_map, nop_map, verdict_map,
+ REDIR_INGRESS, MSG_OOB);
xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
}
Powered by blists - more mailing lists