[PATCH bpf v2 0/2] Fix bpf_skb_load_bytes_relative for cgroup_skb/egress

[PATCH bpf v2 0/2] Fix bpf_skb_load_bytes_relative for cgroup_skb/egress

[YiFei Zhu]

When cgroup_skb/egress triggers the MAC header is not set. On the other hand,
load_bytes_relative unconditionally calls skb_mac_header which, when MC not
set, returns a pointer after the tail pointer, breaking the logic even if the
caller requested the NET header.

Fix is to conditionally use skb_mac_header or skb_network_header depending on
the requested header, -EFAULT when the header is not set. Added a test that
asserts during cgroup_skb/egress request for MAC header returns -EFAULT and
request for NET header succeeds.

Updates since v1:
* Reverted the bound condition check to account for bad offset parameter
  larger than data length.
* Add test asssertion for failure return code on the condition above.

YiFei Zhu (2):
  net/filter: Permit reading NET in load_bytes_relative when MAC not set
  selftests/bpf: Add cgroup_skb/egress test for load_bytes_relative

 net/core/filter.c                             | 16 +++--
 .../bpf/prog_tests/load_bytes_relative.c      | 71 +++++++++++++++++++
 .../selftests/bpf/progs/load_bytes_relative.c | 48 +++++++++++++
 3 files changed, 128 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c
 create mode 100644 tools/testing/selftests/bpf/progs/load_bytes_relative.c

--
2.27.0

[PATCH bpf v2 1/2] net/filter: Permit reading NET in load_bytes_relative when MAC not set

[YiFei Zhu]

Added a check in the switch case on start_header that checks for
the existence of the header, and in the case that MAC is not set
and the caller requests for MAC, -EFAULT. If the caller requests
for NET then MAC's existence is completely ignored.

There is no function to check NET header's existence and as far
as cgroup_skb/egress is concerned it should always be set.

Removed for ptr >= the start of header, considering offset is
bounded unsigned and should always be true. len <= end - mac is
redundant to ptr + len <= end.

