Skip to content

bpf: add support for KASAN checks in JITed programs - #13402

Open
kernel-patches-daemon-bpf[bot] wants to merge 9 commits into
bpf-next_basefrom
series/1150032=>bpf-next
Open

bpf: add support for KASAN checks in JITed programs#13402
kernel-patches-daemon-bpf[bot] wants to merge 9 commits into
bpf-next_basefrom
series/1150032=>bpf-next

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: bpf: add support for KASAN checks in JITed programs
version: 7
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150032

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 669e4fa
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150032
version: 7

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 2e6d9b07002724f48ee4e20da680ee9b0380111a
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

selftests/bpf: move bpf_jit_harden helper into testing_helpers

Move set_bpf_jit_harden to testing helpers so that other selftests can
change the hardening configuration without re-implementing a helper.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_insn_array.c b/tools/testing/selftests/bpf/prog_tests/bpf_insn_array.c
> index 0222a9a5d076..815f3e04540f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_insn_array.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_insn_array.c

[ ... ]

> @@ -272,7 +236,7 @@ static void check_blindness(void)
>  		BPF_MOV64_IMM(BPF_REG_0, 1),
>  		BPF_EXIT_INSN(),
>  	};
> -	int prog_fd = -1, map_fd;
> +	int prog_fd = -1, map_fd, ret;
>  	struct bpf_insn_array_value val = {};
>  	char bpf_jit_harden = '@'; /* non-exizsting value */
>  	int i;
> @@ -291,7 +255,8 @@ static void check_blindness(void)
>  		goto cleanup;
>
>  	bpf_jit_harden = '2';
> -	if (set_bpf_jit_harden(&bpf_jit_harden)) {
> +	ret = set_bpf_jit_harden(&bpf_jit_harden);
> +	if (!ASSERT_OK(ret, "set bpf_jit_harden")) {

This isn't a bug, but could the first call just be
`if (!ASSERT_OK(set_bpf_jit_harden(&bpf_jit_harden), "set bpf_jit_harden"))`
instead?  It would match the restore call below and drop the extra `ret`
local:

>  cleanup:
>  	/* restore the old one */
>  	if (bpf_jit_harden != '@')
> -		set_bpf_jit_harden(&bpf_jit_harden);
> +		ASSERT_OK(set_bpf_jit_harden(&bpf_jit_harden),
> +			  "restore hardening configuration");

> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index c970e7793dfc..737f668b35e2 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -519,6 +519,38 @@ bool is_jit_enabled(void)
>  	return enabled;
>  }
>
> +int set_bpf_jit_harden(char *level)
> +{
> +	char old_level;
> +	int err = -1;
> +	int fd = -1;
> +
> +	fd = open("/proc/sys/net/core/bpf_jit_harden", O_RDWR | O_NONBLOCK);
> +	if (fd < 0)
> +		return -1;
> +
> +	err = read(fd, &old_level, 1);
> +	if (err != 1) {
> +		err = -1;
> +		goto end;
> +	}
> +
> +	lseek(fd, 0, SEEK_SET);
> +
> +	err = write(fd, level, 1);
> +	if (err != 1) {
> +		err = -1;
> +		goto end;
> +	}
> +
> +	err = 0;
> +	*level = old_level;
> +end:
> +	if (fd >= 0)
> +		close(fd);
> +	return err;
> +}

tools/testing/selftests/bpf/ already has a shared sysctl helper with these
semantics.  Is there a reason to add a second sysctl read-modify-write
implementation instead of wrapping the existing one?

sysctl_helpers.h (same directory) exports:

    int sysctl_set(const char *sysctl_path, char *old_val,
                   const char *new_val);
    int sysctl_set_or_fail(const char *sysctl_path, char *old_val,
                           const char *new_val);

and sysctl_helpers.c implements 'open the sysctl, save the previous value
into old_val, write new_val, return -errno on failure' - the same contract
the new set_bpf_jit_harden() open-codes with
open()/read()/lseek()/write()/close(). sysctl_set_or_fail() additionally
emits the PRINT_FAIL diagnostic that the moved helper drops.

sysctl_helpers.c is already linked into test_progs (Makefile
TRUNNER_EXTRA_SOURCES) and is already used by three prog_tests
(test_sysctl.c, token.c, unpriv_bpf_disabled.c). The only consumers of
set_bpf_jit_harden() are test_progs subtests (prog_tests/bpf_insn_array.c
here, and prog_tests/kasan.c added by the follow-up commit 7af63bad7fc6), so
sysctl_set() is reachable from every call site.

Since this patch's stated purpose is precisely to provide a shared way to
change the hardening configuration, landing a second sysctl
set-and-restore implementation means a corner-case fix in one (short
read/write handling, values longer than one byte, error reporting) will not
propagate to the other.

The immediately preceding function in the same file, is_jit_enabled() at
testing_helpers.c:505, already hand-opens /proc/sys/net/core/bpf_jit_enable
with open()/read()/close(), so the new code matches its neighbour.  Also,
the patch is a pure code motion - the body is byte-identical to the version
removed from bpf_insn_array.c apart from the deleted ASSERT_FAIL calls - so
it does not newly introduce the hand-rolled I/O, it only relocates it.
sysctl_set() reads the old value with an unbounded fscanf(fp, "%s",
old_val), which would require the caller to pass a sized buffer rather than
the single char the callers use today, so a straight substitution is not
free.

Worth asking whether set_bpf_jit_harden() should be a thin wrapper over
sysctl_set("/proc/sys/net/core/bpf_jit_harden", ...) instead?

---

This isn't a bug, but the patch is not a pure move: the three ASSERT_FAIL()
calls that reported which of open/read/write failed (with errno) are dropped
from the helper, and two ASSERT_OK() checks are added at the call sites
instead.

The reason that rework is required is not obvious from the diff -
testing_helpers.c contains no ASSERT_*/PRINT_FAIL uses at all, and
testing_helpers.o is linked into standalone binaries (test_maps, test_tag,
test_sockmap, bench) that do not provide the test_progs failure machinery,
which is why network_helpers.c's users have to define stub test__fail() in
xdp_features.c, xskxceiver.c and xdp_hw_metadata.c.

A reader also loses the ability to tell open/read/write apart, while the
retained comment at the call site still says "open, read or write failed =>
no write was done".

Would it be worth a sentence noting that the ASSERT_FAIL() diagnostics had
to move to the callers because testing_helpers.c is linked into binaries
without the test_progs assertion machinery?



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: selftests/bpf: move bpf_jit_harden helper into testing_helpers
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32534828209

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-review-bot

Copy link
Copy Markdown
commit f538e9d6de9a5cb4ac3d8b8eeabc6a8084d18dcb
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

bpf: mark instructions accessing program stack

This commit adds infrastructure to mark BPF instructions that access
non-stack memory, enabling JIT compilers to emit KASAN checks only for
those instructions while skipping stack accesses.

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 004b06785521..412a13a664fa 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -725,6 +725,8 @@ struct bpf_insn_aux_data {
>  	u16 const_reg_map_mask;
>  	u16 const_reg_subprog_mask;
>  	u32 const_reg_vals[10];
> +	/* instruction can access non-stack memory */
> +	bool non_stack_access;
>  };

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e036ae20bf6b..040af75d9550 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3272,6 +3272,11 @@ static void mark_indirect_target(struct bpf_verifier_env *env, int idx)
>  	env->insn_aux_data[idx].indirect_target = true;
>  }
>
> +static void mark_non_stack_access(struct bpf_verifier_env *env, int idx)
> +{
> +	env->insn_aux_data[idx].non_stack_access = true;
> +}
> +

