Commit e2cf9133 authored by Alexei Starovoitov's avatar Alexei Starovoitov
Browse files

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: default avatarAlexei Starovoitov <ast@kernel.org>
parents 5a6ea702 19b6dbc0

kernel/bpf/verifier.c

+7 −3
Original line number Diff line number Diff line
@@ -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;

tools/testing/selftests/bpf/prog_tests/verifier.c

+1 −18
Original line number Diff line number Diff line
@@ -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)

{

tools/testing/selftests/bpf/progs/bpf_misc.h

+12 −0
Original line number Diff line number Diff line
@@ -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))

tools/testing/selftests/bpf/progs/verifier_mtu.c

+3 −1
Original line number Diff line number Diff line
@@ -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;

tools/testing/selftests/bpf/progs/verifier_spill_fill.c

+35 −0
Original line number Diff line number Diff line
@@ -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";