Re: [PATCH bpf-next v2 18/43] bpf: Bound the nesting that the BTF show code follows

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: btf_show_obj_safe() clobbers -E2BIG from btf_show_start_aggr_type()
- [Medium] bpf: btf_show_start_array_type() leaks array_encoding on max depth return
--

commit 759371ec51181daa62496793db6556044a061470
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Bound the nesting that the BTF show code follows

This patch limits BTF struct and array pretty-printing recursion to 32
levels (BTF_SHOW_MAX_DEPTH) to prevent kernel stack overflow when showing
deeply nested BTF types, recording -E2BIG in show->state.status when the
limit is reached.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 64c78434fa050..a9bf4c3c0a273 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1081,6 +1081,8 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
>  }
>  
>  #define BTF_SHOW_MAX_ITER	10
> +/* nested structs and arrays shown; __btf_show_indent() has as many columns */
> +#define BTF_SHOW_MAX_DEPTH	32
>  
>  #define BTF_KIND_BIT(kind)	(1ULL << kind)
>  
> @@ -1474,8 +1476,18 @@ static void *btf_show_start_aggr_type(struct btf_show *show,
>  				      const struct btf_type *t,
>  				      u32 type_id, void *data)
>  {
> -	void *safe_data = btf_show_start_type(show, t, type_id, data);
> +	void *safe_data;
>  
> +	/*
> +	 * Structs and arrays are shown by recursion.  BTF loading bounds the
> +	 * depth of one resolve walk, not how deep resolved types may nest.
> +	 */
> +	if (show->state.depth >= BTF_SHOW_MAX_DEPTH) {
> +		show->state.status = -E2BIG;
> +		return NULL;
> +	}

[Severity: Medium]
Can show->state.status = -E2BIG be overwritten by subsequent members before
btf_type_snprintf_show() or btf_type_seq_show_flags() returns it?

Returning NULL from btf_show_start_aggr_type() only causes the current
__btf_struct_show() or __btf_array_show() call at BTF_SHOW_MAX_DEPTH to return
early. The enclosing __btf_struct_show() and __btf_array_show() loops at
shallower depths continue iterating over remaining members (and execute a
second pass when BTF_SHOW_ZERO is not set and a shallower member is non-zero).

When BTF_SHOW_UNSAFE is not set (such as in bpf_snprintf_btf() and
bpf_seq_printf_btf()), visiting a subsequent member outside the current
32-byte show->obj.safe window calls btf_show_start_type() ->
btf_show_obj_safe():

kernel/bpf/btf.c:btf_show_obj_safe() {
	...
	if (!safe) {
		size_left = btf_show_obj_size_left(show, data);
		if (size_left > BTF_SHOW_OBJ_SAFE_SIZE)
			size_left = BTF_SHOW_OBJ_SAFE_SIZE;
		show->state.status = copy_from_kernel_nofault(show->obj.safe,
							      data, size_left);
		if (!show->state.status) {
			show->obj.data = data;
			safe = show->obj.safe;
		}
	}
	...
}

When copy_from_kernel_nofault() succeeds on that later member, doesn't it
unconditionally reset show->state.status to 0, causing bpf_snprintf_btf() and
bpf_seq_printf_btf() to return success with truncated output instead of
-E2BIG?

[Severity: Medium]
Also, what happens to show->state.array_encoding when an array of 1-byte
integers hits BTF_SHOW_MAX_DEPTH?

In __btf_array_show(), btf_show_start_array_type() sets
show->state.array_encoding before calling btf_show_start_aggr_type():

kernel/bpf/btf.c:btf_show_start_array_type() {
	show->state.array_encoding = array_encoding;
	show->state.array_terminated = 0;
	return btf_show_start_aggr_type(show, t, type_id, data);
}

When btf_show_start_aggr_type() returns NULL at BTF_SHOW_MAX_DEPTH,
__btf_array_show() returns early without calling btf_show_end_array_type() to
reset show->state.array_encoding to 0:

kernel/bpf/btf.c:__btf_array_show() {
	...
	if (!btf_show_start_array_type(show, t, type_id, encoding, data))
		return;
	...
	btf_show_end_array_type(show);
}

If the skipped array had 1-byte integer elements (encoding == BTF_INT_CHAR),
won't show->state.array_encoding remain BTF_INT_CHAR while enclosing structs
continue printing shallower members, causing btf_int_show() to format
subsequent non-array 8-bit integer fields as character literals and treat any
zero byte as a NUL terminator that sets show->state.array_terminated = 1 and
suppresses remaining 8-bit integer fields?

> +
> +	safe_data = btf_show_start_type(show, t, type_id, data);
>  	if (!safe_data)
>  		return safe_data;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017163745.3341787-1-alexei.starovoitov@gmail.com?part=18