Re: [PATCH bpf-next] bpf: Fix out-of-bounds btf->types write for split BTF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: unchecked btf_add_type() return value in btf_check_all_metas()
--

commit 4ac38a4f66ed4b8171a028c6c026eb14f1efb57a
Author: Feng Yang <yangfeng@kylinos.cn>

bpf: Fix out-of-bounds btf->types write for split BTF

This commit bounds the btf->types array growth in btf_add_type() by
subtracting btf->start_id from the maximum type limit and replacing the
equality check with a greater-than-or-equal check to prevent out-of-bounds
writes when parsing split BTF.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 538129ed24188..7949704712791 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1752,15 +1752,17 @@ static int btf_add_type(struct btf_verifier_env *env, struct btf_type *t)
>  		/* Expand 'types' array */
>  
>  		struct btf_type **new_types;
> -		u32 expand_by, new_size;
> +		u32 expand_by, new_size, max_size;
>  
> -		if (btf->start_id + btf->types_size == BTF_MAX_TYPE) {
> +		if (btf->start_id >= BTF_MAX_TYPE ||
> +		    btf->start_id + btf->types_size >= BTF_MAX_TYPE) {
>  			btf_verifier_log(env, "Exceeded max num of types");
>  			return -E2BIG;
>  		}

[Severity: High]
Does returning -E2BIG (or -ENOMEM when kvzalloc_objs() fails) in
btf_add_type() actually stop BTF verification when its sole caller,
btf_check_all_metas(), ignores the return value?

In kernel/bpf/btf.c:btf_check_all_metas():

	while (cur < end) {
		struct btf_type *t = cur;
		s32 meta_size;

		meta_size = btf_check_meta(env, t, end - cur);
		if (meta_size < 0)
			return meta_size;

		btf_add_type(env, t);
		cur += meta_size;
		env->log_type_id++;
	}

	return 0;

When btf_add_type() returns an error here, it returns before appending the
type to btf->types[btf->nr_types++], but btf_check_all_metas() continues
looping over the remaining types and returns 0.

In the btf_parse() path, btf_check_all_types() only iterates up to
btf->nr_types, so if no earlier type references the dropped tail types,
verification succeeds with a silently truncated btf->types array.

In the btf_parse_base() and btf_parse_split() paths, btf_check_all_types() is
not called at all:

kernel/bpf/btf.c:btf_parse_split() {
	...
	err = btf_check_all_metas(env);
	if (err)
		goto errout;

	err = btf_check_modifier_chain_length(env, btf, btf_nr_types(base_btf));
	if (err)
		goto errout;

	return btf;
}

If a split BTF exceeds the limit and an earlier split BTF type references a
dropped type ID at the boundary (which still passes the metadata check in
btf_check_meta()), btf_parse_module() -> btf_parse_split() succeeds with a
truncated btf->types array. Subsequent relocation in btf_relocate() ->
btf_mark_embedded_composite_type_ids() in tools/lib/bpf/btf_relocate.c then
looks up that dropped ID:

	while (next_id) {
		t = btf_type_by_id(r->btf, next_id);
		switch (btf_kind(t)) {

Because next_id is >= btf->start_id + btf->nr_types, btf_type_by_id() returns
null and btf_kind(t) dereferences it, while btf_relocate_rewrite_type_id() can
also read past the end of r->id_map[].

Similarly, if kvzalloc_objs() fails in btf_add_type(), btf_check_all_metas()
can return 0 with btf->types left null or with types shifted after a transient
allocation failure.

Should btf_check_all_metas() check and propagate the error returned by
btf_add_type()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023063133.563998-1-yangfeng59949@163.com?part=1