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@xxxxxxxxxx>
>
> 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 */
