[PATCH bpf-next v2 0/4] Fixes for stack with allow_ptr_leaks

[PATCH bpf-next v2 0/4] Fixes for stack with allow_ptr_leaks

[Kumar Kartikeya Dwivedi]

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:
----------
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

Kumar Kartikeya Dwivedi (3):
  bpf: Don't relax STACK_INVALID to STACK_MISC when not allow_ptr_leaks
  selftests/bpf: Add test for reading from STACK_INVALID slots
  selftests/bpf: Add test for narrow spill into 64-bit spilled scalar

Tao Lyu (1):
  bpf: Fix narrow scalar spill onto 64-bit spilled scalar slots

 kernel/bpf/verifier.c                         |  3 +-
 .../selftests/bpf/prog_tests/verifier.c       | 41 ++++++++++++++++---
 .../selftests/bpf/progs/verifier_spill_fill.c | 18 ++++----
 .../bpf/progs/verifier_stack_noperfmon.c      | 36 ++++++++++++++++
 4 files changed, 82 insertions(+), 16 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c


base-commit: c8d02b547363880d996f80c38cc8b997c7b90725
-- 
2.43.5

[PATCH bpf-next v2 1/4] bpf: Don't relax STACK_INVALID to STACK_MISC when not allow_ptr_leaks

[Kumar Kartikeya Dwivedi]

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.

Currently, the condition is inverted (i.e. checking for true instead of
false), simply invert it to restore correct behavior.

Update error strings of selftests relying on current behavior's verifier
output.

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                          |  2 +-
 .../selftests/bpf/progs/verifier_spill_fill.c  | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1c4ebb326785..f9791a001e25 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1209,7 +1209,7 @@ 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 (!env->allow_ptr_leaks && *stype == STACK_INVALID)
 		return;
 	*stype = STACK_MISC;
 }
diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
index 671d9f415dbf..f52f10dbc91d 100644
--- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
+++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
@@ -464,9 +464,9 @@ l0_%=:	r1 >>= 16;					\
 SEC("raw_tp")
 __log_level(2)
 __success
-__msg("fp-8=0m??scalar()")
-__msg("fp-16=00mm??scalar()")
-__msg("fp-24=00mm???scalar()")
+__msg("fp-8=0mmmscalar()")
+__msg("fp-16=00mmmmscalar()")
+__msg("fp-24=00mmmmmscalar()")
 __naked void spill_subregs_preserve_stack_zero(void)
 {
 	asm volatile (
@@ -717,16 +717,16 @@ SEC("raw_tp")
 __log_level(2) __flag(BPF_F_TEST_STATE_FREQ)
 __success
 /* make sure fp-8 is 32-bit FAKE subregister spill */
-__msg("3: (62) *(u32 *)(r10 -8) = 1          ; R10=fp0 fp-8=????1")
+__msg("3: (62) *(u32 *)(r10 -8) = 1          ; R10=fp0 fp-8=mmmm1")
 /* but fp-16 is spilled IMPRECISE zero const reg */
-__msg("5: (63) *(u32 *)(r10 -16) = r0        ; R0_w=1 R10=fp0 fp-16=????1")
+__msg("5: (63) *(u32 *)(r10 -16) = r0        ; R0_w=1 R10=fp0 fp-16=mmmm1")
 /* validate load from fp-8, which was initialized using BPF_ST_MEM */
-__msg("8: (61) r2 = *(u32 *)(r10 -8)         ; R2_w=1 R10=fp0 fp-8=????1")
+__msg("8: (61) r2 = *(u32 *)(r10 -8)         ; R2_w=1 R10=fp0 fp-8=mmmm1")
 __msg("9: (0f) r1 += r2")
 __msg("mark_precise: frame0: last_idx 9 first_idx 7 subseq_idx -1")
 __msg("mark_precise: frame0: regs=r2 stack= before 8: (61) r2 = *(u32 *)(r10 -8)")
 __msg("mark_precise: frame0: regs= stack=-8 before 7: (bf) r1 = r6")
-__msg("mark_precise: frame0: parent state regs= stack=-8:  R0_w=1 R1=ctx() R6_r=map_value(map=.data.two_byte_,ks=4,vs=2) R10=fp0 fp-8_r=????P1 fp-16=????1")
+__msg("mark_precise: frame0: parent state regs= stack=-8:  R0_w=1 R1=ctx() R6_r=map_value(map=.data.two_byte_,ks=4,vs=2) R10=fp0 fp-8_r=mmmmP1 fp-16=mmmm1")
 __msg("mark_precise: frame0: last_idx 6 first_idx 3 subseq_idx 7")
 __msg("mark_precise: frame0: regs= stack=-8 before 6: (05) goto pc+0")
 __msg("mark_precise: frame0: regs= stack=-8 before 5: (63) *(u32 *)(r10 -16) = r0")
@@ -734,7 +734,7 @@ __msg("mark_precise: frame0: regs= stack=-8 before 4: (b7) r0 = 1")
 __msg("mark_precise: frame0: regs= stack=-8 before 3: (62) *(u32 *)(r10 -8) = 1")
 __msg("10: R1_w=map_value(map=.data.two_byte_,ks=4,vs=2,off=1) R2_w=1")
 /* validate load from fp-16, which was initialized using BPF_STX_MEM */
-__msg("12: (61) r2 = *(u32 *)(r10 -16)       ; R2_w=1 R10=fp0 fp-16=????1")
+__msg("12: (61) r2 = *(u32 *)(r10 -16)       ; R2_w=1 R10=fp0 fp-16=mmmm1")
 __msg("13: (0f) r1 += r2")
 __msg("mark_precise: frame0: last_idx 13 first_idx 7 subseq_idx -1")
 __msg("mark_precise: frame0: regs=r2 stack= before 12: (61) r2 = *(u32 *)(r10 -16)")
@@ -743,7 +743,7 @@ __msg("mark_precise: frame0: regs= stack=-16 before 10: (73) *(u8 *)(r1 +0) = r2
 __msg("mark_precise: frame0: regs= stack=-16 before 9: (0f) r1 += r2")
 __msg("mark_precise: frame0: regs= stack=-16 before 8: (61) r2 = *(u32 *)(r10 -8)")
 __msg("mark_precise: frame0: regs= stack=-16 before 7: (bf) r1 = r6")
-__msg("mark_precise: frame0: parent state regs= stack=-16:  R0_w=1 R1=ctx() R6_r=map_value(map=.data.two_byte_,ks=4,vs=2) R10=fp0 fp-8_r=????P1 fp-16_r=????P1")
+__msg("mark_precise: frame0: parent state regs= stack=-16:  R0_w=1 R1=ctx() R6_r=map_value(map=.data.two_byte_,ks=4,vs=2) R10=fp0 fp-8_r=mmmmP1 fp-16_r=mmmmP1")
 __msg("mark_precise: frame0: last_idx 6 first_idx 3 subseq_idx 7")
 __msg("mark_precise: frame0: regs= stack=-16 before 6: (05) goto pc+0")
 __msg("mark_precise: frame0: regs= stack=-16 before 5: (63) *(u32 *)(r10 -16) = r0")
-- 
2.43.5

Re: [PATCH bpf-next v2 1/4] bpf: Don't relax STACK_INVALID to STACK_MISC when not allow_ptr_leaks

[Eduard Zingerman]

On Wed, 2024-11-27 at 13:20 -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.
> 
> Currently, the condition is inverted (i.e. checking for true instead of
> false), simply invert it to restore correct behavior.
> 
> Update error strings of selftests relying on current behavior's verifier
> output.
> 
> 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                          |  2 +-
>  .../selftests/bpf/progs/verifier_spill_fill.c  | 18 +++++++++---------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1c4ebb326785..f9791a001e25 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1209,7 +1209,7 @@ 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 (!env->allow_ptr_leaks && *stype == STACK_INVALID)

This change makes sense, but it contradicts a few things:
- comment on top of this function;
- commit message for [0]
  (there is my ack on that commit, but I have no memory of this place...).

Andrii, do you remember why STACK_INVALID had to be changed to STACK_MISC
for unprivileged case?

Kumar argues that the following program should be rejected when unprivileged:

    0: (b7) r2 = 1                        ; R2_w=1
    1: (bf) r6 = r10                      ; R6_w=fp0 R10=fp0
    2: (07) r6 += -8                      ; R6_w=fp-8
    3: (73) *(u8 *)(r6 +0) = r2           ; R2_w=1 R6_w=fp-8 fp-8=???????1
    4: (79) r2 = *(u64 *)(r6 +0)
    invalid read from stack off -8+1 size 8

(which makes sense). But on master we have:

    0: (b7) r2 = 1                        ; R2_w=1
    1: (bf) r6 = r10                      ; R6_w=fp0 R10=fp0
    2: (07) r6 += -8                      ; R6_w=fp-8
    3: (73) *(u8 *)(r6 +0) = r2           ; R2_w=1 R6_w=fp-8 fp-8=mmmmmmm1
    4: (79) r2 = *(u64 *)(r6 +0)          ; R2_w=scalar() R6_w=fp-8 fp-8=mmmmmmm1
    5: (b7) r0 = 0                        ; R0_w=0
    6: (95) exit

(which makes much less sense).

Also, technically speaking, there is no longer a need in replacing
STACK_INVALID with STACK_MISC at all, only STACK_SPILL should be replaced.
(Because in privileged mode reads from STACK_INVALID are allowed).

[0] eaf18febd6eb ("bpf: preserve STACK_ZERO slots on partial reg spills")

>  		return;
>  	*stype = STACK_MISC;
>  }

[...]

[PATCH bpf-next v2 2/4] bpf: Fix narrow scalar spill onto 64-bit spilled scalar slots

[Kumar Kartikeya Dwivedi]

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")
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 f9791a001e25..7fb3aa6561f5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4700,6 +4700,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

Re: [PATCH bpf-next v2 2/4] bpf: Fix narrow scalar spill onto 64-bit spilled scalar slots

[Eduard Zingerman]

On Wed, 2024-11-27 at 13:20 -0800, Kumar Kartikeya Dwivedi 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")
> Signed-off-by: Tao Lyu <tao.lyu@epfl.ch>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

[PATCH bpf-next v2 3/4] selftests/bpf: Add test for reading from STACK_INVALID slots

[Kumar Kartikeya Dwivedi]

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.

Since we need to control the capabilities when loading this test to only
retain CAP_BPF, refactor support added to do the same for
test_verifier_mtu and reuse it for this selftest to avoid copy-paste.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/verifier.c       | 41 ++++++++++++++++---
 .../bpf/progs/verifier_stack_noperfmon.c      | 21 ++++++++++
 2 files changed, 56 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c

diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index d9f65adb456b..aaf4324e8ef0 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -63,6 +63,7 @@
 #include "verifier_prevent_map_lookup.skel.h"
 #include "verifier_private_stack.skel.h"
 #include "verifier_raw_stack.skel.h"
+#include "verifier_stack_noperfmon.skel.h"
 #include "verifier_raw_tp_writable.skel.h"
 #include "verifier_reg_equal.skel.h"
 #include "verifier_ref_tracking.skel.h"
@@ -226,22 +227,50 @@ void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_pack
 void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
 void test_verifier_lsm(void)                  { RUN(verifier_lsm); }
 
-void test_verifier_mtu(void)
+static int test_verifier_disable_caps(__u64 *caps)
 {
-	__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);
+	ret = cap_enable_effective(1ULL << CAP_BPF | 1ULL << CAP_NET_ADMIN, caps);
 	if (!ASSERT_OK(ret, "set_cap_bpf_cap_net_admin"))
-		return;
+		return -EINVAL;
 	ret = cap_disable_effective(1ULL << CAP_SYS_ADMIN | 1ULL << CAP_PERFMON, NULL);
 	if (!ASSERT_OK(ret, "disable_cap_sys_admin"))
+		return -EINVAL;
+	return 0;
+}
+
+static void test_verifier_enable_caps(__u64 caps)
+{
+	if (caps)
+		cap_enable_effective(caps, NULL);
+}
+
+void test_verifier_mtu(void)
+{
+	__u64 caps = 0;
+	int ret;
+
+	ret = test_verifier_disable_caps(&caps);
+	if (ret)
 		goto restore_cap;
 	RUN(verifier_mtu);
 restore_cap:
-	if (caps)
-		cap_enable_effective(caps, NULL);
+	test_verifier_enable_caps(caps);
+}
+
+void test_verifier_stack_noperfmon(void)
+{
+	__u64 caps = 0;
+	int ret;
+
+	ret = test_verifier_disable_caps(&caps);
+	if (ret)
+		goto restore_cap;
+	RUN(verifier_stack_noperfmon);
+restore_cap:
+	test_verifier_enable_caps(caps);
 }
 
 static int init_test_val_map(struct bpf_object *obj, char *map_name)
diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c b/tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
new file mode 100644
index 000000000000..52da836d47a6
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+SEC("tc")
+__description("stack_noperfmon: reject read of invalid slots")
+__failure __msg("invalid read from stack off -8+1 size 8")
+__naked void stack_noperfmon_rejecte_invalid_read(void)
+{
+	asm volatile ("					\
+	r2 = 1;						\
+	r6 = r10;					\
+	r6 += -8;					\
+	*(u8 *)(r6 + 0) = r2;				\
+	r2 = *(u64 *)(r6 + 0);				\
+	r0 = 0;						\
+	exit;						\
+"	::: __clobber_all);
+}
-- 
2.43.5

Re: [PATCH bpf-next v2 3/4] selftests/bpf: Add test for reading from STACK_INVALID slots

[Eduard Zingerman]

On Wed, 2024-11-27 at 13:20 -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.
> 
> Since we need to control the capabilities when loading this test to only
> retain CAP_BPF, refactor support added to do the same for
> test_verifier_mtu and reuse it for this selftest to avoid copy-paste.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/verifier.c       | 41 ++++++++++++++++---
>  .../bpf/progs/verifier_stack_noperfmon.c      | 21 ++++++++++
>  2 files changed, 56 insertions(+), 6 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> index d9f65adb456b..aaf4324e8ef0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> @@ -63,6 +63,7 @@
>  #include "verifier_prevent_map_lookup.skel.h"
>  #include "verifier_private_stack.skel.h"
>  #include "verifier_raw_stack.skel.h"
> +#include "verifier_stack_noperfmon.skel.h"
>  #include "verifier_raw_tp_writable.skel.h"
>  #include "verifier_reg_equal.skel.h"
>  #include "verifier_ref_tracking.skel.h"
> @@ -226,22 +227,50 @@ void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_pack
>  void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
>  void test_verifier_lsm(void)                  { RUN(verifier_lsm); }
>  
> -void test_verifier_mtu(void)
> +static int test_verifier_disable_caps(__u64 *caps)

The original thread [0] discusses __caps_unpriv macro.
I'd prefer such macro over these changes to prog_tests/verifier.c,
were there any technical problems with code suggested in [0]?

[0] https://lore.kernel.org/bpf/a1e48f5d9ae133e19adc6adf27e19d585e06bab4.camel@gmail.com/#t

>  {
> -	__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);
> +	ret = cap_enable_effective(1ULL << CAP_BPF | 1ULL << CAP_NET_ADMIN, caps);
>  	if (!ASSERT_OK(ret, "set_cap_bpf_cap_net_admin"))
> -		return;
> +		return -EINVAL;
>  	ret = cap_disable_effective(1ULL << CAP_SYS_ADMIN | 1ULL << CAP_PERFMON, NULL);
>  	if (!ASSERT_OK(ret, "disable_cap_sys_admin"))
> +		return -EINVAL;
> +	return 0;
> +}

[...]

Re: [PATCH bpf-next v2 3/4] selftests/bpf: Add test for reading from STACK_INVALID slots

[Kumar Kartikeya Dwivedi]

On Thu, 28 Nov 2024 at 02:50, Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-11-27 at 13:20 -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.
> >
> > Since we need to control the capabilities when loading this test to only
> > retain CAP_BPF, refactor support added to do the same for
> > test_verifier_mtu and reuse it for this selftest to avoid copy-paste.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  .../selftests/bpf/prog_tests/verifier.c       | 41 ++++++++++++++++---
> >  .../bpf/progs/verifier_stack_noperfmon.c      | 21 ++++++++++
> >  2 files changed, 56 insertions(+), 6 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > index d9f65adb456b..aaf4324e8ef0 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > @@ -63,6 +63,7 @@
> >  #include "verifier_prevent_map_lookup.skel.h"
> >  #include "verifier_private_stack.skel.h"
> >  #include "verifier_raw_stack.skel.h"
> > +#include "verifier_stack_noperfmon.skel.h"
> >  #include "verifier_raw_tp_writable.skel.h"
> >  #include "verifier_reg_equal.skel.h"
> >  #include "verifier_ref_tracking.skel.h"
> > @@ -226,22 +227,50 @@ void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_pack
> >  void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
> >  void test_verifier_lsm(void)                  { RUN(verifier_lsm); }
> >
> > -void test_verifier_mtu(void)
> > +static int test_verifier_disable_caps(__u64 *caps)
>
> The original thread [0] discusses __caps_unpriv macro.
> I'd prefer such macro over these changes to prog_tests/verifier.c,
> were there any technical problems with code suggested in [0]?
>
> [0] https://lore.kernel.org/bpf/a1e48f5d9ae133e19adc6adf27e19d585e06bab4.camel@gmail.com/#t
>

