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

[Yonghong Song <yonghong.song@linux.dev>]

On 9/25/26 10:18 PM, sashiko-bot@kernel.org wrote:
> 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.

Indeed, maybe an array of global's can work. I will explore.

>
>> +	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.

Indeed, a bug. Will fix.

>
>> +		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?

the same as previous, maybe an array of globals.

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