Re: [PATCHv2 bpf-next 01/11] bpf: Factor stackid_init function from __bpf_get_stackid

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A TOCTOU race with `sysctl_perf_event_max_stack` causes an integer underflow in the `max_depth - skip` calculation, leading to a massive heap buffer overflow when copying stack traces.
--

--- Patch [1]: [PATCHv2 bpf-next 01/11] bpf: Factor stackid_init function from __bpf_get_stackid ---
commit 94626e7c900d701c2d2326e06f48ebc28e1f6e93
Author: Jiri Olsa <jolsa@kernel.org>

bpf: Factor stackid_init function from __bpf_get_stackid

The new stackid_init function stores all the necessary bits for stackid
trace and it will be used by other functions in following changes.

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 41fe87d7302f2..0eafe55b18289 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -504,33 +504,54 @@ get_callchain_entry_for_task(struct task_struct *task, u32 max_depth)
[ ... ]
> +static int stackid_init(struct stackid *stackid, struct bpf_map *map,
> +			struct perf_callchain_entry *trace, u64 flags)
>  {
>  	struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map);
> -	struct stack_map_bucket *bucket, *new_bucket, *old_bucket;
> -	u32 hash, id, trace_nr, trace_len, i, max_depth;
>  	u32 skip = flags & BPF_F_SKIP_FIELD_MASK;
> -	bool user = flags & BPF_F_USER_STACK;
> -	u64 *ips;
> -	bool hash_matches;
> +	u32 max_depth;
>  
>  	if (trace->nr <= skip)
>  		/* skipping more than usable stack trace */
>  		return -EFAULT;
>  
>  	max_depth = stack_map_calculate_max_depth(map->value_size, stack_map_data_size(map), flags);
> -	trace_nr = min_t(u32, trace->nr - skip, max_depth - skip);
> -	trace_len = trace_nr * sizeof(u64);
> -	ips = trace->ip + skip;
> -	hash = jhash2((u32 *)ips, trace_len / sizeof(u32), 0);
> -	id = hash & (smap->n_buckets - 1);
> -	bucket = READ_ONCE(smap->buckets[id]);
> +	stackid->nr = min_t(u32, trace->nr - skip, max_depth - skip);

[Severity: High]
This is a pre-existing issue, but can this max_depth - skip calculation
underflow?

If a BPF program is attached to a perf event using bpf_get_stackid_pe(),
the callchain is generated using the current sysctl_perf_event_max_stack
limit. If an administrator concurrently reduces sysctl_perf_event_max_stack to
a value smaller than the skip parameter, stack_map_calculate_max_depth()
clamps max_depth to this new smaller sysctl value:

kernel/bpf/stackmap.c:stack_map_calculate_max_depth() {
    ...
    u32 curr_sysctl_max_stack = READ_ONCE(sysctl_perf_event_max_stack);
    ...
    if (max_depth > curr_sysctl_max_stack)
        return curr_sysctl_max_stack;
    return max_depth;
}

This would cause max_depth - skip to underflow into a huge u32 value. Since
min_t() is used with u32, a large trace->nr - skip could be selected, which
might exceed the stackmap bucket capacity and lead to a heap buffer overflow
when copying the stack trace later in __bpf_get_stackid():

    memcpy(new_bucket->data, stackid->ips, stackid->len);

Does this sequence need to be protected against sysctl changes between trace
gathering and max_depth calculation?

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