[ ... ]

> @@ -6654,6 +6659,10 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b
>  				clear_scalar_id(&regs[value_regno]);
>  		}
>  	}
> +
> +	if (!err && reg->type != PTR_TO_STACK)
> +		mark_non_stack_access(env, insn_idx);
> +
>  	return err;
>  }

Does this test read the correct register type? For a load instruction where
dst_reg == src_reg (e.g., rX = *(u64 *)(rX + off)), check_mem_access()
receives reg = &regs[src_reg] and value_regno = dst_reg, so reg points to
the same slot that gets overwritten by the load.

When the pointer is PTR_TO_STACK, check_stack_read() runs:

check_stack_read_fixed_off() {
    state->regs[dst_regno] = *reg;  /* or mark_reg_stack_read() */
}

By the time execution reaches the new test, reg->type is the loaded value's
type (typically SCALAR_VALUE), not PTR_TO_STACK. The condition evaluates to
true and marks a stack-only access as non-stack.

Both check_load_mem() and check_store_reg() snapshot the pointer type before
calling check_mem_access():

    /* verifier.c check_load_mem */
    src_reg_type = regs[insn->src_reg].type;
    err = check_mem_access(env, env->insn_idx, ...);

    /* verifier.c check_store_reg */
    dst_reg_type = regs[insn->dst_reg].type;
    err = check_mem_access(env, env->insn_idx, ...);

Could check_mem_access() snapshot reg->type at entry and test that snapshot
instead?



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf: mark instructions accessing program stack
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32534828209

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 3c340c014e414e0a46ee743404e89f53c18ff31e
Author: Alexis Lothore (eBPF Foundation) <alexis.lothore@bootlin.com>

bpf, x86: emit KASAN checks in x86 JITed programs

This commit adds KASAN shadow memory checks before memory load and store
operations in x86 JIT-compiled BPF programs to detect memory safety bugs
at runtime.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 13ba3232993a..8134d028a2f2 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -21,6 +21,17 @@
>  #include <asm/unwind.h>
>  #include <asm/cfi.h>
>
> +#if IS_ENABLED(CONFIG_BPF_JIT_KASAN)
> +void __asan_load1(void *p);
> +void __asan_store1(void *p);
> +void __asan_load2(void *p);
> +void __asan_store2(void *p);
> +void __asan_load4(void *p);
> +void __asan_store4(void *p);
> +void __asan_load8(void *p);
> +void __asan_store8(void *p);
> +#endif
> +

This isn't a bug, but these eight prototypes duplicate declarations from
mm/kasan/kasan.h. Would it be worth exporting them from a shared header
such as include/linux/kasan.h under CONFIG_KASAN_GENERIC so each JIT does
not have to re-declare them? Also, this uses #if IS_ENABLED() while the
matching guard in emit_kasan_check() uses #ifdef CONFIG_BPF_JIT_KASAN.

[ ... ]



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf, x86: emit KASAN checks in x86 JITed programs
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32534828209

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 7af63bad7fc6bc506e9279c3747e05a219a15b18
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

selftests/bpf: add tests to validate KASAN on JIT programs

> Add a basic KASAN test runner that loads and test-run programs that can
> trigger memory management bugs. The test captures kernel logs and ensure
> that the expected KASAN splat is emitted by searching for the
> corresponding first lines in the report, hence validated that the needed
> instrumentation has been inserted by the JIT compiler before the
> relevant memory accesses. To allow each test to trigger the expected
> report, the kernel must run with the kasan_multi_shot configuration.
>
> The runner covers different cases and settings: in the nominal case, it
> validates kasan reports on basic instructions (on all supported accesses
> sizes) but also when report _should not_ be emitted (eg: for accesses on
> program stack). The runner also comes with a few specialized tests that
> are then not executed for all sizes/locations:
> - specific atomic ops
> - test for instructions involving different verifier states, with some
>   states flagging memory as stack, and other states as non-stack memory
> - tests that validate the stack marking shifting when a patch is emitted
>   by the verifier (zext/rnd_hi32, constant blindind).
> Most of those tests are able to trigger kasan reports by altering the
> shadow memory (triggering faulty accesses is otherwise complex, because
> of the verifier). A few tests trigger actual faulty accesses (eg
> out-of-bound accesses)
>
> A few of those tests depends on cpuv4 (load_acquire and store_release).
>
>   # ./test_progs -a kasan
>   #171/1   kasan/st_1_not_on_stack:OK
>   #171/2   kasan/st_1_on_stack:OK

[ ... ]

>   #171/29  kasan/simple_atomic_fetch:OK
>   #171/30  kasan/simple_atomic_fetch:OK

