* [PATCH bpf 2/4] bpf: Zero ARG_PTR_TO_{LONG,INT} | MEM_UNINIT args in case of error
2024-08-23 22:20 [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps Daniel Borkmann
@ 2024-08-23 22:20 ` Daniel Borkmann
2024-08-26 6:38 ` Shung-Hsi Yu
2024-08-27 23:42 ` Alexei Starovoitov
2024-08-23 22:20 ` [PATCH bpf 3/4] selftests/bpf: Fix ARG_PTR_TO_LONG {half-,}uninitialized test Daniel Borkmann
` (3 subsequent siblings)
4 siblings, 2 replies; 12+ messages in thread
From: Daniel Borkmann @ 2024-08-23 22:20 UTC (permalink / raw)
To: bpf; +Cc: kongln9170, Daniel Borkmann
For all non-tracing helpers which have ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
input arguments, zero the value for the case of an error as otherwise it
could leak memory. For tracing, it is not needed given CAP_PERFMON can
already read all kernel memory anyway.
Fixes: 8a67f2de9b1d ("bpf: expose bpf_strtol and bpf_strtoul to all program types")
Fixes: d7a4cb9b6705 ("bpf: Introduce bpf_strtol and bpf_strtoul helpers")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/helpers.c | 2 ++
kernel/bpf/syscall.c | 1 +
net/core/filter.c | 4 ++++
3 files changed, 7 insertions(+)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 356a58aeb79b..20f6a2b7e708 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -522,6 +522,7 @@ BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags,
long long _res;
int err;
+ *res = 0;
err = __bpf_strtoll(buf, buf_len, flags, &_res);
if (err < 0)
return err;
@@ -548,6 +549,7 @@ BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
bool is_negative;
int err;
+ *res = 0;
err = __bpf_strtoull(buf, buf_len, flags, &_res, &is_negative);
if (err < 0)
return err;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 6d5942a6f41f..f799179fd6c7 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5932,6 +5932,7 @@ static const struct bpf_func_proto bpf_sys_close_proto = {
BPF_CALL_4(bpf_kallsyms_lookup_name, const char *, name, int, name_sz, int, flags, u64 *, res)
{
+ *res = 0;
if (flags)
return -EINVAL;
diff --git a/net/core/filter.c b/net/core/filter.c
index 2ff210cb068c..a25c32da3d6c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6264,6 +6264,8 @@ BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb,
int skb_len, dev_len;
int mtu;
+ *mtu_len = 0;
+
if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
return -EINVAL;
@@ -6313,6 +6315,8 @@ BPF_CALL_5(bpf_xdp_check_mtu, struct xdp_buff *, xdp,
int ret = BPF_MTU_CHK_RET_SUCCESS;
int mtu, dev_len;
+ *mtu_len = 0;
+
/* XDP variant doesn't support multi-buffer segment check (yet) */
if (unlikely(flags))
return -EINVAL;
--
2.43.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf 2/4] bpf: Zero ARG_PTR_TO_{LONG,INT} | MEM_UNINIT args in case of error
2024-08-23 22:20 ` [PATCH bpf 2/4] bpf: Zero ARG_PTR_TO_{LONG,INT} | MEM_UNINIT args in case of error Daniel Borkmann
@ 2024-08-26 6:38 ` Shung-Hsi Yu
2024-08-27 23:42 ` Alexei Starovoitov
1 sibling, 0 replies; 12+ messages in thread
From: Shung-Hsi Yu @ 2024-08-26 6:38 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: bpf, kongln9170
On Sat, Aug 24, 2024 at 12:20:31AM GMT, Daniel Borkmann wrote:
> For all non-tracing helpers which have ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
> input arguments, zero the value for the case of an error as otherwise it
> could leak memory. For tracing, it is not needed given CAP_PERFMON can
> already read all kernel memory anyway.
>
> Fixes: 8a67f2de9b1d ("bpf: expose bpf_strtol and bpf_strtoul to all program types")
> Fixes: d7a4cb9b6705 ("bpf: Introduce bpf_strtol and bpf_strtoul helpers")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
[...]
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf 2/4] bpf: Zero ARG_PTR_TO_{LONG,INT} | MEM_UNINIT args in case of error
2024-08-23 22:20 ` [PATCH bpf 2/4] bpf: Zero ARG_PTR_TO_{LONG,INT} | MEM_UNINIT args in case of error Daniel Borkmann
2024-08-26 6:38 ` Shung-Hsi Yu
@ 2024-08-27 23:42 ` Alexei Starovoitov
1 sibling, 0 replies; 12+ messages in thread
From: Alexei Starovoitov @ 2024-08-27 23:42 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: bpf, kongln9170
On Fri, Aug 23, 2024 at 3:20 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 2ff210cb068c..a25c32da3d6c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6264,6 +6264,8 @@ BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb,
> int skb_len, dev_len;
> int mtu;
>
> + *mtu_len = 0;
> +
> if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
> return -EINVAL;
>
> @@ -6313,6 +6315,8 @@ BPF_CALL_5(bpf_xdp_check_mtu, struct xdp_buff *, xdp,
> int ret = BPF_MTU_CHK_RET_SUCCESS;
> int mtu, dev_len;
>
> + *mtu_len = 0;
> +
> /* XDP variant doesn't support multi-buffer segment check (yet) */
> if (unlikely(flags))
> return -EINVAL;
This looks wrong.
If selftests are not failing because of that they should.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf 3/4] selftests/bpf: Fix ARG_PTR_TO_LONG {half-,}uninitialized test
2024-08-23 22:20 [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps Daniel Borkmann
2024-08-23 22:20 ` [PATCH bpf 2/4] bpf: Zero ARG_PTR_TO_{LONG,INT} | MEM_UNINIT args in case of error Daniel Borkmann
@ 2024-08-23 22:20 ` Daniel Borkmann
2024-08-23 22:20 ` [PATCH bpf 4/4] selftests/bpf: Add a test case to write into .rodata Daniel Borkmann
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Daniel Borkmann @ 2024-08-23 22:20 UTC (permalink / raw)
To: bpf; +Cc: kongln9170, Daniel Borkmann
The assumption of 'in privileged mode reads from uninitialized stack locations
are permitted' is not quite correct since the verifier was probing for read
access rather than write access. Both tests need to be annotated as __success
for privileged and unprivileged.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
tools/testing/selftests/bpf/progs/verifier_int_ptr.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
index 9fc3fae5cd83..87206803c025 100644
--- a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
+++ b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
@@ -8,7 +8,6 @@
SEC("socket")
__description("ARG_PTR_TO_LONG uninitialized")
__success
-__failure_unpriv __msg_unpriv("invalid indirect read from stack R4 off -16+0 size 8")
__naked void arg_ptr_to_long_uninitialized(void)
{
asm volatile (" \
@@ -36,9 +35,7 @@ __naked void arg_ptr_to_long_uninitialized(void)
SEC("socket")
__description("ARG_PTR_TO_LONG half-uninitialized")
-/* in privileged mode reads from uninitialized stack locations are permitted */
-__success __failure_unpriv
-__msg_unpriv("invalid indirect read from stack R4 off -16+4 size 8")
+__success
__retval(0)
__naked void ptr_to_long_half_uninitialized(void)
{
--
2.43.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH bpf 4/4] selftests/bpf: Add a test case to write into .rodata
2024-08-23 22:20 [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps Daniel Borkmann
2024-08-23 22:20 ` [PATCH bpf 2/4] bpf: Zero ARG_PTR_TO_{LONG,INT} | MEM_UNINIT args in case of error Daniel Borkmann
2024-08-23 22:20 ` [PATCH bpf 3/4] selftests/bpf: Fix ARG_PTR_TO_LONG {half-,}uninitialized test Daniel Borkmann
@ 2024-08-23 22:20 ` Daniel Borkmann
2024-08-26 6:39 ` Shung-Hsi Yu
2024-08-27 22:39 ` Andrii Nakryiko
2024-08-26 6:37 ` [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps Shung-Hsi Yu
2024-08-27 22:37 ` Andrii Nakryiko
4 siblings, 2 replies; 12+ messages in thread
From: Daniel Borkmann @ 2024-08-23 22:20 UTC (permalink / raw)
To: bpf; +Cc: kongln9170, Daniel Borkmann
Add a test case which attempts to write into .rodata section of the
BPF program, and for comparison this adds test cases also for .bss
and .data section.
Before fix:
# ./vmtest.sh -- ./test_progs -t verifier_const
[...]
./test_progs -t verifier_const
tester_init:PASS:tester_log_buf 0 nsec
process_subtest:PASS:obj_open_mem 0 nsec
process_subtest:PASS:specs_alloc 0 nsec
run_subtest:PASS:obj_open_mem 0 nsec
run_subtest:FAIL:unexpected_load_success unexpected success: 0
#465/1 verifier_const/rodata: write rejected:FAIL
#465/2 verifier_const/bss: write accepted:OK
#465/3 verifier_const/data: write accepted:OK
#465 verifier_const:FAIL
[...]
After fix:
# ./vmtest.sh -- ./test_progs -t verifier_const
[...]
./test_progs -t verifier_const
#465/1 verifier_const/rodata: write rejected:OK
#465/2 verifier_const/bss: write accepted:OK
#465/3 verifier_const/data: write accepted:OK
#465 verifier_const:OK
[...]
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
.../selftests/bpf/prog_tests/tc_links.c | 1 +
.../selftests/bpf/prog_tests/verifier.c | 2 +
.../selftests/bpf/progs/verifier_const.c | 42 +++++++++++++++++++
3 files changed, 45 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/verifier_const.c
diff --git a/tools/testing/selftests/bpf/prog_tests/tc_links.c b/tools/testing/selftests/bpf/prog_tests/tc_links.c
index 1af9ec1149aa..92c647dfd6f1 100644
--- a/tools/testing/selftests/bpf/prog_tests/tc_links.c
+++ b/tools/testing/selftests/bpf/prog_tests/tc_links.c
@@ -9,6 +9,7 @@
#define ping_cmd "ping -q -c1 -w1 127.0.0.1 > /dev/null"
#include "test_tc_link.skel.h"
+#include "test_const.skel.h"
#include "netlink_helpers.h"
#include "tc_helpers.h"
diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index 9dc3687bc406..c0cb1a145274 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -21,6 +21,7 @@
#include "verifier_cgroup_inv_retcode.skel.h"
#include "verifier_cgroup_skb.skel.h"
#include "verifier_cgroup_storage.skel.h"
+#include "verifier_const.skel.h"
#include "verifier_const_or.skel.h"
#include "verifier_ctx.skel.h"
#include "verifier_ctx_sk_msg.skel.h"
@@ -140,6 +141,7 @@ void test_verifier_cfg(void) { RUN(verifier_cfg); }
void test_verifier_cgroup_inv_retcode(void) { RUN(verifier_cgroup_inv_retcode); }
void test_verifier_cgroup_skb(void) { RUN(verifier_cgroup_skb); }
void test_verifier_cgroup_storage(void) { RUN(verifier_cgroup_storage); }
+void test_verifier_const(void) { RUN(verifier_const); }
void test_verifier_const_or(void) { RUN(verifier_const_or); }
void test_verifier_ctx(void) { RUN(verifier_ctx); }
void test_verifier_ctx_sk_msg(void) { RUN(verifier_ctx_sk_msg); }
diff --git a/tools/testing/selftests/bpf/progs/verifier_const.c b/tools/testing/selftests/bpf/progs/verifier_const.c
new file mode 100644
index 000000000000..81302d9738fa
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_const.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Isovalent */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+const long foo = 42;
+long bar;
+long bart = 96;
+
+SEC("tc/ingress")
+__description("rodata: write rejected")
+__failure __msg("write into map forbidden")
+int tcx1(struct __sk_buff *skb)
+{
+ char buff[] = { '8', '4', '\0' };
+ bpf_strtol(buff, sizeof(buff), 0, (long *)&foo);
+ return TCX_PASS;
+}
+
+SEC("tc/ingress")
+__description("bss: write accepted")
+__success
+int tcx2(struct __sk_buff *skb)
+{
+ char buff[] = { '8', '4', '\0' };
+ bpf_strtol(buff, sizeof(buff), 0, &bar);
+ return TCX_PASS;
+}
+
+SEC("tc/ingress")
+__description("data: write accepted")
+__success
+int tcx3(struct __sk_buff *skb)
+{
+ char buff[] = { '8', '4', '\0' };
+ bpf_strtol(buff, sizeof(buff), 0, &bart);
+ return TCX_PASS;
+}
+
+char LICENSE[] SEC("license") = "GPL";
--
2.43.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf 4/4] selftests/bpf: Add a test case to write into .rodata
2024-08-23 22:20 ` [PATCH bpf 4/4] selftests/bpf: Add a test case to write into .rodata Daniel Borkmann
@ 2024-08-26 6:39 ` Shung-Hsi Yu
2024-08-27 22:39 ` Andrii Nakryiko
1 sibling, 0 replies; 12+ messages in thread
From: Shung-Hsi Yu @ 2024-08-26 6:39 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: bpf, kongln9170
On Sat, Aug 24, 2024 at 12:20:33AM GMT, Daniel Borkmann wrote:
> Add a test case which attempts to write into .rodata section of the
> BPF program, and for comparison this adds test cases also for .bss
> and .data section.
>
> Before fix:
>
> # ./vmtest.sh -- ./test_progs -t verifier_const
> [...]
> ./test_progs -t verifier_const
> tester_init:PASS:tester_log_buf 0 nsec
> process_subtest:PASS:obj_open_mem 0 nsec
> process_subtest:PASS:specs_alloc 0 nsec
> run_subtest:PASS:obj_open_mem 0 nsec
> run_subtest:FAIL:unexpected_load_success unexpected success: 0
> #465/1 verifier_const/rodata: write rejected:FAIL
> #465/2 verifier_const/bss: write accepted:OK
> #465/3 verifier_const/data: write accepted:OK
> #465 verifier_const:FAIL
> [...]
>
> After fix:
>
> # ./vmtest.sh -- ./test_progs -t verifier_const
> [...]
> ./test_progs -t verifier_const
> #465/1 verifier_const/rodata: write rejected:OK
> #465/2 verifier_const/bss: write accepted:OK
> #465/3 verifier_const/data: write accepted:OK
> #465 verifier_const:OK
> [...]
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
[...]
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf 4/4] selftests/bpf: Add a test case to write into .rodata
2024-08-23 22:20 ` [PATCH bpf 4/4] selftests/bpf: Add a test case to write into .rodata Daniel Borkmann
2024-08-26 6:39 ` Shung-Hsi Yu
@ 2024-08-27 22:39 ` Andrii Nakryiko
1 sibling, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2024-08-27 22:39 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: bpf, kongln9170
On Fri, Aug 23, 2024 at 3:20 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Add a test case which attempts to write into .rodata section of the
> BPF program, and for comparison this adds test cases also for .bss
> and .data section.
>
> Before fix:
>
> # ./vmtest.sh -- ./test_progs -t verifier_const
> [...]
> ./test_progs -t verifier_const
> tester_init:PASS:tester_log_buf 0 nsec
> process_subtest:PASS:obj_open_mem 0 nsec
> process_subtest:PASS:specs_alloc 0 nsec
> run_subtest:PASS:obj_open_mem 0 nsec
> run_subtest:FAIL:unexpected_load_success unexpected success: 0
> #465/1 verifier_const/rodata: write rejected:FAIL
> #465/2 verifier_const/bss: write accepted:OK
> #465/3 verifier_const/data: write accepted:OK
> #465 verifier_const:FAIL
> [...]
>
> After fix:
>
> # ./vmtest.sh -- ./test_progs -t verifier_const
> [...]
> ./test_progs -t verifier_const
> #465/1 verifier_const/rodata: write rejected:OK
> #465/2 verifier_const/bss: write accepted:OK
> #465/3 verifier_const/data: write accepted:OK
> #465 verifier_const:OK
> [...]
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> .../selftests/bpf/prog_tests/tc_links.c | 1 +
> .../selftests/bpf/prog_tests/verifier.c | 2 +
> .../selftests/bpf/progs/verifier_const.c | 42 +++++++++++++++++++
> 3 files changed, 45 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/verifier_const.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tc_links.c b/tools/testing/selftests/bpf/prog_tests/tc_links.c
> index 1af9ec1149aa..92c647dfd6f1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tc_links.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tc_links.c
> @@ -9,6 +9,7 @@
> #define ping_cmd "ping -q -c1 -w1 127.0.0.1 > /dev/null"
>
> #include "test_tc_link.skel.h"
> +#include "test_const.skel.h"
>
> #include "netlink_helpers.h"
> #include "tc_helpers.h"
> diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> index 9dc3687bc406..c0cb1a145274 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> @@ -21,6 +21,7 @@
> #include "verifier_cgroup_inv_retcode.skel.h"
> #include "verifier_cgroup_skb.skel.h"
> #include "verifier_cgroup_storage.skel.h"
> +#include "verifier_const.skel.h"
> #include "verifier_const_or.skel.h"
> #include "verifier_ctx.skel.h"
> #include "verifier_ctx_sk_msg.skel.h"
> @@ -140,6 +141,7 @@ void test_verifier_cfg(void) { RUN(verifier_cfg); }
> void test_verifier_cgroup_inv_retcode(void) { RUN(verifier_cgroup_inv_retcode); }
> void test_verifier_cgroup_skb(void) { RUN(verifier_cgroup_skb); }
> void test_verifier_cgroup_storage(void) { RUN(verifier_cgroup_storage); }
> +void test_verifier_const(void) { RUN(verifier_const); }
> void test_verifier_const_or(void) { RUN(verifier_const_or); }
> void test_verifier_ctx(void) { RUN(verifier_ctx); }
> void test_verifier_ctx_sk_msg(void) { RUN(verifier_ctx_sk_msg); }
> diff --git a/tools/testing/selftests/bpf/progs/verifier_const.c b/tools/testing/selftests/bpf/progs/verifier_const.c
> new file mode 100644
> index 000000000000..81302d9738fa
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_const.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Isovalent */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +const long foo = 42;
nit: would be "safer" to mark it as "const volatile long" to prevent
whatever smartness compiler might decide to do
> +long bar;
> +long bart = 96;
> +
> +SEC("tc/ingress")
> +__description("rodata: write rejected")
> +__failure __msg("write into map forbidden")
> +int tcx1(struct __sk_buff *skb)
> +{
> + char buff[] = { '8', '4', '\0' };
> + bpf_strtol(buff, sizeof(buff), 0, (long *)&foo);
> + return TCX_PASS;
> +}
> +
> +SEC("tc/ingress")
> +__description("bss: write accepted")
> +__success
> +int tcx2(struct __sk_buff *skb)
> +{
> + char buff[] = { '8', '4', '\0' };
> + bpf_strtol(buff, sizeof(buff), 0, &bar);
> + return TCX_PASS;
> +}
> +
> +SEC("tc/ingress")
> +__description("data: write accepted")
> +__success
> +int tcx3(struct __sk_buff *skb)
> +{
> + char buff[] = { '8', '4', '\0' };
> + bpf_strtol(buff, sizeof(buff), 0, &bart);
> + return TCX_PASS;
> +}
> +
> +char LICENSE[] SEC("license") = "GPL";
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps
2024-08-23 22:20 [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps Daniel Borkmann
` (2 preceding siblings ...)
2024-08-23 22:20 ` [PATCH bpf 4/4] selftests/bpf: Add a test case to write into .rodata Daniel Borkmann
@ 2024-08-26 6:37 ` Shung-Hsi Yu
2024-08-27 22:37 ` Andrii Nakryiko
4 siblings, 0 replies; 12+ messages in thread
From: Shung-Hsi Yu @ 2024-08-26 6:37 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: bpf, kongln9170
On Sat, Aug 24, 2024 at 12:20:30AM GMT, Daniel Borkmann wrote:
> Lonial found an issue that despite user- and BPF-side frozen BPF map
> (like in case of .rodata), it was still possible to write into it from
> a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
> as arguments.
>
> In check_func_arg() when the argument is as mentioned, the meta->raw_mode
> is never set. Later, check_helper_mem_access(), under the case of
> PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
> subsequent call to check_map_access_type() and given the BPF map is
> read-only it succeeds.
>
> The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
> when results are written into them as opposed to read out of them. The
> latter indicates that it's okay to pass a pointer to uninitialized memory
> as the memory is written to anyway.
>
> Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
> Reported-by: Lonial Con <kongln9170@gmail.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
[...]
check_raw_mode_ok() might need an update as well since it currently does
not take ARG_PTR_TO_{LONG,INT} | MEM_UNINIT into account.
Aside from that LGTM (for this patch).
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
As a future refactoring it seems like we'd be better off turning
ARG_PTR_TO_{LONG,INT} into the more generalized
ARG_PTR_TO_FIXED_SIZE_MEM?
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps
2024-08-23 22:20 [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps Daniel Borkmann
` (3 preceding siblings ...)
2024-08-26 6:37 ` [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps Shung-Hsi Yu
@ 2024-08-27 22:37 ` Andrii Nakryiko
2024-09-04 16:02 ` Daniel Borkmann
4 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2024-08-27 22:37 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: bpf, kongln9170
On Fri, Aug 23, 2024 at 3:21 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Lonial found an issue that despite user- and BPF-side frozen BPF map
> (like in case of .rodata), it was still possible to write into it from
> a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
> as arguments.
>
> In check_func_arg() when the argument is as mentioned, the meta->raw_mode
> is never set. Later, check_helper_mem_access(), under the case of
> PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
> subsequent call to check_map_access_type() and given the BPF map is
> read-only it succeeds.
>
> The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
> when results are written into them as opposed to read out of them. The
> latter indicates that it's okay to pass a pointer to uninitialized memory
> as the memory is written to anyway.
>
> Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
> Reported-by: Lonial Con <kongln9170@gmail.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> kernel/bpf/helpers.c | 4 ++--
> kernel/bpf/syscall.c | 2 +-
> kernel/bpf/verifier.c | 3 ++-
> kernel/trace/bpf_trace.c | 4 ++--
> net/core/filter.c | 4 ++--
> 5 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index b5f0adae8293..356a58aeb79b 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -538,7 +538,7 @@ const struct bpf_func_proto bpf_strtol_proto = {
> .arg1_type = ARG_PTR_TO_MEM | MEM_RDONLY,
> .arg2_type = ARG_CONST_SIZE,
> .arg3_type = ARG_ANYTHING,
> - .arg4_type = ARG_PTR_TO_LONG,
> + .arg4_type = ARG_PTR_TO_LONG | MEM_UNINIT,
> };
>
> BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
> @@ -566,7 +566,7 @@ const struct bpf_func_proto bpf_strtoul_proto = {
> .arg1_type = ARG_PTR_TO_MEM | MEM_RDONLY,
> .arg2_type = ARG_CONST_SIZE,
> .arg3_type = ARG_ANYTHING,
> - .arg4_type = ARG_PTR_TO_LONG,
> + .arg4_type = ARG_PTR_TO_LONG | MEM_UNINIT,
> };
>
> BPF_CALL_3(bpf_strncmp, const char *, s1, u32, s1_sz, const char *, s2)
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index bf6c5f685ea2..6d5942a6f41f 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5952,7 +5952,7 @@ static const struct bpf_func_proto bpf_kallsyms_lookup_name_proto = {
> .arg1_type = ARG_PTR_TO_MEM,
> .arg2_type = ARG_CONST_SIZE_OR_ZERO,
> .arg3_type = ARG_ANYTHING,
> - .arg4_type = ARG_PTR_TO_LONG,
> + .arg4_type = ARG_PTR_TO_LONG | MEM_UNINIT,
> };
>
> static const struct bpf_func_proto *
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d8520095ca03..70b0474e03a6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8877,8 +8877,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> case ARG_PTR_TO_INT:
> case ARG_PTR_TO_LONG:
> {
> - int size = int_ptr_type_to_size(arg_type);
> + int size = int_ptr_type_to_size(base_type(arg_type));
>
> + meta->raw_mode = arg_type & MEM_UNINIT;
given all existing ARG_PTR_TO_{INT,LONG} use cases just write into
that memory, why not just set meta->raw_mode unconditionally and not
touch helper definitions?
also, isn't it suspicious that int_ptr_types have PTR_TO_MAP_KEY in
it? key should always be immutable, so can't be written into, no?
> err = check_helper_mem_access(env, regno, size, false, meta);
> if (err)
> return err;
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index cd098846e251..95c3409ff374 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1226,7 +1226,7 @@ static const struct bpf_func_proto bpf_get_func_arg_proto = {
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> .arg2_type = ARG_ANYTHING,
> - .arg3_type = ARG_PTR_TO_LONG,
> + .arg3_type = ARG_PTR_TO_LONG | MEM_UNINIT,
> };
>
> BPF_CALL_2(get_func_ret, void *, ctx, u64 *, value)
> @@ -1242,7 +1242,7 @@ static const struct bpf_func_proto bpf_get_func_ret_proto = {
> .func = get_func_ret,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> - .arg2_type = ARG_PTR_TO_LONG,
> + .arg2_type = ARG_PTR_TO_LONG | MEM_UNINIT,
> };
>
> BPF_CALL_1(get_func_arg_cnt, void *, ctx)
> diff --git a/net/core/filter.c b/net/core/filter.c
> index f3c72cf86099..2ff210cb068c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6346,7 +6346,7 @@ static const struct bpf_func_proto bpf_skb_check_mtu_proto = {
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> .arg2_type = ARG_ANYTHING,
> - .arg3_type = ARG_PTR_TO_INT,
> + .arg3_type = ARG_PTR_TO_INT | MEM_UNINIT,
> .arg4_type = ARG_ANYTHING,
> .arg5_type = ARG_ANYTHING,
> };
> @@ -6357,7 +6357,7 @@ static const struct bpf_func_proto bpf_xdp_check_mtu_proto = {
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> .arg2_type = ARG_ANYTHING,
> - .arg3_type = ARG_PTR_TO_INT,
> + .arg3_type = ARG_PTR_TO_INT | MEM_UNINIT,
> .arg4_type = ARG_ANYTHING,
> .arg5_type = ARG_ANYTHING,
> };
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps
2024-08-27 22:37 ` Andrii Nakryiko
@ 2024-09-04 16:02 ` Daniel Borkmann
2024-09-04 17:53 ` Andrii Nakryiko
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Borkmann @ 2024-09-04 16:02 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, kongln9170
On 8/28/24 12:37 AM, Andrii Nakryiko wrote:
> On Fri, Aug 23, 2024 at 3:21 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Lonial found an issue that despite user- and BPF-side frozen BPF map
>> (like in case of .rodata), it was still possible to write into it from
>> a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
>> as arguments.
>>
>> In check_func_arg() when the argument is as mentioned, the meta->raw_mode
>> is never set. Later, check_helper_mem_access(), under the case of
>> PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
>> subsequent call to check_map_access_type() and given the BPF map is
>> read-only it succeeds.
>>
>> The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
>> when results are written into them as opposed to read out of them. The
>> latter indicates that it's okay to pass a pointer to uninitialized memory
>> as the memory is written to anyway.
>>
>> Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
>> Reported-by: Lonial Con <kongln9170@gmail.com>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>> kernel/bpf/helpers.c | 4 ++--
>> kernel/bpf/syscall.c | 2 +-
>> kernel/bpf/verifier.c | 3 ++-
>> kernel/trace/bpf_trace.c | 4 ++--
>> net/core/filter.c | 4 ++--
>> 5 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index b5f0adae8293..356a58aeb79b 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -538,7 +538,7 @@ const struct bpf_func_proto bpf_strtol_proto = {
>> .arg1_type = ARG_PTR_TO_MEM | MEM_RDONLY,
>> .arg2_type = ARG_CONST_SIZE,
>> .arg3_type = ARG_ANYTHING,
>> - .arg4_type = ARG_PTR_TO_LONG,
>> + .arg4_type = ARG_PTR_TO_LONG | MEM_UNINIT,
>> };
>>
>> BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
>> @@ -566,7 +566,7 @@ const struct bpf_func_proto bpf_strtoul_proto = {
>> .arg1_type = ARG_PTR_TO_MEM | MEM_RDONLY,
>> .arg2_type = ARG_CONST_SIZE,
>> .arg3_type = ARG_ANYTHING,
>> - .arg4_type = ARG_PTR_TO_LONG,
>> + .arg4_type = ARG_PTR_TO_LONG | MEM_UNINIT,
>> };
>>
>> BPF_CALL_3(bpf_strncmp, const char *, s1, u32, s1_sz, const char *, s2)
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index bf6c5f685ea2..6d5942a6f41f 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -5952,7 +5952,7 @@ static const struct bpf_func_proto bpf_kallsyms_lookup_name_proto = {
>> .arg1_type = ARG_PTR_TO_MEM,
>> .arg2_type = ARG_CONST_SIZE_OR_ZERO,
>> .arg3_type = ARG_ANYTHING,
>> - .arg4_type = ARG_PTR_TO_LONG,
>> + .arg4_type = ARG_PTR_TO_LONG | MEM_UNINIT,
>> };
>>
>> static const struct bpf_func_proto *
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index d8520095ca03..70b0474e03a6 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -8877,8 +8877,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>> case ARG_PTR_TO_INT:
>> case ARG_PTR_TO_LONG:
>> {
>> - int size = int_ptr_type_to_size(arg_type);
>> + int size = int_ptr_type_to_size(base_type(arg_type));
>>
>> + meta->raw_mode = arg_type & MEM_UNINIT;
>
> given all existing ARG_PTR_TO_{INT,LONG} use cases just write into
> that memory, why not just set meta->raw_mode unconditionally and not
> touch helper definitions?
>
> also, isn't it suspicious that int_ptr_types have PTR_TO_MAP_KEY in
> it? key should always be immutable, so can't be written into, no?
That does not look right agree.. presumably copied over from mem_types for reading not
writing memory (just that none of the helpers using the arg type to actually read mem).
Also, I'm currently looking into whether it's possible to just remove the ARG_PTR_TO_{INT,LONG}
and make that a case of ARG_PTR_TO_FIXED_SIZE_MEM | MEM_UNINIT where we just specify the
arg's size in the func proto. Two special arg cases less to look after in verifier then.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf 1/4] bpf: Fix helper writes to read-only maps
2024-09-04 16:02 ` Daniel Borkmann
@ 2024-09-04 17:53 ` Andrii Nakryiko
0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2024-09-04 17:53 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: bpf, kongln9170
On Wed, Sep 4, 2024 at 9:02 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/28/24 12:37 AM, Andrii Nakryiko wrote:
> > On Fri, Aug 23, 2024 at 3:21 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>
> >> Lonial found an issue that despite user- and BPF-side frozen BPF map
> >> (like in case of .rodata), it was still possible to write into it from
> >> a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
> >> as arguments.
> >>
> >> In check_func_arg() when the argument is as mentioned, the meta->raw_mode
> >> is never set. Later, check_helper_mem_access(), under the case of
> >> PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
> >> subsequent call to check_map_access_type() and given the BPF map is
> >> read-only it succeeds.
> >>
> >> The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
> >> when results are written into them as opposed to read out of them. The
> >> latter indicates that it's okay to pass a pointer to uninitialized memory
> >> as the memory is written to anyway.
> >>
> >> Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
> >> Reported-by: Lonial Con <kongln9170@gmail.com>
> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >> ---
> >> kernel/bpf/helpers.c | 4 ++--
> >> kernel/bpf/syscall.c | 2 +-
> >> kernel/bpf/verifier.c | 3 ++-
> >> kernel/trace/bpf_trace.c | 4 ++--
> >> net/core/filter.c | 4 ++--
> >> 5 files changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> >> index b5f0adae8293..356a58aeb79b 100644
> >> --- a/kernel/bpf/helpers.c
> >> +++ b/kernel/bpf/helpers.c
> >> @@ -538,7 +538,7 @@ const struct bpf_func_proto bpf_strtol_proto = {
> >> .arg1_type = ARG_PTR_TO_MEM | MEM_RDONLY,
> >> .arg2_type = ARG_CONST_SIZE,
> >> .arg3_type = ARG_ANYTHING,
> >> - .arg4_type = ARG_PTR_TO_LONG,
> >> + .arg4_type = ARG_PTR_TO_LONG | MEM_UNINIT,
> >> };
> >>
> >> BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
> >> @@ -566,7 +566,7 @@ const struct bpf_func_proto bpf_strtoul_proto = {
> >> .arg1_type = ARG_PTR_TO_MEM | MEM_RDONLY,
> >> .arg2_type = ARG_CONST_SIZE,
> >> .arg3_type = ARG_ANYTHING,
> >> - .arg4_type = ARG_PTR_TO_LONG,
> >> + .arg4_type = ARG_PTR_TO_LONG | MEM_UNINIT,
> >> };
> >>
> >> BPF_CALL_3(bpf_strncmp, const char *, s1, u32, s1_sz, const char *, s2)
> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >> index bf6c5f685ea2..6d5942a6f41f 100644
> >> --- a/kernel/bpf/syscall.c
> >> +++ b/kernel/bpf/syscall.c
> >> @@ -5952,7 +5952,7 @@ static const struct bpf_func_proto bpf_kallsyms_lookup_name_proto = {
> >> .arg1_type = ARG_PTR_TO_MEM,
> >> .arg2_type = ARG_CONST_SIZE_OR_ZERO,
> >> .arg3_type = ARG_ANYTHING,
> >> - .arg4_type = ARG_PTR_TO_LONG,
> >> + .arg4_type = ARG_PTR_TO_LONG | MEM_UNINIT,
> >> };
> >>
> >> static const struct bpf_func_proto *
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index d8520095ca03..70b0474e03a6 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -8877,8 +8877,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >> case ARG_PTR_TO_INT:
> >> case ARG_PTR_TO_LONG:
> >> {
> >> - int size = int_ptr_type_to_size(arg_type);
> >> + int size = int_ptr_type_to_size(base_type(arg_type));
> >>
> >> + meta->raw_mode = arg_type & MEM_UNINIT;
> >
> > given all existing ARG_PTR_TO_{INT,LONG} use cases just write into
> > that memory, why not just set meta->raw_mode unconditionally and not
> > touch helper definitions?
> >
> > also, isn't it suspicious that int_ptr_types have PTR_TO_MAP_KEY in
> > it? key should always be immutable, so can't be written into, no?
>
> That does not look right agree.. presumably copied over from mem_types for reading not
> writing memory (just that none of the helpers using the arg type to actually read mem).
>
> Also, I'm currently looking into whether it's possible to just remove the ARG_PTR_TO_{INT,LONG}
> and make that a case of ARG_PTR_TO_FIXED_SIZE_MEM | MEM_UNINIT where we just specify the
> arg's size in the func proto. Two special arg cases less to look after in verifier then.
>
When I looked at this last time, my conclusion was that
PTR_TO_{INT,LONG} just have extra alignment checks, which might be
important on some architectures. Not sure how to go about that. We
could probably implement this as another MEM_ALIGNED modifier or
something, not sure.
> Thanks,
> Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread