diff mbox series

[bpf-next] bpf: Fix verifier error due to narrower scalar spill onto 64-bit spilled scalar slots

Message ID 20240405202607.4146642-1-tao.lyu@epfl.ch (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next] bpf: Fix verifier error due to narrower scalar spill onto 64-bit spilled scalar slots | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-PR fail PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 955 this patch: 955
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 13 maintainers not CCed: sdf@google.com daniel@iogearbox.net kpsingh@kernel.org maxim@isovalent.com martin.lau@linux.dev yonghong.song@linux.dev shuah@kernel.org linux-kselftest@vger.kernel.org mykolal@fb.com haoluo@google.com jolsa@kernel.org song@kernel.org eddyz87@gmail.com
netdev/build_clang success Errors and warnings before: 955 this patch: 955
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 966 this patch: 966
netdev/checkpatch fail CHECK: Blank lines aren't necessary after an open brace '{' CHECK: Comparison to NULL could be written "!str_cpy" CHECK: Comparison to NULL could be written "token" CHECK: Concatenated strings should use spaces between elements ERROR: code indent should use tabs where possible ERROR: open brace '{' following function definitions go on the next line ERROR: switch and case should be at the same indent WARNING: Avoid line continuations in quoted strings WARNING: Missing a blank line after declarations WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88 WARNING: line length of 116 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: please, no spaces at the start of a line
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 fail Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-16 success Logs for x86_64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test
bpf/vmtest-bpf-next-VM_Test-14 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for s390x-gcc / test
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-4 fail Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-22 fail Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-12 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-llvm-17 / test
bpf/vmtest-bpf-next-VM_Test-9 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-21 fail Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-llvm-18 / test
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / test

Commit Message

Tao Lyu April 5, 2024, 8:26 p.m. UTC 
When CAP_PERFMON and CAP_SYS_ADMIN (allow_ptr_leaks) are disabled,
the verifier previously aimed to reject partial overwrite on an 8-byte stack slot
that contains a spilled pointer.

However, it rejects all partial stack overwrites
as long as the targeted stack slot is a spilled register in that cap setting,
because it does not check if the stack slot is a spilled pointer.

Finally, incomplete checks will result in the rejection of valid programs,
which spill narrower scalar values onto scalar slots, as shown below.

0: R1=ctx() R10=fp0
; asm volatile ( @ repro.bpf.c:679
0: (7a) *(u64 *)(r10 -8) = 1          ; R10=fp0 fp-8_w=1
1: (62) *(u32 *)(r10 -8) = 1
attempt to corrupt spilled pointer on stack
processed 2 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0.

Note that only when CAP_BPF or CAP_SYS_ADMIN are enabled,
are 64-bit scalars stored onto stacks marked as spilled scalars.

Thus, to reproduce this issue,
we can only assign CAP_BPF capability to the test.

However, the existing bpf selftest doesn't support setting specific caps.

So, I modified the test_loader.c and some related header files to
enabled set specific capabilities on a test with the following attributes:

__msg_caps(msg)
__failure_caps(caps)
__success_caps(caps)
__retval_caps(val)

Here, caps can be any combination of capabilities, like "CAP_BPF | CAP_PERFMON".

Finally, the issue is fixed by checking the spilled register type of targeted slots.

Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
Signed-off-by: Tao Lyu <tao.lyu@epfl.ch>
---
 kernel/bpf/verifier.c                         |   1 +
 tools/testing/selftests/bpf/progs/bpf_misc.h  |  13 ++
 .../selftests/bpf/progs/verifier_spill_fill.c |  18 ++
 tools/testing/selftests/bpf/test_loader.c     | 154 ++++++++++++++++--
 4 files changed, 169 insertions(+), 17 deletions(-)

Comments

Andrii Nakryiko April 5, 2024, 8:33 p.m. UTC | #1 
On Fri, Apr 5, 2024 at 1:26 PM Tao Lyu <tao.lyu@epfl.ch> wrote:
>
> When CAP_PERFMON and CAP_SYS_ADMIN (allow_ptr_leaks) are disabled,
> the verifier previously aimed to reject partial overwrite on an 8-byte stack slot
> that contains a spilled pointer.
>
> However, it rejects all partial stack overwrites
> as long as the targeted stack slot is a spilled register in that cap setting,
> because it does not check if the stack slot is a spilled pointer.
>
> Finally, incomplete checks will result in the rejection of valid programs,
> which spill narrower scalar values onto scalar slots, as shown below.
>
> 0: R1=ctx() R10=fp0
> ; asm volatile ( @ repro.bpf.c:679
> 0: (7a) *(u64 *)(r10 -8) = 1          ; R10=fp0 fp-8_w=1
> 1: (62) *(u32 *)(r10 -8) = 1
> attempt to corrupt spilled pointer on stack
> processed 2 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0.
>
> Note that only when CAP_BPF or CAP_SYS_ADMIN are enabled,
> are 64-bit scalars stored onto stacks marked as spilled scalars.
>
> Thus, to reproduce this issue,
> we can only assign CAP_BPF capability to the test.
>
> However, the existing bpf selftest doesn't support setting specific caps.
>
> So, I modified the test_loader.c and some related header files to
> enabled set specific capabilities on a test with the following attributes:
>
> __msg_caps(msg)
> __failure_caps(caps)
> __success_caps(caps)
> __retval_caps(val)
>
> Here, caps can be any combination of capabilities, like "CAP_BPF | CAP_PERFMON".
>
> Finally, the issue is fixed by checking the spilled register type of targeted slots.
>
> Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
> Signed-off-by: Tao Lyu <tao.lyu@epfl.ch>
> ---
>  kernel/bpf/verifier.c                         |   1 +
>  tools/testing/selftests/bpf/progs/bpf_misc.h  |  13 ++
>  .../selftests/bpf/progs/verifier_spill_fill.c |  18 ++
>  tools/testing/selftests/bpf/test_loader.c     | 154 ++++++++++++++++--
>  4 files changed, 169 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 63749ad5ac6b..da575e295d53 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4493,6 +4493,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
>          */
>         if (!env->allow_ptr_leaks &&
>             is_spilled_reg(&state->stack[spi]) &&
> +           !is_spilled_scalar_reg(&state->stack[spi]) &&
>             size != BPF_REG_SIZE) {
>                 verbose(env, "attempt to corrupt spilled pointer on stack\n");
>                 return -EACCES;

ack for this part:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

but I'm not sure if we need all the *_caps logic for selftests just to
test this, tbh. I'll let other decide, though.

> diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
> index fb2f5513e29e..b8dc0d73932f 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> @@ -2,6 +2,10 @@
>  #ifndef __BPF_MISC_H__
>  #define __BPF_MISC_H__
>

[...]
Eduard Zingerman April 6, 2024, 9:45 a.m. UTC | #2 
On Fri, 2024-04-05 at 13:33 -0700, Andrii Nakryiko wrote:
[...]
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 63749ad5ac6b..da575e295d53 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4493,6 +4493,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> >          */
> >         if (!env->allow_ptr_leaks &&
> >             is_spilled_reg(&state->stack[spi]) &&
> > +           !is_spilled_scalar_reg(&state->stack[spi]) &&
> >             size != BPF_REG_SIZE) {
> >                 verbose(env, "attempt to corrupt spilled pointer on stack\n");
> >                 return -EACCES;
> 
> ack for this part:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> but I'm not sure if we need all the *_caps logic for selftests just to
> test this, tbh. I'll let other decide, though.

Hi Tao, Andrii,

I agree with Andrii that *_caps logic is a bit heavy-handed.
It works on top of the test_loader.c:drop_capabilities() used for
unpriv testing, so I suggest that we add only one macro: __caps_unpriv(<set-of-caps>),
that would enable specified capabilities when testing unpriv.
E.g. as in the patch at the end of this email (based on Tao's work).

A few additional notes:
- changes to the verifier.c, to the test_loader.c and actual feature
  test should be split in separate commits;
- __caps_unpriv as in Tao's or my patches allows to set arbitrary
  capabilities, however restore_capabilities() uses
  cap_enable_effective(requested_caps), which does logical OR
  of current_caps and requested_caps.
  For the purpose of restore_capabilities(), I think
  cap_enable_effective() should be updated with a flag to overwrite
  current_caps with requested_caps. Otherwise it might not undo
  __caps_unpriv() effects in some cases.
- I changed the test case to avoid BPF_ST assembly instructions
  ('*(u64*)(r10 - 8) = 1') in order to allow compilation by LLVM 16.
  
Thanks,
Eduard

---

diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index fb2f5513e29e..14a79bc76b45 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -2,6 +2,10 @@
 #ifndef __BPF_MISC_H__
 #define __BPF_MISC_H__
 
+/* Expand a macro and then stringize the expansion */
+#define QUOTE(str) #str
+#define EXPAND_QUOTE(str) QUOTE(str)
+
 /* This set of attributes controls behavior of the
  * test_loader.c:test_loader__run_subtests().
  *
@@ -72,6 +76,7 @@
 #define __auxiliary		__attribute__((btf_decl_tag("comment:test_auxiliary")))
 #define __auxiliary_unpriv	__attribute__((btf_decl_tag("comment:test_auxiliary_unpriv")))
 #define __btf_path(path)	__attribute__((btf_decl_tag("comment:test_btf_path=" path)))
+#define __caps_unpriv(caps)	__attribute__((btf_decl_tag("comment:test_caps_unpriv="EXPAND_QUOTE(caps))))
 
 /* Convenience macro for use with 'asm volatile' blocks */
 #define __naked __attribute__((naked))
diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
index 85e48069c9e6..a043514da4f0 100644
--- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
+++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
@@ -5,6 +5,7 @@
 #include <bpf/bpf_helpers.h>
 #include "bpf_misc.h"
 #include <../../../tools/include/linux/filter.h>
+#include "../cap_helpers.h"
 
 struct {
 	__uint(type, BPF_MAP_TYPE_RINGBUF);
@@ -1244,4 +1245,19 @@ __naked void old_stack_misc_vs_cur_ctx_ptr(void)
 	: __clobber_all);
 }
 
+/* 32-bit scalars should be able to overwrite 64-bit scalar spilled slots */
+SEC("socket")
+__log_level(2) __success __caps_unpriv(CAP_BPF)
+__naked void spill_32bit_onto_64bit_slot(void)
+{
+	asm volatile("					\
+	r0 = 0;						\
+	*(u64*)(r10 - 8) = r0;				\
+	*(u32*)(r10 - 8) = r0;				\
+	exit;						\
+"	:
+	:
+	: __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 524c38e9cde4..ad79b5948440 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -27,6 +27,7 @@
 #define TEST_TAG_RETVAL_PFX_UNPRIV "comment:test_retval_unpriv="
 #define TEST_TAG_AUXILIARY "comment:test_auxiliary"
 #define TEST_TAG_AUXILIARY_UNPRIV "comment:test_auxiliary_unpriv"
+#define TEST_TAG_CAPS_UNPRIV "comment:test_caps_unpriv="
 #define TEST_BTF_PATH "comment:test_btf_path="
 
 /* Warning: duplicated in bpf_misc.h */
@@ -53,6 +54,7 @@ struct test_subspec {
 	size_t expect_msg_cnt;
 	int retval;
 	bool execute;
+	__u64 caps;
 };
 
 struct test_spec {
@@ -133,6 +135,33 @@ static int parse_int(const char *str, int *val, const char *name)
 	return 0;
 }
 
+static int parse_caps(const char *str, __u64 *val, const char *name)
+{
+	int cap_flag = 0;
+	char *token = NULL, *saveptr = NULL;
+
+	char *str_cpy = strdup(str);
+	if (str_cpy == NULL) {
+		PRINT_FAIL("Memory allocation failed\n");
+		return -EINVAL;
+	}
+
+	token = strtok_r(str_cpy, "|", &saveptr);
+	while (token != NULL) {
+		errno = 0;
+		cap_flag = strtol(token, NULL, 10);
+		if (errno) {
+			PRINT_FAIL("failed to parse caps %s\n", name);
+			return -EINVAL;
+		}
+		*val |= (1ULL << cap_flag);
+		token = strtok_r(NULL, "|", &saveptr);
+	}
+
+	free(str_cpy);
+	return 0;
+}
+
 static int parse_retval(const char *str, int *val, const char *name)
 {
 	struct {
@@ -258,6 +287,12 @@ static int parse_test_spec(struct test_loader *tester,
 			spec->mode_mask |= UNPRIV;
 			spec->unpriv.execute = true;
 			has_unpriv_retval = true;
+                } else if (str_has_pfx(s, TEST_TAG_CAPS_UNPRIV)) {
+			val = s + sizeof(TEST_TAG_CAPS_UNPRIV) - 1;
+			err = parse_caps(val, &spec->unpriv.caps, "test caps");
+			if (err)
+				goto cleanup;
+			spec->mode_mask |= UNPRIV;
 		} else if (str_has_pfx(s, TEST_TAG_LOG_LEVEL_PFX)) {
 			val = s + sizeof(TEST_TAG_LOG_LEVEL_PFX) - 1;
 			err = parse_int(val, &spec->log_level, "test log level");
@@ -575,6 +610,7 @@ void run_subtest(struct test_loader *tester,
 		return;
 
 	if (unpriv) {
+		fprintf(stderr, "can_execute_unpriv: %d\n", can_execute_unpriv(tester, spec));
 		if (!can_execute_unpriv(tester, spec)) {
 			test__skip();
 			test__end_subtest();
@@ -584,6 +620,13 @@ void run_subtest(struct test_loader *tester,
 			test__end_subtest();
 			return;
 		}
+		if (subspec->caps) {
+			err = cap_enable_effective(subspec->caps, NULL);
+			if (err) {
+				PRINT_FAIL("failed to set capabilities: %i, %s\n", err, strerror(err));
+				goto subtest_cleanup;
+			}
+		}
 	}
 
 	/* Implicitly reset to NULL if next test case doesn't specify */
Tao Lyu April 8, 2024, 8:13 p.m. UTC | #3 
> [...]
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 63749ad5ac6b..da575e295d53 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -4493,6 +4493,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> > >          */
> > >         if (!env->allow_ptr_leaks &&
> > >             is_spilled_reg(&state->stack[spi]) &&
> > > +           !is_spilled_scalar_reg(&state->stack[spi]) &&
> > >             size != BPF_REG_SIZE) {
> > >                 verbose(env, "attempt to corrupt spilled pointer on stack\n");
> > >                 return -EACCES;
> > 
> > ack for this part:
> > 
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > 
> > but I'm not sure if we need all the *_caps logic for selftests just to
> > test this, tbh. I'll let other decide, though.
> 
> Hi Tao, Andrii,
> 
> I agree with Andrii that *_caps logic is a bit heavy-handed.
> It works on top of the test_loader.c:drop_capabilities() used for
> unpriv testing, so I suggest that we add only one macro: __caps_unpriv(<set-of-caps>),
> that would enable specified capabilities when testing unpriv.
> E.g. as in the patch at the end of this email (based on Tao's work).
> 
> A few additional notes:
> - changes to the verifier.c, to the test_loader.c and actual feature
>   test should be split in separate commits;
> - __caps_unpriv as in Tao's or my patches allows to set arbitrary
>   capabilities, however restore_capabilities() uses
>   cap_enable_effective(requested_caps), which does logical OR
>   of current_caps and requested_caps.
>   For the purpose of restore_capabilities(), I think
>   cap_enable_effective() should be updated with a flag to overwrite
>   current_caps with requested_caps. Otherwise it might not undo
>   __caps_unpriv() effects in some cases.
> - I changed the test case to avoid BPF_ST assembly instructions
>   ('*(u64*)(r10 - 8) = 1') in order to allow compilation by LLVM 16.
>   
> Thanks,
> Eduard

Hi Andrii and Eduard,

Thanks for the comments.

Eduard's proposal looks good to me.

If we all agree on it, I can send three separate commits for the bug fix,
the extension on test_loader.c, and the test case for this issue.

Best,
Tao

> 
> ---
> 
> diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
> index fb2f5513e29e..14a79bc76b45 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> @@ -2,6 +2,10 @@
>  #ifndef __BPF_MISC_H__
>  #define __BPF_MISC_H__
>  
> +/* Expand a macro and then stringize the expansion */
> +#define QUOTE(str) #str
> +#define EXPAND_QUOTE(str) QUOTE(str)
> +
>  /* This set of attributes controls behavior of the
>   * test_loader.c:test_loader__run_subtests().
>   *
> @@ -72,6 +76,7 @@
>  #define __auxiliary             __attribute__((btf_decl_tag("comment:test_auxiliary")))
>  #define __auxiliary_unpriv      __attribute__((btf_decl_tag("comment:test_auxiliary_unpriv")))
>  #define __btf_path(path)        __attribute__((btf_decl_tag("comment:test_btf_path=" path)))
> +#define __caps_unpriv(caps)    __attribute__((btf_decl_tag("comment:test_caps_unpriv="EXPAND_QUOTE(caps))))
>  
>  /* Convenience macro for use with 'asm volatile' blocks */
>  #define __naked __attribute__((naked))
> diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> index 85e48069c9e6..a043514da4f0 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> @@ -5,6 +5,7 @@
>  #include <bpf/bpf_helpers.h>
>  #include "bpf_misc.h"
>  #include <../../../tools/include/linux/filter.h>
> +#include "../cap_helpers.h"
>  
>  struct {
>          __uint(type, BPF_MAP_TYPE_RINGBUF);
> @@ -1244,4 +1245,19 @@ __naked void old_stack_misc_vs_cur_ctx_ptr(void)
>          : __clobber_all);
>  }
>  
> +/* 32-bit scalars should be able to overwrite 64-bit scalar spilled slots */
> +SEC("socket")
> +__log_level(2) __success __caps_unpriv(CAP_BPF)
> +__naked void spill_32bit_onto_64bit_slot(void)
> +{
> +       asm volatile("                                  \
> +       r0 = 0;                                         \
> +       *(u64*)(r10 - 8) = r0;                          \
> +       *(u32*)(r10 - 8) = r0;                          \
> +       exit;                                           \
> +"      :
> +       :
> +       : __clobber_all);
> +}
> +
>  char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> index 524c38e9cde4..ad79b5948440 100644
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c
> @@ -27,6 +27,7 @@
>  #define TEST_TAG_RETVAL_PFX_UNPRIV "comment:test_retval_unpriv="
>  #define TEST_TAG_AUXILIARY "comment:test_auxiliary"
>  #define TEST_TAG_AUXILIARY_UNPRIV "comment:test_auxiliary_unpriv"
> +#define TEST_TAG_CAPS_UNPRIV "comment:test_caps_unpriv="
>  #define TEST_BTF_PATH "comment:test_btf_path="
>  
>  /* Warning: duplicated in bpf_misc.h */
> @@ -53,6 +54,7 @@ struct test_subspec {
>          size_t expect_msg_cnt;
>          int retval;
>          bool execute;
> +       __u64 caps;
>  };
>  
>  struct test_spec {
> @@ -133,6 +135,33 @@ static int parse_int(const char *str, int *val, const char *name)
>          return 0;
>  }
>  
> +static int parse_caps(const char *str, __u64 *val, const char *name)
> +{
> +       int cap_flag = 0;
> +       char *token = NULL, *saveptr = NULL;
> +
> +       char *str_cpy = strdup(str);
> +       if (str_cpy == NULL) {
> +               PRINT_FAIL("Memory allocation failed\n");
> +               return -EINVAL;
> +       }
> +
> +       token = strtok_r(str_cpy, "|", &saveptr);
> +       while (token != NULL) {
> +               errno = 0;
> +               cap_flag = strtol(token, NULL, 10);
> +               if (errno) {
> +                       PRINT_FAIL("failed to parse caps %s\n", name);
> +                       return -EINVAL;
> +               }
> +               *val |= (1ULL << cap_flag);
> +               token = strtok_r(NULL, "|", &saveptr);
> +       }
> +
> +       free(str_cpy);
> +       return 0;
> +}
> +
>  static int parse_retval(const char *str, int *val, const char *name)
>  {
>          struct {
> @@ -258,6 +287,12 @@ static int parse_test_spec(struct test_loader *tester,
>                          spec->mode_mask |= UNPRIV;
>                          spec->unpriv.execute = true;
>                          has_unpriv_retval = true;
> +                } else if (str_has_pfx(s, TEST_TAG_CAPS_UNPRIV)) {
> +                       val = s + sizeof(TEST_TAG_CAPS_UNPRIV) - 1;
> +                       err = parse_caps(val, &spec->unpriv.caps, "test caps");
> +                       if (err)
> +                               goto cleanup;
> +                       spec->mode_mask |= UNPRIV;
>                  } else if (str_has_pfx(s, TEST_TAG_LOG_LEVEL_PFX)) {
>                          val = s + sizeof(TEST_TAG_LOG_LEVEL_PFX) - 1;
>                          err = parse_int(val, &spec->log_level, "test log level");
> @@ -575,6 +610,7 @@ void run_subtest(struct test_loader *tester,
>                  return;
>  
>          if (unpriv) {
> +               fprintf(stderr, "can_execute_unpriv: %d\n", can_execute_unpriv(tester, spec));
>                  if (!can_execute_unpriv(tester, spec)) {
>                          test__skip();
>                          test__end_subtest();
> @@ -584,6 +620,13 @@ void run_subtest(struct test_loader *tester,
>                          test__end_subtest();
>                          return;
>                  }
> +               if (subspec->caps) {
> +                       err = cap_enable_effective(subspec->caps, NULL);
> +                       if (err) {
> +                               PRINT_FAIL("failed to set capabilities: %i, %s\n", err, strerror(err));
> +                               goto subtest_cleanup;
> +                       }
> +               }
>          }
>  
>          /* Implicitly reset to NULL if next test case doesn't specify */
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 63749ad5ac6b..da575e295d53 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4493,6 +4493,7 @@  static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
 	 */
 	if (!env->allow_ptr_leaks &&
 	    is_spilled_reg(&state->stack[spi]) &&
+	    !is_spilled_scalar_reg(&state->stack[spi]) &&
 	    size != BPF_REG_SIZE) {
 		verbose(env, "attempt to corrupt spilled pointer on stack\n");
 		return -EACCES;
diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index fb2f5513e29e..b8dc0d73932f 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -2,6 +2,10 @@ 
 #ifndef __BPF_MISC_H__
 #define __BPF_MISC_H__
 
+/* Expand a macro and then stringize the expansion */
+#define QUOTE(str) #str
+#define EXPAND_QUOTE(str) QUOTE(str)
+
 /* This set of attributes controls behavior of the
  * test_loader.c:test_loader__run_subtests().
  *
@@ -24,11 +28,15 @@ 
  *                   Multiple __msg attributes could be specified.
  * __msg_unpriv      Same as __msg but for unprivileged mode.
  *
+ * __msg_caps        Same as __msg but for specified capabilities.
+ *
  * __success         Expect program load success in privileged mode.
  * __success_unpriv  Expect program load success in unprivileged mode.
+ * __success_caps    Expect program load success in specified cap mode.
  *
  * __failure         Expect program load failure in privileged mode.
  * __failure_unpriv  Expect program load failure in unprivileged mode.
+ * __failure_caps    Expect program load failure in specified cap mode.
  *
  * __retval          Execute the program using BPF_PROG_TEST_RUN command,
  *                   expect return value to match passed parameter:
@@ -38,6 +46,7 @@ 
  *                   - literal POINTER_VALUE (see definition below)
  *                   - literal TEST_DATA_LEN (see definition below)
  * __retval_unpriv   Same, but load program in unprivileged mode.
+ * __retval_caps     Same, but load program in specified cap mode.
  *
  * __description     Text to be used instead of a program name for display
  *                   and filtering purposes.
@@ -63,12 +72,16 @@ 
 #define __success		__attribute__((btf_decl_tag("comment:test_expect_success")))
 #define __description(desc)	__attribute__((btf_decl_tag("comment:test_description=" desc)))
 #define __msg_unpriv(msg)	__attribute__((btf_decl_tag("comment:test_expect_msg_unpriv=" msg)))
+#define __msg_caps(msg)	__attribute__((btf_decl_tag("comment:test_expect_msg_caps=" msg)))
 #define __failure_unpriv	__attribute__((btf_decl_tag("comment:test_expect_failure_unpriv")))
 #define __success_unpriv	__attribute__((btf_decl_tag("comment:test_expect_success_unpriv")))
+#define __failure_caps(caps)	__attribute__((btf_decl_tag("comment:test_expect_failure_caps="EXPAND_QUOTE(caps))))
+#define __success_caps(caps)	__attribute__((btf_decl_tag("comment:test_expect_success_caps="EXPAND_QUOTE(caps))))
 #define __log_level(lvl)	__attribute__((btf_decl_tag("comment:test_log_level="#lvl)))
 #define __flag(flag)		__attribute__((btf_decl_tag("comment:test_prog_flags="#flag)))
 #define __retval(val)		__attribute__((btf_decl_tag("comment:test_retval="#val)))
 #define __retval_unpriv(val)	__attribute__((btf_decl_tag("comment:test_retval_unpriv="#val)))
+#define __retval_caps(val)	__attribute__((btf_decl_tag("comment:test_retval_caps="#val)))
 #define __auxiliary		__attribute__((btf_decl_tag("comment:test_auxiliary")))
 #define __auxiliary_unpriv	__attribute__((btf_decl_tag("comment:test_auxiliary_unpriv")))
 #define __btf_path(path)	__attribute__((btf_decl_tag("comment:test_btf_path=" path)))
diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
index 85e48069c9e6..c3eb6f7cf0c2 100644
--- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
+++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
@@ -5,6 +5,7 @@ 
 #include <bpf/bpf_helpers.h>
 #include "bpf_misc.h"
 #include <../../../tools/include/linux/filter.h>
+#include "../cap_helpers.h"
 
 struct {
 	__uint(type, BPF_MAP_TYPE_RINGBUF);
@@ -1244,4 +1245,21 @@  __naked void old_stack_misc_vs_cur_ctx_ptr(void)
 	: __clobber_all);
 }
 
+SEC("socket")
+__description("32-bit scalars should be able to overwrite 64-bit scalar spilled slots")
+__log_level(2)
+__success __success_unpriv
+__success_caps(CAP_BPF) __retval_caps(0)
+__naked void spill_32bit_onto_64bit_slot(void)
+{
+	asm volatile("					\
+	*(u64*)(r10 - 8) = 1;				\
+	*(u32*)(r10 - 8) = 1;				\
+	r0 = 0;						\
+	exit;						\
+"	:
+	:
+	: __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 524c38e9cde4..42701ec0212f 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -20,11 +20,15 @@ 
 #define TEST_TAG_EXPECT_FAILURE_UNPRIV "comment:test_expect_failure_unpriv"
 #define TEST_TAG_EXPECT_SUCCESS_UNPRIV "comment:test_expect_success_unpriv"
 #define TEST_TAG_EXPECT_MSG_PFX_UNPRIV "comment:test_expect_msg_unpriv="
+#define TEST_TAG_EXPECT_FAILURE_CAPS "comment:test_expect_failure_caps="
+#define TEST_TAG_EXPECT_SUCCESS_CAPS "comment:test_expect_success_caps="
+#define TEST_TAG_EXPECT_MSG_PFX_CAPS "comment:test_expect_msg_caps="
 #define TEST_TAG_LOG_LEVEL_PFX "comment:test_log_level="
 #define TEST_TAG_PROG_FLAGS_PFX "comment:test_prog_flags="
 #define TEST_TAG_DESCRIPTION_PFX "comment:test_description="
 #define TEST_TAG_RETVAL_PFX "comment:test_retval="
 #define TEST_TAG_RETVAL_PFX_UNPRIV "comment:test_retval_unpriv="
+#define TEST_TAG_RETVAL_PFX_CAPS "comment:test_retval_caps="
 #define TEST_TAG_AUXILIARY "comment:test_auxiliary"
 #define TEST_TAG_AUXILIARY_UNPRIV "comment:test_auxiliary_unpriv"
 #define TEST_BTF_PATH "comment:test_btf_path="
@@ -43,7 +47,8 @@  static int sysctl_unpriv_disabled = -1;
 
 enum mode {
 	PRIV = 1,
-	UNPRIV = 2
+	UNPRIV = 2,
+	CUSTCAPS = 4
 };
 
 struct test_subspec {
@@ -59,6 +64,8 @@  struct test_spec {
 	const char *prog_name;
 	struct test_subspec priv;
 	struct test_subspec unpriv;
+	struct test_subspec custom_caps;
+	__u64 caps;
 	const char *btf_custom_path;
 	int log_level;
 	int prog_flags;
@@ -133,6 +140,33 @@  static int parse_int(const char *str, int *val, const char *name)
 	return 0;
 }
 
+static int parse_caps(const char *str, __u64 *val, const char *name)
+{
+	int cap_flag = 0;
+	char *token = NULL, *saveptr = NULL;
+
+	char *str_cpy = strdup(str);
+	if (str_cpy == NULL) {
+		PRINT_FAIL("Memory allocation failed\n");
+		return -EINVAL;
+	}
+
+	token = strtok_r(str_cpy, "|", &saveptr);
+	while (token != NULL) {
+		errno = 0;
+		cap_flag = strtol(token, NULL, 10);
+		if (errno) {
+			PRINT_FAIL("failed to parse caps %s\n", name);
+			return -EINVAL;
+		}
+		*val |= (1ULL << cap_flag);
+		token = strtok_r(NULL, "|", &saveptr);
+	}
+
+	free(str_cpy);
+	return 0;
+}
+
 static int parse_retval(const char *str, int *val, const char *name)
 {
 	struct {
@@ -163,6 +197,22 @@  static void update_flags(int *flags, int flag, bool clear)
 		*flags |= flag;
 }
 
+static char *create_desc(const char *description, const char *suffix)
+{
+	int descr_len = strlen(description);
+	char *name;
+
+	name = malloc(descr_len + strlen(suffix) + 1);
+	if (!name) {
+		PRINT_FAIL("failed to allocate memory for unpriv.name\n");
+		return NULL;
+	}
+
+	strcpy(name, description);
+	strcpy(&name[descr_len], suffix);
+	return name;
+}
+
 /* Uses btf_decl_tag attributes to describe the expected test
  * behavior, see bpf_misc.h for detailed description of each attribute
  * and attribute combinations.
@@ -225,6 +275,20 @@  static int parse_test_spec(struct test_loader *tester,
 			spec->unpriv.expect_failure = false;
 			spec->mode_mask |= UNPRIV;
 			has_unpriv_result = true;
+		} else if (str_has_pfx(s, TEST_TAG_EXPECT_FAILURE_CAPS)) {
+			val = s + sizeof(TEST_TAG_EXPECT_FAILURE_CAPS) - 1;
+			err = parse_caps(val, &spec->caps, "test caps");
+			if (err)
+				goto cleanup;
+			spec->custom_caps.expect_failure = true;
+			spec->mode_mask |= CUSTCAPS;
+                } else if (str_has_pfx(s, TEST_TAG_EXPECT_SUCCESS_CAPS)) {
+			val = s + sizeof(TEST_TAG_EXPECT_SUCCESS_CAPS) - 1;
+			err = parse_caps(val, &spec->caps, "test caps");
+			if (err)
+				goto cleanup;
+			spec->custom_caps.expect_failure = false;
+			spec->mode_mask |= CUSTCAPS;
 		} else if (strcmp(s, TEST_TAG_AUXILIARY) == 0) {
 			spec->auxiliary = true;
 			spec->mode_mask |= PRIV;
@@ -243,6 +307,12 @@  static int parse_test_spec(struct test_loader *tester,
 			if (err)
 				goto cleanup;
 			spec->mode_mask |= UNPRIV;
+		} else if (str_has_pfx(s, TEST_TAG_EXPECT_MSG_PFX_CAPS)) {
+			msg = s + sizeof(TEST_TAG_EXPECT_MSG_PFX_CAPS) - 1;
+			err = push_msg(msg, &spec->custom_caps);
+			if (err)
+				goto cleanup;
+			spec->mode_mask |= CUSTCAPS;
 		} else if (str_has_pfx(s, TEST_TAG_RETVAL_PFX)) {
 			val = s + sizeof(TEST_TAG_RETVAL_PFX) - 1;
 			err = parse_retval(val, &spec->priv.retval, "__retval");
@@ -258,6 +328,13 @@  static int parse_test_spec(struct test_loader *tester,
 			spec->mode_mask |= UNPRIV;
 			spec->unpriv.execute = true;
 			has_unpriv_retval = true;
+		} else if (str_has_pfx(s, TEST_TAG_RETVAL_PFX_CAPS)) {
+			val = s + sizeof(TEST_TAG_RETVAL_PFX_CAPS) - 1;
+			err = parse_retval(val, &spec->custom_caps.retval, "__retval_caps");
+			if (err)
+				goto cleanup;
+			spec->mode_mask |= CUSTCAPS;
+			spec->custom_caps.execute = true;
 		} else if (str_has_pfx(s, TEST_TAG_LOG_LEVEL_PFX)) {
 			val = s + sizeof(TEST_TAG_LOG_LEVEL_PFX) - 1;
 			err = parse_int(val, &spec->log_level, "test log level");
@@ -311,22 +388,23 @@  static int parse_test_spec(struct test_loader *tester,
 	}
 
 	if (spec->mode_mask & UNPRIV) {
-		int descr_len = strlen(description);
-		const char *suffix = " @unpriv";
-		char *name;
-
-		name = malloc(descr_len + strlen(suffix) + 1);
+		char *name = create_desc(description, " @unpriv");
 		if (!name) {
-			PRINT_FAIL("failed to allocate memory for unpriv.name\n");
 			err = -ENOMEM;
 			goto cleanup;
 		}
-
-		strcpy(name, description);
-		strcpy(&name[descr_len], suffix);
 		spec->unpriv.name = name;
 	}
 
+	if (spec->mode_mask & CUSTCAPS) {
+		char *name = create_desc(description, " @caps");
+		if (!name) {
+			err = -ENOMEM;
+			goto cleanup;
+		}
+		spec->custom_caps.name = name;
+	}
+
 	if (spec->mode_mask & (PRIV | UNPRIV)) {
 		if (!has_unpriv_result)
 			spec->unpriv.expect_failure = spec->priv.expect_failure;
@@ -461,6 +539,28 @@  static int restore_capabilities(struct cap_state *caps)
 	return err;
 }
 
+static int set_capabilities(__u64 new_caps, struct cap_state *save_caps) {
+
+	int err;
+
+	/* Drop all bpf related caps */
+	err = drop_capabilities(save_caps);
+	if (err) {
+		PRINT_FAIL("failed to drop capabilities: %i, %s\n", err, strerror(err));
+		return err;
+	}
+
+	/* Set the specified caps */
+	err = cap_enable_effective(new_caps, NULL);
+	if (err) {
+		PRINT_FAIL("failed to set capabilities: %i, %s\n", err, strerror(err));
+		return err;
+	}
+
+	save_caps->initialized = true;
+	return 0;
+}
+
 static bool can_execute_unpriv(struct test_loader *tester, struct test_spec *spec)
 {
 	if (sysctl_unpriv_disabled < 0)
@@ -560,21 +660,34 @@  void run_subtest(struct test_loader *tester,
 		 size_t obj_byte_cnt,
 		 struct test_spec *specs,
 		 struct test_spec *spec,
-		 bool unpriv)
+		 int priv_level)
 {
-	struct test_subspec *subspec = unpriv ? &spec->unpriv : &spec->priv;
+	struct test_subspec *subspec = NULL;
 	struct bpf_program *tprog = NULL, *tprog_iter;
 	struct test_spec *spec_iter;
 	struct cap_state caps = {};
 	struct bpf_object *tobj;
 	struct bpf_map *map;
 	int retval, err, i;
-	bool should_load;
+	bool should_load, unpriv = (priv_level != PRIV);
+
+	switch (priv_level) {
+		case PRIV:
+			subspec = &spec->priv;
+			break;
+		case UNPRIV:
+			subspec = &spec->unpriv;
+			break;
+		case CUSTCAPS:
+			subspec = &spec->custom_caps;
+			break;
+		default:
+	}
 
 	if (!test__start_subtest(subspec->name))
 		return;
 
-	if (unpriv) {
+	if (priv_level == UNPRIV) {
 		if (!can_execute_unpriv(tester, spec)) {
 			test__skip();
 			test__end_subtest();
@@ -584,6 +697,11 @@  void run_subtest(struct test_loader *tester,
 			test__end_subtest();
 			return;
 		}
+	} else if (priv_level == CUSTCAPS) {
+		if (set_capabilities(spec->caps, &caps)) {
+			test__end_subtest();
+			return;
+		}
 	}
 
 	/* Implicitly reset to NULL if next test case doesn't specify */
@@ -714,11 +832,13 @@  static void process_subtest(struct test_loader *tester,
 
 		if (spec->mode_mask & PRIV)
 			run_subtest(tester, &open_opts, obj_bytes, obj_byte_cnt,
-				    specs, spec, false);
+				    specs, spec, PRIV);
 		if (spec->mode_mask & UNPRIV)
 			run_subtest(tester, &open_opts, obj_bytes, obj_byte_cnt,
-				    specs, spec, true);
-
+				    specs, spec, UNPRIV);
+		if (spec->mode_mask & CUSTCAPS)
+			run_subtest(tester, &open_opts, obj_bytes, obj_byte_cnt,
+				    specs, spec, CUSTCAPS);
 	}
 
 	for (i = 0; i < nr_progs; ++i)