* [PATCH bpf 0/2] Fix bpf_skb_load_bytes_relative for cgroup_skb/egress
@ 2020-06-05 0:07 YiFei Zhu
2020-06-05 0:07 ` [PATCH bpf 1/2] net/filter: Permit reading NET in load_bytes_relative when MAC not set YiFei Zhu
2020-06-05 0:07 ` [PATCH bpf 2/2] selftests/bpf: Add cgroup_skb/egress test for load_bytes_relative YiFei Zhu
0 siblings, 2 replies; 5+ messages in thread
From: YiFei Zhu @ 2020-06-05 0:07 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev, 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.
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 | 44 ++++++++++++
3 files changed, 124 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH bpf 1/2] net/filter: Permit reading NET in load_bytes_relative when MAC not set
2020-06-05 0:07 [PATCH bpf 0/2] Fix bpf_skb_load_bytes_relative for cgroup_skb/egress YiFei Zhu
@ 2020-06-05 0:07 ` YiFei Zhu
2020-06-08 13:56 ` Daniel Borkmann
2020-06-05 0:07 ` [PATCH bpf 2/2] selftests/bpf: Add cgroup_skb/egress test for load_bytes_relative YiFei Zhu
1 sibling, 1 reply; 5+ messages in thread
From: YiFei Zhu @ 2020-06-05 0:07 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev, 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. ptr + len <= end is
overflow-unsafe and replaced with len <= end - ptr, and
len <= end - mac is redundant to this condition.
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..d3e8445b5494 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(len <= end - ptr)) {
memcpy(to, ptr, len);
return 0;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH bpf 2/2] selftests/bpf: Add cgroup_skb/egress test for load_bytes_relative
2020-06-05 0:07 [PATCH bpf 0/2] Fix bpf_skb_load_bytes_relative for cgroup_skb/egress YiFei Zhu
2020-06-05 0:07 ` [PATCH bpf 1/2] net/filter: Permit reading NET in load_bytes_relative when MAC not set YiFei Zhu
@ 2020-06-05 0:07 ` YiFei Zhu
1 sibling, 0 replies; 5+ messages in thread
From: YiFei Zhu @ 2020-06-05 0:07 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev, 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.
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 | 44 ++++++++++++
2 files changed, 115 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..4311d406d75f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/load_bytes_relative.c
@@ -0,0 +1,44 @@
+// 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, ð, 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;
+
+ test_passed = 1;
+
+fail:
+ bpf_map_update_elem(&test_result, &map_key, &test_passed, BPF_ANY);
+
+ return 1;
+}
--
2.27.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf 1/2] net/filter: Permit reading NET in load_bytes_relative when MAC not set
2020-06-05 0:07 ` [PATCH bpf 1/2] net/filter: Permit reading NET in load_bytes_relative when MAC not set YiFei Zhu
@ 2020-06-08 13:56 ` Daniel Borkmann
2020-06-08 15:54 ` YiFei Zhu
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Borkmann @ 2020-06-08 13:56 UTC (permalink / raw)
To: YiFei Zhu, bpf; +Cc: Alexei Starovoitov, Stanislav Fomichev, YiFei Zhu
On 6/5/20 2:07 AM, YiFei Zhu wrote:
> 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. ptr + len <= end is
> overflow-unsafe and replaced with len <= end - ptr, and
> len <= end - mac is redundant to this condition.
>
> 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..d3e8445b5494 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(len <= end - ptr)) {
Couldn't you run into the case above where the passed offset is large enough
that start + offset goes beyond end pointer [and then above comparison is
performed as unsigned ..]? (At least on x86-64, the 'ptr + len <= end' should
never have an issue [0].) Either way, maybe lets add a test in 2/2 to assert
correct behavior there.
[0] https://www.kernel.org/doc/Documentation/x86/x86_64/mm.txt
> memcpy(to, ptr, len);
> return 0;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf 1/2] net/filter: Permit reading NET in load_bytes_relative when MAC not set
2020-06-08 13:56 ` Daniel Borkmann
@ 2020-06-08 15:54 ` YiFei Zhu
0 siblings, 0 replies; 5+ messages in thread
From: YiFei Zhu @ 2020-06-08 15:54 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: YiFei Zhu, bpf, Alexei Starovoitov, Stanislav Fomichev
On Mon, Jun 8, 2020 at 8:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> Couldn't you run into the case above where the passed offset is large enough
> that start + offset goes beyond end pointer [and then above comparison is
> performed as unsigned ..]?
You are right. I missed that offset would be large and make start +
offset > end,
when I was trying to reason the offsets and overflows. I just checked
that on x86_64
it emits a 'jg' instruction on x86_64, and the test I tried with
offset = 0xffff does
return -EFAULT. However, I searched around and saw that this is due to integer
promotion of len and the test would fail (i.e. not returning -EFAULT) on x86_32
(I have not tested this).
> (At least on x86-64, the 'ptr + len <= end' should
> never have an issue [0].)
Alright, I see that len is an ARG_CONST_SIZE, which would be checked by
check_helper_mem_access, so it is bound by the stack size. So the argument
against ptr >= start also applies here, correct?
YiFei Zhu
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-08 15:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-05 0:07 [PATCH bpf 0/2] Fix bpf_skb_load_bytes_relative for cgroup_skb/egress YiFei Zhu
2020-06-05 0:07 ` [PATCH bpf 1/2] net/filter: Permit reading NET in load_bytes_relative when MAC not set YiFei Zhu
2020-06-08 13:56 ` Daniel Borkmann
2020-06-08 15:54 ` YiFei Zhu
2020-06-05 0:07 ` [PATCH bpf 2/2] selftests/bpf: Add cgroup_skb/egress test for load_bytes_relative YiFei Zhu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).