Re: [PATCH bpf-next v2 8/8] selftests/bpf: add tests to validate KASAN on JIT programs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Double poison instead of unpoison corrupts KASAN state
- [High] Test fails reliably on default configurations due to disabled KASAN multi-shot
- [Medium] Insufficient read buffer size for /dev/kmsg causes test failure
- [Medium] Uninitialized context program name leads to weakened log validation
- [Medium] Missing file descriptor validation for manual program lookup
- [Low] Multi-line comments violate the BPF subsystem comment style guidelines
--

commit 79343bf831cb7ab53c164d28efe25516ad5dc27f
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
Date:   Thu Jun 4 20:22:06 2026 +0000

    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.

> 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 0000000000000..adf61e230ec98
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/kasan.c

[ ... ]

> +static int read_kernel_logs(int fd, char *buf, size_t max_len)
> +{
> +	char record[512];
> +	size_t total = 0;
> +	ssize_t n;
> +
> +	buf[0] = '\0';
> +	while (1) {
> +		char *msg, *eol;
> +		size_t len;
> +
> +		n = read(fd, record, sizeof(record) - 1);

[Severity: Medium]
Can this buffer be too small for KASAN reports?

If a kernel log message, including its priority, sequence, timestamp, and
facility metadata, exceeds 511 bytes, the kernel's devkmsg_read()
implementation will return -EINVAL.

This causes the read() syscall to fail, which in turn causes the
ASSERT_GE() macro checking the return value to fail and abort the test.
Kernel logs, particularly long KASAN bug reports or concurrent stack traces,
can easily exceed this limit.

> +		if (n < 0) {
> +			if (errno == EAGAIN)
> +				break;
> +			return n;
> +		}
> +		record[n] = '\0';
> +
> +		/* Each kmsg record starts with some metadata, separated
> +		 * from the actual content by a semi-colon
> +		 */

[Severity: Low]
This isn't a bug, but the BPF subsystem guidelines require the opening block
comment marker to be on its own line for multi-line comments.

[ ... ]

> +static int check_kasan_report_in_kernel_logs(char *buf, struct test_ctx *ctx,
> +					     bool is_write, int size)
> +{
> +	char *access_desc_start, *access_desc_end, *tmp;
> +	char access_log[READ_CHUNK_SIZE];
> +	char *kasan_report_start;
> +	int nsize;
> +
> +	snprintf(access_log, READ_CHUNK_SIZE, KASAN_PATTERN_SLAB_UAF,
> +		 ctx->prog_tag[0], ctx->prog_tag[1], ctx->prog_tag[2],
> +		 ctx->prog_tag[3], ctx->prog_tag[4], ctx->prog_tag[5],
> +		 ctx->prog_tag[6], ctx->prog_tag[7], ctx->prog_name);

[Severity: Medium]
Will this properly match the full program name?

In run_subtest_with_size_and_location(), the program name is stored in a
local variable but never copied into ctx->prog_name. Since ctx is allocated
with calloc(), ctx->prog_name remains an empty string.

This causes snprintf to format the string with an empty program name,
weakening the validation as it will only search for the tag instead of the
fully qualified tag and program name.

> +	/* Searched kasan report is valid if
> +	 * - it contains the expected kasan pattern
> +	 * - the next line is the description of the faulty access
> +	 * - faulty access properties match the tested type and size
> +	 */

[Severity: Low]
This isn't a bug, but the opening block comment marker should be on its own
line to follow BPF subsystem styling rules.

[ ... ]

> +static void run_subtest_with_size_and_location(struct test_ctx *ctx,
> +					       struct test_spec *test,
> +					       int access_size,
> +					       bool on_stack)
> +{

[ ... ]

> +	ctx->prog = bpf_object__find_program_by_name(ctx->skel->obj, prog_name);
> +	if (!ASSERT_OK_PTR(ctx->prog, "find test prog"))
> +		return;
> +
> +	info_len = sizeof(info);
> +	memset(&info, 0, info_len);
> +	ret = bpf_prog_get_info_by_fd(bpf_program__fd(ctx->prog), &info,

[Severity: Medium]
Is it safe to use bpf_program__fd() directly without validating it is >= 0?

Although the skeleton is successfully loaded, the BPF subsystem guide requires
explicit file descriptor checks when using manual lookups such as
bpf_object__find_program_by_name() before using the fd.

[ ... ]

> +static struct test_spec tests[] = {

[ ... ]

> +	{
> +		.prog_type = "ldx_patched",
> +		.is_write = false,
> +		.skip_multi_size_testing = true,
> +		.skip_on_stack_testing = true,
> +		.run_size = 4,
> +		/* Make the verifier patch instruction to test
> +		 * adjust_insn_aux_data logic
> +		 */

[Severity: Low]
This isn't a bug, but the opening block comment marker should be on its own
line to follow BPF subsystem styling rules.

[ ... ]

> +void test_kasan(void)
> +{

[ ... ]

> +	for (i = 0; i < ARRAY_SIZE(tests); i++) {
> +		test = &tests[i];
> +		run_subtest(ctx, test);
> +	}

[Severity: High]
Will the tests after the first one fail to capture a KASAN report?

By default, KASAN only prints a bug report for the first invalid memory
access. Subsequent invalid accesses are suppressed unless the kernel is
booted with the kasan_multi_shot command-line parameter.

Since the test suite does not enable multi-shot at runtime or check for its
presence, the first subtest will generate a report and pass, but all
subsequent subtests will fail because KASAN will not print the expected
reports in standard test environments.

> +
> +close:
> +	close(ctx->klog_fd);
> +destroy:
> +	kasan__destroy(ctx->skel);
> +end:
> +	free(ctx);
> +}

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

[ ... ]

> +SEC("tcx/ingress")
> +int stx_on_stack(struct __sk_buff *skb)
> +{
> +	struct kasan_write_val val;
> +
> +	bpf_kfunc_kasan_poison(&val, sizeof(struct kasan_write_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_poison(&val, sizeof(struct kasan_write_val));

[Severity: High]
Does this code inadvertently call bpf_kfunc_kasan_poison() again instead of
unpoisoning?

Leaving the task's stack memory permanently poisoned in the KASAN shadow map
will cause subsequent stack usage by the kernel, such as interrupts or
function calls, to trigger spurious KASAN stack-out-of-bounds or
use-after-free panics.

> +	return 0;
> +}
> +
> +SEC("tcx/ingress")
> +int stx_not_on_stack(struct __sk_buff *skb)
> +{
> +	struct kasan_write_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_write_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_poison(val, sizeof(struct kasan_write_val));

[Severity: High]
Is this also meant to be bpf_kfunc_kasan_unpoison()?  Leaving the map value
poisoned corrupts the KASAN state for future accesses to this map element.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-kasan-v2-0-c066e627fda8@bootlin.com?part=8