Re: [PATCHv2 bpf-next 06/11] bpf: Factor callchain_store function from __bpf_get_stack

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Preemption can cause `trace->nr` to be modified concurrently, leading to an integer underflow in `trace_nr = trace->nr - skip` and a massive out-of-bounds `memcpy`.
--

--- Patch [1]: [PATCHv2 bpf-next 06/11] bpf: Factor callchain_store function from __bpf_get_stack ---
commit 41f5133836d848295ac407c006940d4f0641b50f
Author: Jiri Olsa <jolsa@kernel.org>

bpf: Factor callchain_store function from __bpf_get_stack

This commit factors out the callchain storage logic from __bpf_get_stack
into a new callchain_store function. The new function handles both IP and
build ID data copying.

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 41fe87d7302f2..18d1a10bf76e5 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -680,6 +680,29 @@ const struct bpf_func_proto bpf_get_stackid_proto_pe = {
>  	.arg3_type	= ARG_ANYTHING,
>  };
>  
> +static u32 callchain_store(struct perf_callchain_entry *trace, void *buf,
> +			   u32 elem_size, u64 flags)
> +{
> +	bool user_build_id = flags & BPF_F_USER_BUILD_ID;
> +	u32 skip = flags & BPF_F_SKIP_FIELD_MASK;
> +	u32 trace_nr, copy_len;
> +	u64 *ips;
> +
> +	trace_nr = trace->nr - skip;

[Severity: Critical]
This is a pre-existing issue, but could this calculation underflow if
trace->nr is modified concurrently?

Looking at __bpf_get_stack(), when called from a sleepable BPF program via
bpf_get_stack_sleepable(), it calls get_perf_callchain(). This allocates
a per-CPU recursion buffer but unpins it by calling put_callchain_entry()
before returning.

__bpf_get_stack() then checks if trace->nr is less than skip:

	if (unlikely(!trace) || trace->nr < skip) {
		if (may_fault)
			rcu_read_unlock();
		goto err_fault;
	}

If the task is preempted immediately after this check (which is possible for
sleepable BPF programs under CONFIG_PREEMPT_RCU, as rcu_read_lock() does not
prevent preemption), could a new task on the same CPU trigger a BPF program
that reacquires the same level-0 recursion buffer and overwrites trace->nr
to 0?

When the original task resumes and calls callchain_store(), it re-reads
trace->nr. If it is now 0, wouldn't 0 - skip underflow to a massive unsigned
value?

> +	copy_len = trace_nr * elem_size;
> +
> +	ips = trace->ip + skip;
> +	if (user_build_id) {
> +		struct bpf_stack_build_id *id_offs = buf;
> +
> +		for (u32 i = 0; i < trace_nr; i++)
> +			id_offs[i].ip = ips[i];
> +	} else {
> +		memcpy(buf, ips, copy_len);

If the underflow occurs, would this result in a massive out-of-bounds memcpy
that could overwrite the BPF stack buffer or map value and cause memory
corruption?

> +	}
> +	return trace_nr;
> +}
> +
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729083807.1588544-7-jolsa@kernel.org?part=1