summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAlexei Starovoitov <ast@kernel.org>2024-12-04 09:19:50 -0800
committerAlexei Starovoitov <ast@kernel.org>2024-12-04 09:19:51 -0800
commite2cf913314b9543f4479788443c7e9009c6c56d8 (patch)
tree946a937b0d54c465efe576bd77e6e1676345d25b
parent5a6ea7022ff4d2a65ae328619c586d6a8909b48b (diff)
parent19b6dbc006ecc1f2c8d7fa2d5afbfb7e7eba7920 (diff)
Merge branch 'fixes-for-stack-with-allow_ptr_leaks'
Kumar Kartikeya Dwivedi says: ==================== Fixes for stack with allow_ptr_leaks Two fixes for usability/correctness gaps when interacting with the stack without CAP_PERFMON (i.e. with allow_ptr_leaks = false). See the commits for details. I've verified that the tests fail when run without the fixes. Changelog: ---------- v3 -> v4 v3: https://lore.kernel.org/bpf/20241202083814.1888784-1-memxor@gmail.com * Address Andrii's comments * Fix bug paperered over by missing CAP_NET_ADMIN in verifier_mtu test * Add warning when undefined CAP_ constant is specified, and fail test * Reorder annotations to be more clear * Verify that fixes fail without patches again * Add Acked-by from Andrii v2 -> v3 v2: https://lore.kernel.org/bpf/20241127212026.3580542-1-memxor@gmail.com * Address comments from Eduard * Fix comment for mark_stack_slot_misc * We can simply always return early when stype == STACK_INVALID * Drop allow_ptr_leaks conditionals * Add Eduard's __caps_unpriv patch into the series * Convert test_verifier_mtu to use it * Move existing tests to __caps_unpriv annotation and verifier_spill_fill.c * Add Acked-by from Eduard v1 -> v2 v1: https://lore.kernel.org/bpf/20241127185135.2753982-1-memxor@gmail.com * Fix CI errors in selftest by removing dependence on BPF_ST ==================== Link: https://patch.msgid.link/20241204044757.1483141-1-memxor@gmail.com Signed-off-by: Alexei Starovoitov <ast@kernel.org>
-rw-r--r--kernel/bpf/verifier.c10
-rw-r--r--tools/testing/selftests/bpf/prog_tests/verifier.c19
-rw-r--r--tools/testing/selftests/bpf/progs/bpf_misc.h12
-rw-r--r--tools/testing/selftests/bpf/progs/verifier_mtu.c4
-rw-r--r--tools/testing/selftests/bpf/progs/verifier_spill_fill.c35
-rw-r--r--tools/testing/selftests/bpf/test_loader.c46
6 files changed, 104 insertions, 22 deletions
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2fd35465d650..01fbef9576e0 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1202,14 +1202,17 @@ static bool is_spilled_scalar_reg64(const struct bpf_stack_state *stack)
/* Mark stack slot as STACK_MISC, unless it is already STACK_INVALID, in which
* case they are equivalent, or it's STACK_ZERO, in which case we preserve
* more precise STACK_ZERO.
- * Note, in uprivileged mode leaving STACK_INVALID is wrong, so we take
- * env->allow_ptr_leaks into account and force STACK_MISC, if necessary.
+ * Regardless of allow_ptr_leaks setting (i.e., privileged or unprivileged
+ * mode), we won't promote STACK_INVALID to STACK_MISC. In privileged case it is
+ * unnecessary as both are considered equivalent when loading data and pruning,
+ * in case of unprivileged mode it will be incorrect to allow reads of invalid
+ * slots.
*/
static void mark_stack_slot_misc(struct bpf_verifier_env *env, u8 *stype)
{
if (*stype == STACK_ZERO)
return;
- if (env->allow_ptr_leaks && *stype == STACK_INVALID)
+ if (*stype == STACK_INVALID)
return;
*stype = STACK_MISC;
}
@@ -4700,6 +4703,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/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index d9f65adb456b..3ee40ee9413a 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -225,24 +225,7 @@ void test_verifier_xdp(void) { RUN(verifier_xdp); }
void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_packet_access); }
void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
void test_verifier_lsm(void) { RUN(verifier_lsm); }
-
-void test_verifier_mtu(void)
-{
- __u64 caps = 0;
- int ret;
-
- /* In case CAP_BPF and CAP_PERFMON is not set */
- ret = cap_enable_effective(1ULL << CAP_BPF | 1ULL << CAP_NET_ADMIN, &caps);
- if (!ASSERT_OK(ret, "set_cap_bpf_cap_net_admin"))
- return;
- ret = cap_disable_effective(1ULL << CAP_SYS_ADMIN | 1ULL << CAP_PERFMON, NULL);
- if (!ASSERT_OK(ret, "disable_cap_sys_admin"))
- goto restore_cap;
- RUN(verifier_mtu);
-restore_cap:
- if (caps)
- cap_enable_effective(caps, NULL);
-}
+void test_verifier_mtu(void) { RUN(verifier_mtu); }
static int init_test_val_map(struct bpf_object *obj, char *map_name)
{
diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index eccaf955e394..f45f4352feeb 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -5,6 +5,10 @@
#define XSTR(s) STR(s)
#define STR(s) #s
+/* 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().
*
@@ -106,6 +110,7 @@
* __arch_* Specify on which architecture the test case should be tested.
* Several __arch_* annotations could be specified at once.
* When test case is not run on current arch it is marked as skipped.
+ * __caps_unpriv Specify the capabilities that should be set when running the test.
*/
#define __msg(msg) __attribute__((btf_decl_tag("comment:test_expect_msg=" XSTR(__COUNTER__) "=" msg)))
#define __xlated(msg) __attribute__((btf_decl_tag("comment:test_expect_xlated=" XSTR(__COUNTER__) "=" msg)))
@@ -129,6 +134,13 @@
#define __arch_x86_64 __arch("X86_64")
#define __arch_arm64 __arch("ARM64")
#define __arch_riscv64 __arch("RISCV64")
+#define __caps_unpriv(caps) __attribute__((btf_decl_tag("comment:test_caps_unpriv=" EXPAND_QUOTE(caps))))
+
+/* Define common capabilities tested using __caps_unpriv */
+#define CAP_NET_ADMIN 12
+#define CAP_SYS_ADMIN 21
+#define CAP_PERFMON 38
+#define CAP_BPF 39
/* Convenience macro for use with 'asm volatile' blocks */
#define __naked __attribute__((naked))
diff --git a/tools/testing/selftests/bpf/progs/verifier_mtu.c b/tools/testing/selftests/bpf/progs/verifier_mtu.c
index 70c7600a26a0..4ccf1ebc42d1 100644
--- a/tools/testing/selftests/bpf/progs/verifier_mtu.c
+++ b/tools/testing/selftests/bpf/progs/verifier_mtu.c
@@ -6,7 +6,9 @@
SEC("tc/ingress")
__description("uninit/mtu: write rejected")
-__failure __msg("invalid indirect read from stack")
+__success
+__caps_unpriv(CAP_BPF|CAP_NET_ADMIN)
+__failure_unpriv __msg_unpriv("invalid indirect read from stack")
int tc_uninit_mtu(struct __sk_buff *ctx)
{
__u32 mtu;
diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
index 671d9f415dbf..1e5a511e8494 100644
--- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
+++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
@@ -1244,4 +1244,39 @@ __naked void old_stack_misc_vs_cur_ctx_ptr(void)
: __clobber_all);
}
+SEC("socket")
+__description("stack_noperfmon: reject read of invalid slots")
+__success
+__caps_unpriv(CAP_BPF)
+__failure_unpriv __msg_unpriv("invalid read from stack off -8+1 size 8")
+__naked void stack_noperfmon_reject_invalid_read(void)
+{
+ asm volatile (" \
+ r2 = 1; \
+ r6 = r10; \
+ r6 += -8; \
+ *(u8 *)(r6 + 0) = r2; \
+ r2 = *(u64 *)(r6 + 0); \
+ r0 = 0; \
+ exit; \
+" ::: __clobber_all);
+}
+
+SEC("socket")
+__description("stack_noperfmon: narrow spill onto 64-bit scalar spilled slots")
+__success
+__caps_unpriv(CAP_BPF)
+__success_unpriv
+__naked void stack_noperfmon_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 3e9b009580d4..53b06647cf57 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -36,6 +36,7 @@
#define TEST_TAG_ARCH "comment:test_arch="
#define TEST_TAG_JITED_PFX "comment:test_jited="
#define TEST_TAG_JITED_PFX_UNPRIV "comment:test_jited_unpriv="
+#define TEST_TAG_CAPS_UNPRIV "comment:test_caps_unpriv="
/* Warning: duplicated in bpf_misc.h */
#define POINTER_VALUE 0xcafe4all
@@ -74,6 +75,7 @@ struct test_subspec {
struct expected_msgs jited;
int retval;
bool execute;
+ __u64 caps;
};
struct test_spec {
@@ -276,6 +278,37 @@ 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;
+ if (!strncmp("CAP_", token, sizeof("CAP_") - 1)) {
+ PRINT_FAIL("define %s constant in bpf_misc.h, failed to parse caps\n", token);
+ return -EINVAL;
+ }
+ cap_flag = strtol(token, NULL, 10);
+ if (!cap_flag || 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 {
@@ -541,6 +574,12 @@ static int parse_test_spec(struct test_loader *tester,
jit_on_next_line = true;
} else if (str_has_pfx(s, TEST_BTF_PATH)) {
spec->btf_custom_path = s + sizeof(TEST_BTF_PATH) - 1;
+ } 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;
}
}
@@ -917,6 +956,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 */