I think that patch worked as well, but I got to look at this now after
all these months, and concluded that
what Daniel did in
https://lore.kernel.org/bpf/20241021152809.33343-5-daniel@iogearbox.net
was also
acceptable and preferred.

I can add your patch to this set and respin, or post a follow-up converting
test_verifier_mtu to it as well. Whatever is preferred.


> >  {
> > -     __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);
> > +     ret = cap_enable_effective(1ULL << CAP_BPF | 1ULL << CAP_NET_ADMIN, caps);
> >       if (!ASSERT_OK(ret, "set_cap_bpf_cap_net_admin"))
> > -             return;
> > +             return -EINVAL;
> >       ret = cap_disable_effective(1ULL << CAP_SYS_ADMIN | 1ULL << CAP_PERFMON, NULL);
> >       if (!ASSERT_OK(ret, "disable_cap_sys_admin"))
> > +             return -EINVAL;
> > +     return 0;
> > +}
>
> [...]
>

Re: [PATCH bpf-next v2 3/4] selftests/bpf: Add test for reading from STACK_INVALID slots

[Eduard Zingerman]

On Thu, 2024-11-28 at 02:57 +0100, Kumar Kartikeya Dwivedi wrote:
> On Thu, 28 Nov 2024 at 02:50, Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Wed, 2024-11-27 at 13:20 -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.
> > > 
> > > Since we need to control the capabilities when loading this test to only
> > > retain CAP_BPF, refactor support added to do the same for
> > > test_verifier_mtu and reuse it for this selftest to avoid copy-paste.
> > > 
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > >  .../selftests/bpf/prog_tests/verifier.c       | 41 ++++++++++++++++---
> > >  .../bpf/progs/verifier_stack_noperfmon.c      | 21 ++++++++++
> > >  2 files changed, 56 insertions(+), 6 deletions(-)
> > >  create mode 100644 tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
> > > 
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > index d9f65adb456b..aaf4324e8ef0 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > @@ -63,6 +63,7 @@
> > >  #include "verifier_prevent_map_lookup.skel.h"
> > >  #include "verifier_private_stack.skel.h"
> > >  #include "verifier_raw_stack.skel.h"
> > > +#include "verifier_stack_noperfmon.skel.h"
> > >  #include "verifier_raw_tp_writable.skel.h"
> > >  #include "verifier_reg_equal.skel.h"
> > >  #include "verifier_ref_tracking.skel.h"
> > > @@ -226,22 +227,50 @@ void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_pack
> > >  void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
> > >  void test_verifier_lsm(void)                  { RUN(verifier_lsm); }
> > > 
> > > -void test_verifier_mtu(void)
> > > +static int test_verifier_disable_caps(__u64 *caps)
> > 
> > The original thread [0] discusses __caps_unpriv macro.
> > I'd prefer such macro over these changes to prog_tests/verifier.c,
> > were there any technical problems with code suggested in [0]?
> > 
> > [0] https://lore.kernel.org/bpf/a1e48f5d9ae133e19adc6adf27e19d585e06bab4.camel@gmail.com/#t
> > 
> 
> I think that patch worked as well, but I got to look at this now after
> all these months, and concluded that
> what Daniel did in
> https://lore.kernel.org/bpf/20241021152809.33343-5-daniel@iogearbox.net
> was also
> acceptable and preferred.
> 
> I can add your patch to this set and respin, or post a follow-up converting
> test_verifier_mtu to it as well. Whatever is preferred.

