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

On Wed, Jul 29, 2026 at 08:51:36AM +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

not possible, replied in previous version

jirka