Fixes: 3eee1f75f2b9 ("bpf: fix bpf_skb_load_bytes_relative pkt length check")
Reviewed-by: Stanislav Fomichev <sdf@google.com>
Signed-off-by: YiFei Zhu <zhuyifei@google.com>
---
 net/core/filter.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index d01a244b5087..6cd82c9539a0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1755,25 +1755,27 @@ BPF_CALL_5(bpf_skb_load_bytes_relative, const struct sk_buff *, skb,
 	   u32, offset, void *, to, u32, len, u32, start_header)
 {
 	u8 *end = skb_tail_pointer(skb);
-	u8 *net = skb_network_header(skb);
-	u8 *mac = skb_mac_header(skb);
-	u8 *ptr;
+	u8 *start, *ptr;
 
-	if (unlikely(offset > 0xffff || len > (end - mac)))
+	if (unlikely(offset > 0xffff))
 		goto err_clear;
 
 	switch (start_header) {
 	case BPF_HDR_START_MAC:
-		ptr = mac + offset;
+		if (unlikely(!skb_mac_header_was_set(skb)))
+			goto err_clear;
+		start = skb_mac_header(skb);
 		break;
 	case BPF_HDR_START_NET:
-		ptr = net + offset;
+		start = skb_network_header(skb);
 		break;
 	default:
 		goto err_clear;
 	}
 
-	if (likely(ptr >= mac && ptr + len <= end)) {
+	ptr = start + offset;
+
+	if (likely(ptr + len <= end)) {
 		memcpy(to, ptr, len);
 		return 0;
 	}
-- 
2.27.0

[PATCH bpf v2 2/2] selftests/bpf: Add cgroup_skb/egress test for load_bytes_relative

[YiFei Zhu]

When cgroup_skb/egress triggers the MAC header is not set. Added a
test that asserts reading MAC header is a -EFAULT but NET header
succeeds. The test result from within the eBPF program is stored in
an 1-element array map that the userspace then reads and asserts on.

Another assertion is added that reading from a large offset, past
the end of packet, returns -EFAULT.

Reviewed-by: Stanislav Fomichev <sdf@google.com>
Signed-off-by: YiFei Zhu <zhuyifei@google.com>
---
 .../bpf/prog_tests/load_bytes_relative.c      | 71 +++++++++++++++++++
 .../selftests/bpf/progs/load_bytes_relative.c | 48 +++++++++++++
 2 files changed, 119 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c
 create mode 100644 tools/testing/selftests/bpf/progs/load_bytes_relative.c

diff --git a/tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c b/tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c
new file mode 100644
index 000000000000..c1168e4a9036
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright 2020 Google LLC.
+ */
+
+#include <test_progs.h>
+#include <network_helpers.h>
+
+void test_load_bytes_relative(void)
+{
+	int server_fd, cgroup_fd, prog_fd, map_fd, client_fd;
+	int err;
+	struct bpf_object *obj;
+	struct bpf_program *prog;
+	struct bpf_map *test_result;
+	__u32 duration = 0;
+
+	__u32 map_key = 0;
+	__u32 map_value = 0;
+
+	cgroup_fd = test__join_cgroup("/load_bytes_relative");
+	if (CHECK_FAIL(cgroup_fd < 0))
+		return;
+
+	server_fd = start_server(AF_INET, SOCK_STREAM);
+	if (CHECK_FAIL(server_fd < 0))
+		goto close_cgroup_fd;
+
+	err = bpf_prog_load("./load_bytes_relative.o", BPF_PROG_TYPE_CGROUP_SKB,
+			    &obj, &prog_fd);
+	if (CHECK_FAIL(err))
+		goto close_server_fd;
+
+	test_result = bpf_object__find_map_by_name(obj, "test_result");
+	if (CHECK_FAIL(!test_result))
+		goto close_bpf_object;
+
+	map_fd = bpf_map__fd(test_result);
+	if (map_fd < 0)
+		goto close_bpf_object;
+
+	prog = bpf_object__find_program_by_name(obj, "load_bytes_relative");
+	if (CHECK_FAIL(!prog))
+		goto close_bpf_object;
+
+	err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS,
+			      BPF_F_ALLOW_MULTI);
+	if (CHECK_FAIL(err))
+		goto close_bpf_object;
+
+	client_fd = connect_to_fd(AF_INET, SOCK_STREAM, server_fd);
+	if (CHECK_FAIL(client_fd < 0))
+		goto close_bpf_object;
+	close(client_fd);
+
+	err = bpf_map_lookup_elem(map_fd, &map_key, &map_value);
+	if (CHECK_FAIL(err))
+		goto close_bpf_object;
+
+	CHECK(map_value != 1, "bpf", "bpf program returned failure");
+
+close_bpf_object:
+	bpf_object__close(obj);
+
+close_server_fd:
+	close(server_fd);
+
+close_cgroup_fd:
+	close(cgroup_fd);
+}
diff --git a/tools/testing/selftests/bpf/progs/load_bytes_relative.c b/tools/testing/selftests/bpf/progs/load_bytes_relative.c
new file mode 100644
index 000000000000..dc1d04a7a3d6
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/load_bytes_relative.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright 2020 Google LLC.
+ */
+
+#include <errno.h>
+#include <linux/bpf.h>
+#include <linux/if_ether.h>
+#include <linux/ip.h>
+#include <bpf/bpf_helpers.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u32);
+} test_result SEC(".maps");
+
+SEC("cgroup_skb/egress")
+int load_bytes_relative(struct __sk_buff *skb)
+{
+	struct ethhdr eth;
+	struct iphdr iph;
+
+	__u32 map_key = 0;
+	__u32 test_passed = 0;
+
+	/* MAC header is not set by the time cgroup_skb/egress triggers */
+	if (bpf_skb_load_bytes_relative(skb, 0, &eth, sizeof(eth),
+					BPF_HDR_START_MAC) != -EFAULT)
+		goto fail;
+
+	if (bpf_skb_load_bytes_relative(skb, 0, &iph, sizeof(iph),
+					BPF_HDR_START_NET))
+		goto fail;
+
+	if (bpf_skb_load_bytes_relative(skb, 0xffff, &iph, sizeof(iph),
+					BPF_HDR_START_NET) != -EFAULT)
+		goto fail;
+
+	test_passed = 1;
+
+fail:
+	bpf_map_update_elem(&test_result, &map_key, &test_passed, BPF_ANY);
+
+	return 1;
+}
-- 
2.27.0

Re: [PATCH bpf v2 0/2] Fix bpf_skb_load_bytes_relative for cgroup_skb/egress

[Daniel Borkmann]

On 6/10/20 8:41 PM, YiFei Zhu wrote:
> When cgroup_skb/egress triggers the MAC header is not set. On the other hand,
> load_bytes_relative unconditionally calls skb_mac_header which, when MC not
> set, returns a pointer after the tail pointer, breaking the logic even if the
> caller requested the NET header.
> 
> Fix is to conditionally use skb_mac_header or skb_network_header depending on
> the requested header, -EFAULT when the header is not set. Added a test that
> asserts during cgroup_skb/egress request for MAC header returns -EFAULT and
> request for NET header succeeds.
> 
> Updates since v1:
> * Reverted the bound condition check to account for bad offset parameter
>    larger than data length.
> * Add test asssertion for failure return code on the condition above.
> 
> YiFei Zhu (2):
>    net/filter: Permit reading NET in load_bytes_relative when MAC not set
>    selftests/bpf: Add cgroup_skb/egress test for load_bytes_relative
> 
>   net/core/filter.c                             | 16 +++--
>   .../bpf/prog_tests/load_bytes_relative.c      | 71 +++++++++++++++++++
>   .../selftests/bpf/progs/load_bytes_relative.c | 48 +++++++++++++
>   3 files changed, 128 insertions(+), 7 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c
>   create mode 100644 tools/testing/selftests/bpf/progs/load_bytes_relative.c
> 
> --
> 2.27.0
> 

Applied, thanks!