* [PATCH bpf-next v3 1/5] bpf: Don't mark STACK_INVALID as STACK_MISC in mark_stack_slot_misc
2024-12-02 8:38 [PATCH bpf-next v3 0/5] Fixes for stack with allow_ptr_leaks Kumar Kartikeya Dwivedi
@ 2024-12-02 8:38 ` Kumar Kartikeya Dwivedi
2024-12-02 22:51 ` Eduard Zingerman
2024-12-03 0:09 ` Andrii Nakryiko
2024-12-02 8:38 ` [PATCH bpf-next v3 2/5] bpf: Fix narrow scalar spill onto 64-bit spilled scalar slots Kumar Kartikeya Dwivedi
` (3 subsequent siblings)
4 siblings, 2 replies; 12+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-12-02 8:38 UTC (permalink / raw)
To: bpf
Cc: kkd, Tao Lyu, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Eduard Zingerman,
Mathias Payer, Meng Xu, Sanidhya Kashyap
Inside mark_stack_slot_misc, we should not upgrade STACK_INVALID to
STACK_MISC when allow_ptr_leaks is false, since invalid contents
shouldn't be read unless the program has the relevant capabilities.
The relaxation only makes sense when env->allow_ptr_leaks is true.
However, such conversion in privileged mode becomes unnecessary, as
invalid slots can be read without being upgraded to STACK_MISC.
Currently, the condition is inverted (i.e. checking for true instead of
false), simply remove it to restore correct behavior.
Fixes: eaf18febd6eb ("bpf: preserve STACK_ZERO slots on partial reg spills")
Reported-by: Tao Lyu <tao.lyu@epfl.ch>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1c4ebb326785..c6a5c431495c 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;
}
--
2.43.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v3 1/5] bpf: Don't mark STACK_INVALID as STACK_MISC in mark_stack_slot_misc
2024-12-02 8:38 ` [PATCH bpf-next v3 1/5] bpf: Don't mark STACK_INVALID as STACK_MISC in mark_stack_slot_misc Kumar Kartikeya Dwivedi
@ 2024-12-02 22:51 ` Eduard Zingerman
2024-12-03 0:09 ` Andrii Nakryiko
1 sibling, 0 replies; 12+ messages in thread
From: Eduard Zingerman @ 2024-12-02 22:51 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi, bpf
Cc: kkd, Tao Lyu, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Mathias Payer, Meng Xu,
Sanidhya Kashyap
On Mon, 2024-12-02 at 00:38 -0800, Kumar Kartikeya Dwivedi wrote:
> Inside mark_stack_slot_misc, we should not upgrade STACK_INVALID to
> STACK_MISC when allow_ptr_leaks is false, since invalid contents
> shouldn't be read unless the program has the relevant capabilities.
> The relaxation only makes sense when env->allow_ptr_leaks is true.
>
> However, such conversion in privileged mode becomes unnecessary, as
> invalid slots can be read without being upgraded to STACK_MISC.
>
> Currently, the condition is inverted (i.e. checking for true instead of
> false), simply remove it to restore correct behavior.
>
> Fixes: eaf18febd6eb ("bpf: preserve STACK_ZERO slots on partial reg spills")
> Reported-by: Tao Lyu <tao.lyu@epfl.ch>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v3 1/5] bpf: Don't mark STACK_INVALID as STACK_MISC in mark_stack_slot_misc
2024-12-02 8:38 ` [PATCH bpf-next v3 1/5] bpf: Don't mark STACK_INVALID as STACK_MISC in mark_stack_slot_misc Kumar Kartikeya Dwivedi
2024-12-02 22:51 ` Eduard Zingerman
@ 2024-12-03 0:09 ` Andrii Nakryiko
1 sibling, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2024-12-03 0:09 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, kkd, Tao Lyu, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Eduard Zingerman,
Mathias Payer, Meng Xu, Sanidhya Kashyap
On Mon, Dec 2, 2024 at 12:38 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Inside mark_stack_slot_misc, we should not upgrade STACK_INVALID to
> STACK_MISC when allow_ptr_leaks is false, since invalid contents
> shouldn't be read unless the program has the relevant capabilities.
> The relaxation only makes sense when env->allow_ptr_leaks is true.
>
> However, such conversion in privileged mode becomes unnecessary, as
> invalid slots can be read without being upgraded to STACK_MISC.
>
> Currently, the condition is inverted (i.e. checking for true instead of
> false), simply remove it to restore correct behavior.
>
> Fixes: eaf18febd6eb ("bpf: preserve STACK_ZERO slots on partial reg spills")
> Reported-by: Tao Lyu <tao.lyu@epfl.ch>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> kernel/bpf/verifier.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1c4ebb326785..c6a5c431495c 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)
It's a bit worrying that my original comment explicitly states that in
unpriv mode we *have* to set STACK_MISC, but I can't recall why.
Looking at this now, it looks good, so:
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> return;
> *stype = STACK_MISC;
> }
> --
> 2.43.5
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next v3 2/5] bpf: Fix narrow scalar spill onto 64-bit spilled scalar slots
2024-12-02 8:38 [PATCH bpf-next v3 0/5] Fixes for stack with allow_ptr_leaks Kumar Kartikeya Dwivedi
2024-12-02 8:38 ` [PATCH bpf-next v3 1/5] bpf: Don't mark STACK_INVALID as STACK_MISC in mark_stack_slot_misc Kumar Kartikeya Dwivedi
@ 2024-12-02 8:38 ` Kumar Kartikeya Dwivedi
2024-12-03 0:12 ` Andrii Nakryiko
2024-12-02 8:38 ` [PATCH bpf-next v3 3/5] selftests/bpf: Introduce __caps_unpriv annotation for tests Kumar Kartikeya Dwivedi
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-12-02 8:38 UTC (permalink / raw)
To: bpf
Cc: kkd, Eduard Zingerman, Tao Lyu, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, Mathias Payer,
Meng Xu, Sanidhya Kashyap
From: Tao Lyu <tao.lyu@epfl.ch>
When CAP_PERFMON and CAP_SYS_ADMIN (allow_ptr_leaks) are disabled, the
verifier aims to reject partial overwrite on an 8-byte stack slot that
contains a spilled pointer.
However, in such a scenario, it rejects all partial stack overwrites as
long as the targeted stack slot is a spilled register, because it does
not check if the stack slot is a spilled pointer.
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.
Fix this by expanding the check to not consider spilled scalar registers
when rejecting the write into the stack.
Previous discussion on this patch is at link [0].
[0]: https://lore.kernel.org/bpf/20240403202409.2615469-1-tao.lyu@epfl.ch
Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Tao Lyu <tao.lyu@epfl.ch>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c6a5c431495c..51f7a846d719 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4703,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;
--
2.43.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v3 2/5] bpf: Fix narrow scalar spill onto 64-bit spilled scalar slots
2024-12-02 8:38 ` [PATCH bpf-next v3 2/5] bpf: Fix narrow scalar spill onto 64-bit spilled scalar slots Kumar Kartikeya Dwivedi
@ 2024-12-03 0:12 ` Andrii Nakryiko
0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2024-12-03 0:12 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, kkd, Eduard Zingerman, Tao Lyu, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, Mathias Payer,
Meng Xu, Sanidhya Kashyap
On Mon, Dec 2, 2024 at 12:38 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> From: Tao Lyu <tao.lyu@epfl.ch>
>
> When CAP_PERFMON and CAP_SYS_ADMIN (allow_ptr_leaks) are disabled, the
> verifier aims to reject partial overwrite on an 8-byte stack slot that
> contains a spilled pointer.
>
> However, in such a scenario, it rejects all partial stack overwrites as
> long as the targeted stack slot is a spilled register, because it does
> not check if the stack slot is a spilled pointer.
>
> 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.
>
> Fix this by expanding the check to not consider spilled scalar registers
> when rejecting the write into the stack.
>
> Previous discussion on this patch is at link [0].
>
> [0]: https://lore.kernel.org/bpf/20240403202409.2615469-1-tao.lyu@epfl.ch
>
> Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> Signed-off-by: Tao Lyu <tao.lyu@epfl.ch>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> kernel/bpf/verifier.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c6a5c431495c..51f7a846d719 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4703,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]) &&
Makes sense:
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> size != BPF_REG_SIZE) {
> verbose(env, "attempt to corrupt spilled pointer on stack\n");
> return -EACCES;
> --
> 2.43.5
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next v3 3/5] selftests/bpf: Introduce __caps_unpriv annotation for tests
2024-12-02 8:38 [PATCH bpf-next v3 0/5] Fixes for stack with allow_ptr_leaks Kumar Kartikeya Dwivedi
2024-12-02 8:38 ` [PATCH bpf-next v3 1/5] bpf: Don't mark STACK_INVALID as STACK_MISC in mark_stack_slot_misc Kumar Kartikeya Dwivedi
2024-12-02 8:38 ` [PATCH bpf-next v3 2/5] bpf: Fix narrow scalar spill onto 64-bit spilled scalar slots Kumar Kartikeya Dwivedi
@ 2024-12-02 8:38 ` Kumar Kartikeya Dwivedi
2024-12-03 0:14 ` Andrii Nakryiko
2024-12-02 8:38 ` [PATCH bpf-next v3 4/5] selftests/bpf: Add test for reading from STACK_INVALID slots Kumar Kartikeya Dwivedi
2024-12-02 8:38 ` [PATCH bpf-next v3 5/5] selftests/bpf: Add test for narrow spill into 64-bit spilled scalar Kumar Kartikeya Dwivedi
4 siblings, 1 reply; 12+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-12-02 8:38 UTC (permalink / raw)
To: bpf
Cc: kkd, Eduard Zingerman, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Tao Lyu, Mathias Payer,
Meng Xu, Sanidhya Kashyap
From: Eduard Zingerman <eddyz87@gmail.com>
Add a __caps_unpriv annotation so that tests requiring specific
capabilities while dropping the rest can conveniently specify them
during selftest declaration instead of munging with capabilities at
runtime from the testing binary.
While at it, let us convert test_verifier_mtu to use this new support
instead.
The original diff for this idea is available at link [0].
[0]: https://lore.kernel.org/bpf/a1e48f5d9ae133e19adc6adf27e19d585e06bab4.camel@gmail.com
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
[ Kartikeya: rebase on bpf-next, remove unnecessary bits, convert test_verifier_mtu ]
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
.../selftests/bpf/prog_tests/verifier.c | 19 +--------
tools/testing/selftests/bpf/progs/bpf_misc.h | 2 +
.../selftests/bpf/progs/verifier_mtu.c | 3 +-
tools/testing/selftests/bpf/test_loader.c | 41 +++++++++++++++++++
4 files changed, 46 insertions(+), 19 deletions(-)
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..cd9dd427a91d 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -106,6 +106,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 +130,7 @@
#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=" XSTR(caps))))
/* 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..88b1fa5f6030 100644
--- a/tools/testing/selftests/bpf/progs/verifier_mtu.c
+++ b/tools/testing/selftests/bpf/progs/verifier_mtu.c
@@ -6,7 +6,8 @@
SEC("tc/ingress")
__description("uninit/mtu: write rejected")
-__failure __msg("invalid indirect read from stack")
+__success __failure_unpriv __msg_unpriv("invalid indirect read from stack")
+__caps_unpriv(CAP_BPF)
int tc_uninit_mtu(struct __sk_buff *ctx)
{
__u32 mtu;
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 3e9b009580d4..d693e1fc6fa5 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,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 {
@@ -541,6 +570,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 +952,12 @@ 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 */
--
2.43.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v3 3/5] selftests/bpf: Introduce __caps_unpriv annotation for tests
2024-12-02 8:38 ` [PATCH bpf-next v3 3/5] selftests/bpf: Introduce __caps_unpriv annotation for tests Kumar Kartikeya Dwivedi
@ 2024-12-03 0:14 ` Andrii Nakryiko
0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2024-12-03 0:14 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, kkd, Eduard Zingerman, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Tao Lyu, Mathias Payer,
Meng Xu, Sanidhya Kashyap
On Mon, Dec 2, 2024 at 12:38 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> From: Eduard Zingerman <eddyz87@gmail.com>
>
> Add a __caps_unpriv annotation so that tests requiring specific
> capabilities while dropping the rest can conveniently specify them
> during selftest declaration instead of munging with capabilities at
> runtime from the testing binary.
>
> While at it, let us convert test_verifier_mtu to use this new support
> instead.
>
> The original diff for this idea is available at link [0].
>
> [0]: https://lore.kernel.org/bpf/a1e48f5d9ae133e19adc6adf27e19d585e06bab4.camel@gmail.com
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> [ Kartikeya: rebase on bpf-next, remove unnecessary bits, convert test_verifier_mtu ]
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> .../selftests/bpf/prog_tests/verifier.c | 19 +--------
> tools/testing/selftests/bpf/progs/bpf_misc.h | 2 +
> .../selftests/bpf/progs/verifier_mtu.c | 3 +-
> tools/testing/selftests/bpf/test_loader.c | 41 +++++++++++++++++++
> 4 files changed, 46 insertions(+), 19 deletions(-)
>
> 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..cd9dd427a91d 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> @@ -106,6 +106,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 +130,7 @@
> #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=" XSTR(caps))))
>
> /* 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..88b1fa5f6030 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_mtu.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_mtu.c
> @@ -6,7 +6,8 @@
>
> SEC("tc/ingress")
> __description("uninit/mtu: write rejected")
> -__failure __msg("invalid indirect read from stack")
> +__success __failure_unpriv __msg_unpriv("invalid indirect read from stack")
nit: I'd move unpriv specification to a separate line to make it
easier to follow that __success is for privileged, while unpriv has
expected failure with a message
> +__caps_unpriv(CAP_BPF)
original code was setting both CAP_BPF and CAP_NET_ADMIN, but you are
changing to just CAP_BPF? Any reason why?
> int tc_uninit_mtu(struct __sk_buff *ctx)
> {
> __u32 mtu;
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next v3 4/5] selftests/bpf: Add test for reading from STACK_INVALID slots
2024-12-02 8:38 [PATCH bpf-next v3 0/5] Fixes for stack with allow_ptr_leaks Kumar Kartikeya Dwivedi
` (2 preceding siblings ...)
2024-12-02 8:38 ` [PATCH bpf-next v3 3/5] selftests/bpf: Introduce __caps_unpriv annotation for tests Kumar Kartikeya Dwivedi
@ 2024-12-02 8:38 ` Kumar Kartikeya Dwivedi
2024-12-02 22:50 ` Eduard Zingerman
2024-12-03 0:16 ` Andrii Nakryiko
2024-12-02 8:38 ` [PATCH bpf-next v3 5/5] selftests/bpf: Add test for narrow spill into 64-bit spilled scalar Kumar Kartikeya Dwivedi
4 siblings, 2 replies; 12+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-12-02 8:38 UTC (permalink / raw)
To: bpf
Cc: kkd, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Eduard Zingerman, Tao Lyu, Mathias Payer,
Meng Xu, Sanidhya Kashyap
Ensure that when CAP_PERFMON is dropped, and the verifier sees
allow_ptr_leaks as false, we are not permitted to read from a
STACK_INVALID slot. Without the fix, the test will report unexpected
success in loading.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
.../selftests/bpf/progs/verifier_spill_fill.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
index 671d9f415dbf..f5cd21326811 100644
--- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
+++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
@@ -1244,4 +1244,21 @@ __naked void old_stack_misc_vs_cur_ctx_ptr(void)
: __clobber_all);
}
+SEC("tc")
+__description("stack_noperfmon: reject read of invalid slots")
+__success __failure_unpriv __msg_unpriv("invalid read from stack off -8+1 size 8")
+__caps_unpriv(CAP_BPF)
+__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);
+}
+
char _license[] SEC("license") = "GPL";
--
2.43.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v3 4/5] selftests/bpf: Add test for reading from STACK_INVALID slots
2024-12-02 8:38 ` [PATCH bpf-next v3 4/5] selftests/bpf: Add test for reading from STACK_INVALID slots Kumar Kartikeya Dwivedi
@ 2024-12-02 22:50 ` Eduard Zingerman
2024-12-03 0:16 ` Andrii Nakryiko
1 sibling, 0 replies; 12+ messages in thread
From: Eduard Zingerman @ 2024-12-02 22:50 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi, bpf
Cc: kkd, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Tao Lyu, Mathias Payer, Meng Xu,
Sanidhya Kashyap
On Mon, 2024-12-02 at 00:38 -0800, Kumar Kartikeya Dwivedi wrote:
> Ensure that when CAP_PERFMON is dropped, and the verifier sees
> allow_ptr_leaks as false, we are not permitted to read from a
> STACK_INVALID slot. Without the fix, the test will report unexpected
> success in loading.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v3 4/5] selftests/bpf: Add test for reading from STACK_INVALID slots
2024-12-02 8:38 ` [PATCH bpf-next v3 4/5] selftests/bpf: Add test for reading from STACK_INVALID slots Kumar Kartikeya Dwivedi
2024-12-02 22:50 ` Eduard Zingerman
@ 2024-12-03 0:16 ` Andrii Nakryiko
1 sibling, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2024-12-03 0:16 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, kkd, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Eduard Zingerman, Tao Lyu, Mathias Payer,
Meng Xu, Sanidhya Kashyap
On Mon, Dec 2, 2024 at 12:38 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Ensure that when CAP_PERFMON is dropped, and the verifier sees
> allow_ptr_leaks as false, we are not permitted to read from a
> STACK_INVALID slot. Without the fix, the test will report unexpected
> success in loading.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> .../selftests/bpf/progs/verifier_spill_fill.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> index 671d9f415dbf..f5cd21326811 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> @@ -1244,4 +1244,21 @@ __naked void old_stack_misc_vs_cur_ctx_ptr(void)
> : __clobber_all);
> }
>
> +SEC("tc")
> +__description("stack_noperfmon: reject read of invalid slots")
> +__success __failure_unpriv __msg_unpriv("invalid read from stack off -8+1 size 8")
> +__caps_unpriv(CAP_BPF)
same styling nit about __success staying on a separate line
I'd actually do it this way to make it a bit more explicit that we
have custom unpriv caps:
__success
__caps_unpriv(CAP_BPF)
__failure_unpriv __msg_unpriv("...")
but it's minor
> +__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);
> +}
> +
> char _license[] SEC("license") = "GPL";
> --
> 2.43.5
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next v3 5/5] selftests/bpf: Add test for narrow spill into 64-bit spilled scalar
2024-12-02 8:38 [PATCH bpf-next v3 0/5] Fixes for stack with allow_ptr_leaks Kumar Kartikeya Dwivedi
` (3 preceding siblings ...)
2024-12-02 8:38 ` [PATCH bpf-next v3 4/5] selftests/bpf: Add test for reading from STACK_INVALID slots Kumar Kartikeya Dwivedi
@ 2024-12-02 8:38 ` Kumar Kartikeya Dwivedi
4 siblings, 0 replies; 12+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-12-02 8:38 UTC (permalink / raw)
To: bpf
Cc: kkd, Eduard Zingerman, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Tao Lyu, Mathias Payer,
Meng Xu, Sanidhya Kashyap
Add a test case to verify that without CAP_PERFMON, the test now
succeeds instead of failing due to a verification error.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
.../selftests/bpf/progs/verifier_spill_fill.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
index f5cd21326811..83b5cd705ccd 100644
--- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
+++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
@@ -1261,4 +1261,20 @@ __naked void stack_noperfmon_reject_invalid_read(void)
" ::: __clobber_all);
}
+SEC("socket")
+__description("stack_noperfmon: narrow spill onto 64-bit scalar spilled slots")
+__success __success_unpriv
+__caps_unpriv(CAP_BPF)
+__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";
--
2.43.5
^ permalink raw reply related [flat|nested] 12+ messages in thread