Patch #1 would need a respin because the comment for mark_stack_slot_misc() needs fixing.
If you agree with adding __caps_unpriv, could you please make it a part of v3?

[...]

Re: [PATCH bpf-next v2 3/4] selftests/bpf: Add test for reading from STACK_INVALID slots

[Kumar Kartikeya Dwivedi]

On Thu, 28 Nov 2024 at 03:01, Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-11-28 at 02:57 +0100, Kumar Kartikeya Dwivedi wrote:
> > On Thu, 28 Nov 2024 at 02:50, Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > On Wed, 2024-11-27 at 13:20 -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.
> > > >
> > > > Since we need to control the capabilities when loading this test to only
> > > > retain CAP_BPF, refactor support added to do the same for
> > > > test_verifier_mtu and reuse it for this selftest to avoid copy-paste.
> > > >
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > > ---
> > > >  .../selftests/bpf/prog_tests/verifier.c       | 41 ++++++++++++++++---
> > > >  .../bpf/progs/verifier_stack_noperfmon.c      | 21 ++++++++++
> > > >  2 files changed, 56 insertions(+), 6 deletions(-)
> > > >  create mode 100644 tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > > index d9f65adb456b..aaf4324e8ef0 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > > @@ -63,6 +63,7 @@
> > > >  #include "verifier_prevent_map_lookup.skel.h"
> > > >  #include "verifier_private_stack.skel.h"
> > > >  #include "verifier_raw_stack.skel.h"
> > > > +#include "verifier_stack_noperfmon.skel.h"
> > > >  #include "verifier_raw_tp_writable.skel.h"
> > > >  #include "verifier_reg_equal.skel.h"
> > > >  #include "verifier_ref_tracking.skel.h"
> > > > @@ -226,22 +227,50 @@ void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_pack
> > > >  void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
> > > >  void test_verifier_lsm(void)                  { RUN(verifier_lsm); }
> > > >
> > > > -void test_verifier_mtu(void)
> > > > +static int test_verifier_disable_caps(__u64 *caps)
> > >
> > > The original thread [0] discusses __caps_unpriv macro.
> > > I'd prefer such macro over these changes to prog_tests/verifier.c,
> > > were there any technical problems with code suggested in [0]?
> > >
> > > [0] https://lore.kernel.org/bpf/a1e48f5d9ae133e19adc6adf27e19d585e06bab4.camel@gmail.com/#t
> > >
> >
> > I think that patch worked as well, but I got to look at this now after
> > all these months, and concluded that
> > what Daniel did in
> > https://lore.kernel.org/bpf/20241021152809.33343-5-daniel@iogearbox.net
> > was also
> > acceptable and preferred.
> >
> > I can add your patch to this set and respin, or post a follow-up converting
> > test_verifier_mtu to it as well. Whatever is preferred.
>
> Patch #1 would need a respin because the comment for mark_stack_slot_misc() needs fixing.
> If you agree with adding __caps_unpriv, could you please make it a part of v3?