The test output pasted here does not match the code being committed. It
shows two subtests both named kasan/simple_atomic_fetch (entries #171/29
and #171/30). With the code in this patch, the simple_atomic_fetch spec
has skip_on_stack_testing = false, so run_subtest_with_size_and_location()
applies the location suffix:

	snprintf(subtest_name, SUBTEST_NAME_MAX_LEN, "%s%s",
		 test->prog_type,
		 test->skip_on_stack_testing ? "" :
		 on_stack		     ? "_on_stack" :
					       "_not_on_stack");

The actual names would be simple_atomic_fetch_not_on_stack and
simple_atomic_fetch_on_stack. The same duplication appears for ldx_patched
(#171/47 and #171/48). Regenerating the test output against this version
of the code would remove the duplicate names, which is important because
duplicate subtest names would make test_progs -t kasan/<name> unable to
select individual cases.

>   #171/47  kasan/ldx_patched:OK
>   #171/48  kasan/ldx_patched:OK

[ ... ]

> diff --git a/tools/testing/selftests/bpf/prog_tests/kasan.c b/tools/testing/selftests/bpf/prog_tests/kasan.c
> new file mode 100644
> index 000000000000..2b424767a0f3
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/kasan.c

[ ... ]

> +#define SUBTEST_NAME_MAX_LEN	128
> +#define PROG_NAME_MAX_LEN	128

[ ... ]

> +struct test_ctx {
> +	__u8  prog_tag[BPF_TAG_SIZE];
> +	struct bpf_object *obj;
> +	int *access_size;
> +	bool skip_load_acq_store_rel;
> +	struct bpf_program *prog;
> +	char prog_name[SUBTEST_NAME_MAX_LEN];
> +	int klog_fd;
> +};

The prog_name buffer is declared with SUBTEST_NAME_MAX_LEN but is written
with PROG_NAME_MAX_LEN as the bound (at run_subtest_with_size_and_location
and run_blinding_subtest). Today both macros are 128, but they are
independent defines with no static assertion linking them. If
PROG_NAME_MAX_LEN were raised (or SUBTEST_NAME_MAX_LEN lowered), the two
writers would overflow into the next struct member. Could the buffer be
sized with the same macro used to bound the writes, or would
sizeof(ctx->prog_name) work at the call sites?

[ ... ]

> +static void run_subtest_with_size_and_location(struct test_ctx *ctx,
> +					       struct test_spec *test,
> +					       int access_size,
> +					       bool on_stack)
> +{
> +	char subtest_name[SUBTEST_NAME_MAX_LEN];
> +
> +	if (test->skip_multi_size_testing) {
> +		snprintf(subtest_name, SUBTEST_NAME_MAX_LEN, "%s%s",
> +			 test->prog_type,
> +			 test->skip_on_stack_testing ? "" :
> +			 on_stack		     ? "_on_stack" :
> +						       "_not_on_stack");
> +	} else {
> +		snprintf(subtest_name, SUBTEST_NAME_MAX_LEN, "%s_%d_%s",
> +			 test->prog_type, access_size,
> +			 on_stack ? "on_stack" : "not_on_stack");
> +	}
> +
> +	snprintf(ctx->prog_name, PROG_NAME_MAX_LEN, "%s%s", test->prog_type,
> +		 test->skip_on_stack_testing ? "" :
> +		 on_stack		     ? "_on_stack" :
> +					       "_not_on_stack");

[ ... ]

> +static void run_blinding_subtest(void)
> +{
> +	struct test_spec blinding_spec = {
> +		.prog_type = "st_blinded",
> +		.is_write = true,
> +	};
> +	char bpf_jit_harden = '2';
> +	struct kasan_harden *skel;
> +	struct test_ctx *ctx;
> +
> +	if (!test__start_subtest("st_blinded"))
> +		return;
> +
> +	ctx = calloc(1, sizeof(*ctx));
> +	if (!ASSERT_OK_PTR(ctx, "alloc blinding ctx"))
> +		return;
> +	ctx->klog_fd = -1;
> +
> +	if (set_bpf_jit_harden(&bpf_jit_harden))
> +		goto free_ctx;

run_blinding_subtest() starts the subtest with test__start_subtest() but
then bails out on set_bpf_jit_harden() failure without recording the
result:

	if (!test__start_subtest("st_blinded"))
		return;
	...
	if (set_bpf_jit_harden(&bpf_jit_harden))
		goto free_ctx;

There is no ASSERT_*() and no test__skip() on that path, so
test__end_subtest() sees error_cnt == 0 and skipped == false and takes the
success branch. The subtest reports OK when the blinded program was never
loaded or run. set_bpf_jit_harden() returns -1 when
open("/proc/sys/net/core/bpf_jit_harden", O_RDWR|O_NONBLOCK) fails or the
read/write fails, which happens in containers or VMs with /proc/sys
read-only, or without privileges to write that sysctl. The only other
caller of this helper, check_blindness() in prog_tests/bpf_insn_array.c,
treats the identical failure as an assertion. Should this do the same
(ASSERT_OK, or an explicit test__skip() with a reason)?

[ ... ]

> +void test_kasan(void)
> +{
> +	struct kasan_write_val val;
> +	struct test_spec *test;
> +	struct test_ctx *ctx;
> +	struct kasan *skel;
> +	__u32 key = 0;
> +	int i, ret;
> +
> +	ctx = calloc(1, sizeof(struct test_ctx));
> +	if (!ASSERT_OK_PTR(ctx, "alloc test ctx"))
> +		return;
> +
> +	if (!is_jit_enabled() || !get_kasan_jit_enabled() ||
> +	    !get_kasan_multi_shot_enabled()) {
> +		test__skip();
> +		goto end;
> +	}

test_kasan() is registered as a parallel-capable test (the entry point is
test_kasan, not serial_test_kasan), yet through run_blinding_subtest() it
does a read-modify-write swap of the machine-global sysctl
net.core.bpf_jit_harden:

	char bpf_jit_harden = '2';
	...
	if (set_bpf_jit_harden(&bpf_jit_harden))   /* writes '2' */
		goto free_ctx;
	...
	set_bpf_jit_harden(&bpf_jit_harden);       /* restores old value */

Under test_progs -j, other worker processes JIT-compile programs
concurrently, so the raised hardening level leaks into unrelated tests
while the window is open. Constant blinding rewrites the emitted JIT
image, which breaks any test that inspects jited output.

More concretely, this patch adds a second racer on that sysctl:
check_blindness() in prog_tests/bpf_insn_array.c performs the same
read-modify-write swap, and test_bpf_insn_array is also a plain test_.
Interleaving the two loses a restore permanently, e.g.

  A reads 0, A writes 2
  B reads 2, B writes 2
  A restores 0
  B restores 2      <- bpf_jit_harden left at 2 after test_progs exits

leaving JIT hardening enabled system-wide after the run, which then
perturbs every subsequent test invocation on that machine. The test also
scans the global /dev/kmsg ring, which other concurrently running tests
are free to write into. Renaming the entry point to serial_test_kasan()
would close both the cross-test interference and the lost-restore
interleaving.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/kasan.c b/tools/testing/selftests/bpf/progs/kasan.c
> new file mode 100644
> index 000000000000..ea29197646b0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/kasan.c

[ ... ]

> +SEC("tcx/ingress")
> +int stx_on_stack(struct __sk_buff *skb)
> +{
> +	struct kasan_test_val val;
> +
> +	bpf_kfunc_kasan_poison(&val, sizeof(struct kasan_test_val));
> +	switch (access_size) {
> +	case 1:
> +		val.data_1 = access_size;
> +		break;
> +	case 2:
> +		val.data_2 = access_size;
> +		break;
> +	case 4:
> +		val.data_4 = access_size;
> +		break;
> +	case 8:
> +		val.data_8 = access_size;
> +		break;
> +	}
> +	bpf_kfunc_kasan_unpoison(&val, sizeof(struct kasan_test_val));
> +	return 0;
> +}

In stx_on_stack and stx_not_on_stack, the value stored is access_size -
the same variable the enclosing switch selects on. Inside case 1: the
compiler knows access_size == 1, so the store degenerates to a
store-immediate.

Specifically, access_size is a plain global in .bss, the switch load and
the re-load in the case arm are separated by no call or store, so GVN CSEs
the second load into the switch condition, and GVN's switch handling
propagates the case value into the successor. The store operand becomes
the literal 1/2/4/8, and clang emits BPF_ST|BPF_MEM, not BPF_STX|BPF_MEM.

That is exactly the distinction the st_* vs stx_* program pair is meant to
cover: arch/x86/net/bpf_jit_comp.c gains two independent emit_kasan_check()
call sites in this series, one under case BPF_ST | BPF_MEM and one under
case BPF_STX | BPF_MEM. If the stx programs compile to BPF_ST they merely
re-run the st tests, and no program in this file covers BPF_STX|BPF_MEM
instrumentation. Nothing fails because the emitted BPF_ST is instrumented
with the same __asan_storeN, so the loss of coverage is invisible. Storing
a value the compiler cannot correlate with the switch selector would pin
the BPF_STX encoding.

[ ... ]

> +SEC("tcx/ingress")
> +int simple_atomic_fetch_on_stack(struct __sk_buff *skb)
> +{
> +	struct kasan_test_val val;
> +
> +	bpf_kfunc_kasan_poison(&val, sizeof(struct kasan_test_val));
> +	__sync_fetch_and_or(&val.data_8, 8);
> +	bpf_kfunc_kasan_unpoison(&val, sizeof(struct kasan_test_val));
> +	return 0;
> +}
> +
> +SEC("tcx/ingress")
> +int simple_atomic_fetch_not_on_stack(struct __sk_buff *skb)
> +{
> +	struct kasan_test_val *val;
> +	__u32 key = 0;
> +
> +	val = bpf_map_lookup_elem(&test_map, &key);
> +	if (!val)
> +		return 0;
> +
> +	bpf_kfunc_kasan_poison(val, sizeof(struct kasan_test_val));
> +	__sync_fetch_and_or(&val->data_8, 8);
> +	bpf_kfunc_kasan_unpoison(val, sizeof(struct kasan_test_val));
> +	return 0;
> +}

The simple_atomic_fetch_{on,not_on}_stack programs discard the result of
__sync_fetch_and_or(), so clang emits the noreturn atomic form
BPF_ATOMIC|BPF_OR, not BPF_OR|BPF_FETCH. The in-tree convention proves
this: progs/atomics.c deliberately keeps two variables per operation,
or64_result = __sync_fetch_and_or(&or64_value, ...) for the fetching
encoding and a separate __sync_fetch_and_or(&or_noreturn_value, ...) whose
only difference is that the result is dropped.

That matters because the fetching encoding is the only thing these two
subtests exist for. In arch/x86/net/bpf_jit_comp.c the fetch case is a
separate, delicate code path:

  bool is_atomic_fetch = (insn->imm == (BPF_AND | BPF_FETCH) ||
                          insn->imm == (BPF_OR  | BPF_FETCH) ||
                          insn->imm == (BPF_XOR | BPF_FETCH));
  if (is_atomic_fetch) {
          pprog = prog;
          emit_mov_reg(&prog, true, BPF_REG_AX, BPF_REG_0);
          ...
          ip += (prog - pprog);
  }

The CMPXCHG-loop retry target is moved to sit after the injected KASAN
call, and ip is manually re-adjusted so that emit_call() computes a
correct relative displacement. None of that is exercised: with BPF_OR (no
FETCH) is_atomic_fetch is false, so the extra ip accounting and the
branch_target placement are never reached. The subtest still passes,
because a non-fetch 8-byte atomic OR is instrumented with __asan_store8
and produces the same Write of size 8 report the runner greps for, so the
gap is silent. Consuming the result (__sink(__sync_fetch_and_or(...)) as
the ldx programs in this same file already do) is what forces the
BPF_OR|BPF_FETCH encoding the test is named after.

[ ... ]

> +SEC("tcx/ingress")
> +int ldx_oob(struct __sk_buff *skb)
> +{
> +	struct bpf_testmod_oob *val;
> +	struct kasan_test_val volatile tmp;
> +
> +	val = bpf_testmod_oob_alloc();
> +	if (!val)
> +		return 0;
> +
> +	switch (access_size) {
> +	case 1:
> +		tmp.data_1 = (__u8)val->redzone_1;
> +		break;
> +	case 2:
> +		tmp.data_2 = (__u16)val->redzone_2;
> +		break;
> +	case 4:
> +		tmp.data_4 = (__u32)val->redzone_4;
> +		break;
> +	case 8:
> +		tmp.data_8 = (__u64)val->redzone_8;
> +		break;
> +	}
> +	bpf_testmod_oob_free(val);
> +	return tmp.data_1;
> +}

ldx_oob() returns tmp.data_1, but tmp.data_1 is only ever assigned on the
access_size == 1 path. In struct kasan_test_val the fields sit at offsets
0, 2, 4 and 8, so the case 2/4/8 stores never touch offset 0. The runner
exercises exactly those sizes: the ldx_oob spec has
skip_on_stack_testing = true and no skip_multi_size_testing, so
run_subtest() calls run_subtest_with_size() for sizes 1, 2, 4 and 8
(subtests ldx_oob_2/4/8_not_on_stack in the commit log). Because tmp is
declared volatile, the compiler cannot substitute the stored value or elide
the load, so on those three subtests the program performs a real read of an
uninitialized BPF stack slot and returns whatever was left there.

Verification of this program depends on env->allow_uninit_stack. All four
switch arms are explored at load time, so the verifier sees a path where
the 8-byte slot at fp-16 is STACK_INVALID when it is read;
check_stack_read_fixed_off() rejects that unless allow_uninit_stack is set,
and bpf_allow_uninit_stack() requires CAP_PERFMON. The program therefore
only loads for a sufficiently privileged caller.

The intent is clearly just to keep the load from being optimized away, and
the sibling programs in this same file already use the established idiom
for that - __sink(val->data_1) etc. Using __sink() here would drop the
volatile stack struct, the uninitialized read and the CAP_PERFMON
dependency in one go.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/kasan_harden.c b/tools/testing/selftests/bpf/progs/kasan_harden.c
> new file mode 100644
> index 000000000000..a2756bbfd529
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/kasan_harden.c

A subsystem pattern flags this as potentially concerning: progs/kasan_harden.c
is a new 42-line BPF object file whose only program, st_blinded(), is a
verbatim copy of the access_size == 1 arm of st_not_on_stack() in
progs/kasan.c:

	val = bpf_map_lookup_elem(&test_map, &key);
	if (!val)
		return 0;

	bpf_kfunc_kasan_poison(val, sizeof(struct kasan_test_val));
	val->data_1 = 0xAA;
	bpf_kfunc_kasan_unpoison(val, sizeof(struct kasan_test_val));

The new file also re-declares struct kasan_test_val, the test_map ARRAY
map and both bpf_kfunc_kasan_poison/unpoison externs, all of which already
exist in progs/kasan.c. Nothing in the program text differs from the
existing test; the only new variable is on the runner side, where
run_blinding_subtest() raises the bpf_jit_harden sysctl to 2 before
loading it so the BPF_ST|BPF_MEM imm store gets rewritten by
bpf_jit_blind_insn(). That makes this a second copy of an existing case,
kept in sync by hand, that exists purely to be loaded under a different
sysctl value.

In BPF selftests one skeleton per progs/*.c is the normal structure, and
the blinding subtest genuinely needs a separately-loaded object:
bpf_jit_harden must be raised before load, and reloading the whole kasan
object (14 programs) under raised harden would slow the test and change
the JIT for every other program in it. A dedicated single-program object
is the cheapest way to get that. The duplicated struct/map/extern
declarations are also unavoidable without introducing a shared header,
since each progs/*.c is compiled independently. The concrete downside is
only maintenance: if struct kasan_test_val or the st_* body changes in
progs/kasan.c, this copy must be updated in lockstep or the two tests
quietly diverge. Could a comment pointing at st_not_on_stack help track
the relationship?

[ ... ]



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: selftests/bpf: add tests to validate KASAN on JIT programs
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32534828209

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: a284ed4
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150032
version: 7

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 5e289c5
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150032
version: 7

In order to prepare to emit KASAN checks in JITed programs, JIT
compilers need to be aware about whether some load/store instructions
are targeting the bpf program stack, as those should not be monitored
(we already have guard pages for that, and it is difficult anyway to
correctly monitor any kind of data passed on stack).

To support this need, make the BPF verifier mark the instructions
depending on whether they could access or not memory other than stack.
As different states in the verifier could lead to different memory types
for the same access, just marking an instruction as accessing stack only
is not enough (it could be some other memory type in another verifier
state), so the algorithm rather sets by default any load/store
instruction as stack only, and if _any_ state leads to any memory access
type other than PTR_TO_STACK, it overrides this setting. It also takes
care about shifting back the instruction marking in adjust_insn_aux_data
if the verifier patches instructions. However, if the verifier generates
new BPF_ST/BPF_STX/BPF_LDX while patching some instructions, those new
ones are systematically marked as non-stack-accessing: this may
over-instrument a few memory accessing instructions, but it allows
making sure that we will not miss accidentally any.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
Add a new Kconfig option CONFIG_BPF_JIT_KASAN that automatically enables
generic KASAN (Kernel Address SANitizer) memory access checks for
JIT-compiled BPF programs as well, when both KASAN (and more
specifically, generic KASAN with KASAN_VMALLOC) and JIT compiler are
enabled. This new Kconfig is not a user selectable one: it is
automatically enabled if KASAN is enabled on a compatible platform. When
enabled, the JIT compiler will emit shadow memory checks before memory
loads and stores to detect use-after-free or out-of-bounds accesses at
runtime. The option is gated behind HAVE_EBPF_JIT_KASAN, as it needs
proper arch-specific implementation.

Acked-by: Andrey Konovalov <andreyknvl@gmail.com>
Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
In order to prepare for KASAN checks insertion before every
memory-related load or store, group all BPF_ST instructions that indeed
access memory in a single helper to allow instrumenting those in one
call, rather than having to instrument all cases individually.

Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
Insert KASAN shadow memory checks before memory load and store
operations in JIT-compiled BPF programs. This helps detect memory safety
bugs such as use-after-free and out-of-bounds accesses at runtime.

The main instructions being targeted are BPF_ST, BPF_STX and BPF_LDX,
but not all of them are being instrumented:
- if the load/store instruction is in fact accessing the program stack,
  emit_kasan_check silently skips the instrumentation, as we can already
  benefit from guard pages to monitor stack accesses.
- if the load/store instruction is a BPF_PROBE_MEM or a BPF_PROBE_ATOMIC
  instruction, we do not instrument it, as the passed address can fault
  (hence the custom fault management with BPF_PROBE_XXX instructions),
  and so the corresponding kasan check could fault as well.

To support those new instructions insertion, create the
emit_kasan_check() helper that emits KASAN shadow memory checks before
memory accesses in JIT-compiled BPF programs. The implementation relies
on the existing __asan_{load,store}X functions from KASAN subsystem. The
helper:
- saves registers. This includes caller-saved registers, but also
  temporary registers, as those were possibly used by the
  affected program. Theoretically, r10 and r11 should be saved as well,
  but the number of called function and their scope being limited, they
  are skipped for the sake of reducing the overhead
- computes the accessed address and stores it in %rdi
- calls the relevant function, depending on the instruction being a load
  or a store, and the size of the access.
- restores registers

The special care needed when inserting this instrumentation comes at the
cost of a non negligeable increase in JITed code size. For example, a
bare

  mov 	0x0(%si),rbx # Load in rbx content at address stored in rsi

becomes

  push    %rax
  push    %rcx
  push    %rdx
  push    %rsi
  push    %rdi
  push    %r8
  push    %r9
  mov     %rsi,%rdi
  call    0xffffffff81da0a60 <__asan_load8>
  pop     %r9
  pop     %r8
  pop     %rdi
  pop     %rsi
  pop     %rdx
  pop     %rcx
  pop     %rax
  mov     0x0(%rsi),rbx

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
Mark x86 as supporting KASAN checks in JITed programs so that the
corresponding JIT compiler inserts checks on the translated
instructions.

Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
cmdline_contains is used by BPF selftests to check the presence of
specific kernel commandline parameters, but it currently suffers from
two issues:
- the read commandline isn't NULL terminated right after the read data
  but only at the end of the buffer, leaving uninitialized bytes that
  are then possibly tokenized
- the comparison of found tokens is done based on the size of found
  token. This could lead to too-short-but-matching tokens to wrongly
  match the search pattern.

Enforce stricter checks in cmdline_contains to avoid accidental matches.

Fixes: 399f618 ("selftests/bpf: Fix selftests broken by mitigations=off")
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
Add two simple helpers to allow checking whether KASAN for eBPF tests
should be executed:
- one helper to check if BPF_JIT_KASAN is enabled in kernel
  configuration
- one helper to check if the kernel is running with kasan_multi_shot
  (otherwise only the first test will be able to trigger a report)

Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
Move set_bpf_jit_harden to testing helpers so that other selftests can
change the hardening configuration without re-implementing a helper.

Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
Add a basic KASAN test runner that loads and test-run programs that can
trigger memory management bugs. The test captures kernel logs and ensure
that the expected KASAN splat is emitted by searching for the
corresponding first lines in the report, hence validated that the needed
instrumentation has been inserted by the JIT compiler before the
relevant memory accesses. To allow each test to trigger the expected
report, the kernel must run with the kasan_multi_shot configuration.

The runner covers different cases and settings: in the nominal case, it
validates kasan reports on basic instructions (on all supported accesses
sizes) but also when report _should not_ be emitted (eg: for accesses on
program stack). The runner also comes with a few specialized tests that
are then not executed for all sizes/locations:
- specific atomic ops
- test for instructions involving different verifier states, with some
  states flagging memory as stack, and other states as non-stack memory
- tests that validate the stack marking shifting when a patch is emitted
  by the verifier (zext/rnd_hi32, constant blindind).
Most of those tests are able to trigger kasan reports by altering the
shadow memory (triggering faulty accesses is otherwise complex, because
of the verifier). A few tests trigger actual faulty accesses (eg
out-of-bound accesses)

A few of those tests depends on cpuv4 (load_acquire and store_release).

  # ./test_progs -a kasan
  #171/1   kasan/st_1_not_on_stack:OK
  #171/2   kasan/st_1_on_stack:OK
  #171/3   kasan/st_2_not_on_stack:OK
  #171/4   kasan/st_2_on_stack:OK
  #171/5   kasan/st_4_not_on_stack:OK
  #171/6   kasan/st_4_on_stack:OK
  #171/7   kasan/st_8_not_on_stack:OK
  #171/8   kasan/st_8_on_stack:OK
  #171/9   kasan/stx_1_not_on_stack:OK
  #171/10  kasan/stx_1_on_stack:OK
  #171/11  kasan/stx_2_not_on_stack:OK
  #171/12  kasan/stx_2_on_stack:OK
  #171/13  kasan/stx_4_not_on_stack:OK
  #171/14  kasan/stx_4_on_stack:OK
  #171/15  kasan/stx_8_not_on_stack:OK
  #171/16  kasan/stx_8_on_stack:OK
  #171/17  kasan/ldx_1_not_on_stack:OK
  #171/18  kasan/ldx_1_on_stack:OK
  #171/19  kasan/ldx_2_not_on_stack:OK
  #171/20  kasan/ldx_2_on_stack:OK
  #171/21  kasan/ldx_4_not_on_stack:OK
  #171/22  kasan/ldx_4_on_stack:OK
  #171/23  kasan/ldx_8_not_on_stack:OK
  #171/24  kasan/ldx_8_on_stack:OK
  #171/25  kasan/simple_atomic_4_not_on_stack:OK
  #171/26  kasan/simple_atomic_4_on_stack:OK
  #171/27  kasan/simple_atomic_8_not_on_stack:OK
  #171/28  kasan/simple_atomic_8_on_stack:OK
  #171/29  kasan/simple_atomic_fetch:OK
  #171/30  kasan/simple_atomic_fetch:OK
  #171/31  kasan/load_acquire_1_not_on_stack:SKIP
  #171/32  kasan/load_acquire_1_on_stack:SKIP
  #171/33  kasan/load_acquire_2_not_on_stack:SKIP
  #171/34  kasan/load_acquire_2_on_stack:SKIP
  #171/35  kasan/load_acquire_4_not_on_stack:SKIP
  #171/36  kasan/load_acquire_4_on_stack:SKIP
  #171/37  kasan/load_acquire_8_not_on_stack:SKIP
  #171/38  kasan/load_acquire_8_on_stack:SKIP
  #171/39  kasan/store_release_1_not_on_stack:SKIP
  #171/40  kasan/store_release_1_on_stack:SKIP
  #171/41  kasan/store_release_2_not_on_stack:SKIP
  #171/42  kasan/store_release_2_on_stack:SKIP
  #171/43  kasan/store_release_4_not_on_stack:SKIP
  #171/44  kasan/store_release_4_on_stack:SKIP
  #171/45  kasan/store_release_8_not_on_stack:SKIP
  #171/46  kasan/store_release_8_on_stack:SKIP
  #171/47  kasan/ldx_patched:OK
  #171/48  kasan/ldx_patched:OK
  #171/49  kasan/verifier_paths_stack_and_non_stack:OK
  #171/50  kasan/ldx_oob_1_not_on_stack:OK
  #171/51  kasan/ldx_oob_2_not_on_stack:OK
  #171/52  kasan/ldx_oob_4_not_on_stack:OK
  #171/53  kasan/ldx_oob_8_not_on_stack:OK
  #171/54  kasan/st_blinded:OK
  #171     kasan:OK (SKIP: 16/54)
  Summary: 1/38 PASSED, 16 SKIPPED, 0 FAILED

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 5e289c5
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150032
version: 7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant