Re: [PATCH bpf-next v6 19/21] selftests/bpf: Add __set_global() and __ret_global() test tags

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: selftests: out-of-bounds heap access in access_global_var
- [Medium] bpf: selftests: multiple __set_global or __ret_global tags are silently overwritten
--

commit 81344b890bb87ceeed0ef105e0aab6e9d6797e2d
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Add __set_global() and __ret_global() test tags

This commit adds __set_global() and __ret_global() tags to the BPF
test loader. These tags allow setting a program's global variable before
execution and verifying its value afterwards.

> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c

[ ... ]

> @@ -311,6 +326,142 @@ static int parse_caps(const char *str, __u64 *val, const char *name)
> +static int parse_global_var(const char *str, char **var, __u64 *val, const char *name)
> +{
> +	const char *colon = strrchr(str, ':');
> +	char *end;

[ ... ]

> +		term = end + 1;
> +	}
> +
> +	free(*var);
> +	*var = strndup(str, colon - str);

[Severity: Medium]
If a test author uses multiple __set_global() or __ret_global() tags in a
single test, does this silently overwrite the previous allocations? Only the
last tag seems to take effect, making it impossible to configure multiple
global variables in one test.

> +	if (!*var) {
> +		PRINT_FAIL("failed to allocate %s variable name\n", name);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int access_global_var(struct bpf_object *obj, const char *name,
> +			     __u64 *val, bool set)
> +{
> +	__u32 off, sz, zero = 0;
> +	struct bpf_map *map;
> +	size_t vsz;
> +	void *buf;
> +	int err;
> +
> +	err = find_global_var(obj, name, &map, &off, &sz);
> +	if (err)
> +		return err;
> +
> +	vsz = bpf_map__value_size(map);
> +	buf = calloc(1, vsz);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	err = bpf_map__lookup_elem(map, &zero, sizeof(zero), buf, vsz, 0);
> +	if (err) {
> +		PRINT_FAIL("failed to read '%s': %d\n", name, err);
> +		goto out;
> +	}
> +	if (!set) {
> +		*val = sz == 4 ? *(__u32 *)(buf + off) : *(__u64 *)(buf + off);

[Severity: Medium]
Is it possible to have an out-of-bounds heap read/write here? The function
find_global_var() extracts off and sz from BTF, but off + sz is never
validated against the buffer size vsz before accessing buf + off.

> +		goto out;
> +	}
> +	if (sz == 4)
> +		*(__u32 *)(buf + off) = *val;
> +	else
> +		*(__u64 *)(buf + off) = *val;
> +	err = bpf_map__update_elem(map, &zero, sizeof(zero), buf, vsz, 0);
> +	if (err)
> +		PRINT_FAIL("failed to write '%s': %d\n", name, err);
> +out:
> +	free(buf);
> +	return err;
> +}

[ ... ]

> @@ -557,6 +708,24 @@ static int parse_test_spec(struct test_loader *tester,
>  			spec->mode_mask |= UNPRIV;
>  			spec->unpriv.execute = true;
>  			has_unpriv_retval = true;
> +		} else if ((val = str_has_pfx(s, "test_global_set="))) {
> +			err = parse_global_var(val, &spec->priv.set_global_var,
> +					       &spec->priv.set_global_val,
> +					       "__set_global");

[Severity: Medium]
Storing the variable in a singular field here also means that using multiple
__set_global() or __ret_global() tags will result in the earlier ones being
ignored. Should this support a list of variables instead?

> +			if (err)
> +				goto cleanup;
> +			spec->priv.has_set_global = true;
> +			spec->priv.execute = true;
> +			spec->mode_mask |= PRIV;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926050006.2213110-1-yonghong.song@linux.dev?part=19