Should I leave STACK_INVALID as-is regardless of privilege, which
would basically remove the need to check allow_ptr_leaks in this
function? As you said, pruning considers them equivalent if
allow_uninit_stack and stack_read logic allows loading from invalid
slots, so upgrading to STACK_MISC isn't necessary.

>
> [...]
>

[PATCH bpf-next v2 4/4] selftests/bpf: Add test for narrow spill into 64-bit spilled scalar

[Kumar Kartikeya Dwivedi]

Add a test case to verify that without CAP_PERFMON, the test now
succeeds instead of failing due to a verification error.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../bpf/progs/verifier_stack_noperfmon.c          | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c b/tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
index 52da836d47a6..787e01ef477a 100644
--- a/tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
+++ b/tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
@@ -19,3 +19,18 @@ __naked void stack_noperfmon_rejecte_invalid_read(void)
 	exit;						\
 "	::: __clobber_all);
 }
+
+SEC("socket")
+__description("stack_noperfmon: narrow spill onto 64-bit scalar spilled slots")
+__success
+__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);
+}
-- 
2.43.5

Re: [PATCH bpf-next v2 4/4] selftests/bpf: Add test for narrow spill into 64-bit spilled scalar

[Eduard Zingerman]

On Wed, 2024-11-27 at 13:20 -0800, Kumar Kartikeya Dwivedi wrote:
> Add a test case to verify that without CAP_PERFMON, the test now
> succeeds instead of failing due to a verification error.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>