Re: [PATCH bpf-next 17/38] 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 set by btf_show_start_aggr_type()
- [Medium] bpf: btf_show_start_array_type() leaks array_encoding on max depth error
--

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

bpf: Bound the nesting that the BTF show code follows

This commit limits the recursion depth in btf_show_start_aggr_type() to
BTF_SHOW_MAX_DEPTH (32) and sets show->state.status to -E2BIG to prevent
kernel stack overflows when pretty-printing deeply nested BTF structs and
arrays.

> 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 get overwritten by a subsequent call to
btf_show_obj_safe()?

Returning NULL from btf_show_start_aggr_type() stops the current aggregate at
depth 32, but enclosing callers in __btf_struct_show() and __btf_array_show()
continue iterating over their remaining members or elements (as well as running
the second display pass after the depth_check pass in btf_struct_show() and
btf_array_show() when BTF_SHOW_ZERO is not set).

When bpf_snprintf_btf() or bpf_seq_printf_btf() runs, BTF_SHOW_UNSAFE is not
set. If any subsequent member or pass calls
btf_show_start_type()->btf_show_obj_safe() for data outside the current 32-byte
show->obj.safe window, btf_show_obj_safe() unconditionally overwrites
show->state.status:

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, show->state.status is reset to 0, so
btf_type_snprintf_show() and btf_type_seq_show_flags() will return success
instead of -E2BIG.

[Severity: Medium]
Also, when btf_show_start_aggr_type() returns NULL at BTF_SHOW_MAX_DEPTH, does
btf_show_start_array_type() leave show->state.array_encoding set?

In __btf_array_show(), a 1-byte integer array sets encoding = BTF_INT_CHAR and
calls btf_show_start_array_type(), which 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, __btf_array_show() returns early
without calling btf_show_end_array_type() to clear show->state.array_encoding:

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

When recursion unwinds to enclosing structs, any subsequent scalar 8-bit
integer member visited by btf_int_show() will see
show->state.array_encoding == BTF_INT_CHAR, causing printable bytes to be
formatted as character literals and a zero byte to set
show->state.array_terminated = 1, suppressing subsequent 8-bit integer fields:

kernel/bpf/btf.c:btf_int_show() {
	case 8:
		if (show->state.array_encoding == BTF_INT_CHAR) {
			/* check for null terminator */
			if (show->state.array_terminated)
				break;
			if (*(char *)data == '\0') {
				show->state.array_terminated = 1;
				break;
			}
}

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

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