Re: [PATCH bpf-next v9 7/7] selftests/bpf: Convert veristat to BPF_PROG_LOAD_STRATEGY_MANUAL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: uninitialized log buffer in veristat process_prog()
--

commit fdd973586d17be0beb9ecafccb03a9c1c652febc
Author: Andrey Grodzovsky <andrey.grodzovsky@crowdstrike.com>

selftests/bpf: Convert veristat to BPF_PROG_LOAD_STRATEGY_MANUAL

This patch converts veristat from using bpf_program__clone() to
BPF_PROG_LOAD_STRATEGY_MANUAL with bpf_program__load() and
bpf_program__unload() so that RODATA maps are bound during program
verification. It also adds a test case in test_struct_ops_arena.c covering
manual program loading.

> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 9cfc9b4b41c12..0cd19b59037f6 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -1678,7 +1678,6 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>  	const char *base_filename = basename(strdupa(filename));
>  	const char *prog_name = bpf_program__name(prog);
>  	long mem_peak_a, mem_peak_b, mem_peak = -1;
> -	LIBBPF_OPTS(bpf_prog_load_opts, opts);
>  	char *buf;
>  	int buf_sz, log_level;
>  	struct verif_stats *stats;
> @@ -1726,18 +1725,15 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>  	if (env.force_reg_invariants)
>  		bpf_program__add_flags(prog, BPF_F_TEST_REG_INVARIANTS);
>  
> -	opts.log_buf = buf;
> -	opts.log_size = buf_sz;
> -	opts.log_level = log_level;
> +	bpf_program__set_log_buf(prog, buf, buf_sz);
> +	bpf_program__set_log_level(prog, log_level);
>  
>  	cgroup_err = reset_stat_cgroup();
>  	mem_peak_a = cgroup_memory_peak();
> -	fd = bpf_program__clone(prog, &opts);
> -	if (fd < 0) {
> -		err = fd;
> -		if (env.verbose)
> -			fprintf(stderr, "Failed to load program %s %d\n", prog_name, err);
> -	}
> +	err = bpf_program__load(prog);
> +	fd = err == 0 ? bpf_program__fd(prog) : -1;
> +	if (err && env.verbose)
> +		fprintf(stderr, "Failed to load program %s %d\n", prog_name, err);

[Severity: Medium]
Can bpf_program__load(prog) read uninitialized heap memory or over-read buf
when veristat is run with -v or --top-src-lines?

In tools/testing/selftests/bpf/veristat.c:process_prog(), when env.verbose or
env.top_src_lines > 0 is set, buf is allocated with malloc(buf_sz), but only
verif_log_buf[0] is initialized to '\0':

	if (env.verbose || env.top_src_lines > 0) {
		buf_sz = env.log_size ? env.log_size : max_verifier_log_size();
		buf = malloc(buf_sz);
		if (!buf)
			return -ENOMEM;
		...
	} else {
		buf = verif_log_buf;
		buf_sz = sizeof(verif_log_buf);
		...
	}
	verif_log_buf[0] = '\0';

Previously, bpf_program__clone() did not inspect buf in userspace, and
parse_verif_log() later set buf[buf_sz - 1] = '\0' before calling strlen(buf).

With bpf_program__set_log_buf(prog, buf, buf_sz) and bpf_program__load(prog),
if program loading fails before the kernel verifier writes to the log buffer
(for example, an early check in bpf_prog_load() before bpf_check() emits any
verifier log), bpf_program__load() -> bpf_object_load_prog() calls
fixup_verifier_log() in tools/lib/bpf/libbpf.c:

	/* post-process verifier log to improve error descriptions */
	fixup_verifier_log(prog, log_buf, log_buf_size);

which immediately calls strlen(buf) on the uninitialized heap buffer:

	if (!buf)
		return;

	log_sz = strlen(buf) + 1;

In addition, if bpf_object__prepare() or bpf_program__load() fails without
writing to buf, parse_verif_log(), printf(), and print_top_src_lines() in
process_prog() will read uninitialized or stale log contents from a previous
program's freed buffer, and freeing buf at the end of process_prog() leaves
prog->log_buf pointing to freed memory.

Should process_prog() initialize buf[0] = '\0' (and buf[buf_sz - 1] = '\0')
instead of verif_log_buf[0] = '\0', and clear the log buffer on prog before
freeing buf?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029223348.3730738-1-andrey.grodzovsky@crowdstrike.com?part=7