* [PATCH bpf-next v5 1/2] bpf: refactor max_depth computation in bpf_get_stack() @ 2025-08-26 21:22 Arnaud Lecomte 2025-08-26 21:23 ` [PATCH bpf-next v5 2/2] bpf: fix stackmap overflow check in __bpf_get_stackid() Arnaud Lecomte 0 siblings, 1 reply; 7+ messages in thread From: Arnaud Lecomte @ 2025-08-26 21:22 UTC (permalink / raw) To: song, yonghong.song, martin.lau Cc: andrii, ast, bpf, daniel, eddyz87, haoluo, john.fastabend, jolsa, kpsingh, linux-kernel, sdf, syzbot+c9b724fbb41cf2538b7b, syzkaller-bugs, Arnaud Lecomte A new helper function stack_map_calculate_max_depth() that computes the max depth for a stackmap. Changes in v2: - Removed the checking 'map_size % map_elem_size' from stack_map_calculate_max_depth - Changed stack_map_calculate_max_depth params name to be more generic Changes in v3: - Changed map size param to size in max depth helper Changes in v4: - Fixed indentation in max depth helper for args Changes in v5: - Bound back trace_nr to num_elem in __bpf_get_stack - Make a copy of sysctl_perf_event_max_stack in stack_map_calculate_max_depth Link to v4: https://lore.kernel.org/all/20250819162652.8776-1-contact@arnaud-lcm.com/ Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com> Acked-by: Yonghong Song <yonghong.song@linux.dev> --- kernel/bpf/stackmap.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c index 3615c06b7dfa..796cc105eacb 100644 --- a/kernel/bpf/stackmap.c +++ b/kernel/bpf/stackmap.c @@ -42,6 +42,28 @@ static inline int stack_map_data_size(struct bpf_map *map) sizeof(struct bpf_stack_build_id) : sizeof(u64); } +/** + * stack_map_calculate_max_depth - Calculate maximum allowed stack trace depth + * @size: Size of the buffer/map value in bytes + * @elem_size: Size of each stack trace element + * @flags: BPF stack trace flags (BPF_F_USER_STACK, BPF_F_USER_BUILD_ID, ...) + * + * Return: Maximum number of stack trace entries that can be safely stored + */ +static u32 stack_map_calculate_max_depth(u32 size, u32 elem_size, u64 flags) +{ + u32 skip = flags & BPF_F_SKIP_FIELD_MASK; + u32 max_depth; + u32 curr_sysctl_max_stack = READ_ONCE(sysctl_perf_event_max_stack); + + max_depth = size / elem_size; + max_depth += skip; + if (max_depth > curr_sysctl_max_stack) + return curr_sysctl_max_stack; + + return max_depth; +} + static int prealloc_elems_and_freelist(struct bpf_stack_map *smap) { u64 elem_size = sizeof(struct stack_map_bucket) + @@ -438,10 +460,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, goto clear; } - num_elem = size / elem_size; - max_depth = num_elem + skip; - if (sysctl_perf_event_max_stack < max_depth) - max_depth = sysctl_perf_event_max_stack; + max_depth = stack_map_calculate_max_depth(size, elem_size, flags); if (may_fault) rcu_read_lock(); /* need RCU for perf's callchain below */ @@ -460,6 +479,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, goto err_fault; } + num_elem = size / elem_size; trace_nr = trace->nr - skip; trace_nr = (trace_nr <= num_elem) ? trace_nr : num_elem; copy_len = trace_nr * elem_size; -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next v5 2/2] bpf: fix stackmap overflow check in __bpf_get_stackid() 2025-08-26 21:22 [PATCH bpf-next v5 1/2] bpf: refactor max_depth computation in bpf_get_stack() Arnaud Lecomte @ 2025-08-26 21:23 ` Arnaud Lecomte 2025-08-29 17:29 ` Alexei Starovoitov 0 siblings, 1 reply; 7+ messages in thread From: Arnaud Lecomte @ 2025-08-26 21:23 UTC (permalink / raw) To: song, yonghong.song, martin.lau Cc: andrii, ast, bpf, daniel, eddyz87, haoluo, john.fastabend, jolsa, kpsingh, linux-kernel, sdf, syzbot+c9b724fbb41cf2538b7b, syzkaller-bugs, Arnaud Lecomte Syzkaller reported a KASAN slab-out-of-bounds write in __bpf_get_stackid() when copying stack trace data. The issue occurs when the perf trace contains more stack entries than the stack map bucket can hold, leading to an out-of-bounds write in the bucket's data array. Changes in v2: - Fixed max_depth names across get stack id Changes in v4: - Removed unnecessary empty line in __bpf_get_stackid Link to v4: https://lore.kernel.org/all/20250813205506.168069-1-contact@arnaud-lcm.com/ Reported-by: syzbot+c9b724fbb41cf2538b7b@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=c9b724fbb41cf2538b7b Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com> Acked-by: Yonghong Song <yonghong.song@linux.dev> --- kernel/bpf/stackmap.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c index 796cc105eacb..ef8269ab8d6f 100644 --- a/kernel/bpf/stackmap.c +++ b/kernel/bpf/stackmap.c @@ -247,7 +247,7 @@ get_callchain_entry_for_task(struct task_struct *task, u32 max_depth) } static long __bpf_get_stackid(struct bpf_map *map, - struct perf_callchain_entry *trace, u64 flags) + struct perf_callchain_entry *trace, u64 flags, u32 max_depth) { struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); struct stack_map_bucket *bucket, *new_bucket, *old_bucket; @@ -263,6 +263,8 @@ static long __bpf_get_stackid(struct bpf_map *map, trace_nr = trace->nr - skip; trace_len = trace_nr * sizeof(u64); + trace_nr = min(trace_nr, max_depth - skip); + ips = trace->ip + skip; hash = jhash2((u32 *)ips, trace_len / sizeof(u32), 0); id = hash & (smap->n_buckets - 1); @@ -322,19 +324,17 @@ static long __bpf_get_stackid(struct bpf_map *map, BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, u64, flags) { - u32 max_depth = map->value_size / stack_map_data_size(map); - u32 skip = flags & BPF_F_SKIP_FIELD_MASK; + u32 elem_size = stack_map_data_size(map); bool user = flags & BPF_F_USER_STACK; struct perf_callchain_entry *trace; bool kernel = !user; + u32 max_depth; if (unlikely(flags & ~(BPF_F_SKIP_FIELD_MASK | BPF_F_USER_STACK | BPF_F_FAST_STACK_CMP | BPF_F_REUSE_STACKID))) return -EINVAL; - max_depth += skip; - if (max_depth > sysctl_perf_event_max_stack) - max_depth = sysctl_perf_event_max_stack; + max_depth = stack_map_calculate_max_depth(map->value_size, elem_size, flags); trace = get_perf_callchain(regs, 0, kernel, user, max_depth, false, false); @@ -343,7 +343,7 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, /* couldn't fetch the stack trace */ return -EFAULT; - return __bpf_get_stackid(map, trace, flags); + return __bpf_get_stackid(map, trace, flags, max_depth); } const struct bpf_func_proto bpf_get_stackid_proto = { @@ -375,6 +375,7 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx, bool kernel, user; __u64 nr_kernel; int ret; + u32 elem_size, max_depth; /* perf_sample_data doesn't have callchain, use bpf_get_stackid */ if (!(event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)) @@ -393,12 +394,13 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx, return -EFAULT; nr_kernel = count_kernel_ip(trace); - + elem_size = stack_map_data_size(map); if (kernel) { __u64 nr = trace->nr; trace->nr = nr_kernel; - ret = __bpf_get_stackid(map, trace, flags); + max_depth = stack_map_calculate_max_depth(map->value_size, elem_size, flags); + ret = __bpf_get_stackid(map, trace, flags, max_depth); /* restore nr */ trace->nr = nr; @@ -410,7 +412,8 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx, return -EFAULT; flags = (flags & ~BPF_F_SKIP_FIELD_MASK) | skip; - ret = __bpf_get_stackid(map, trace, flags); + max_depth = stack_map_calculate_max_depth(map->value_size, elem_size, flags); + ret = __bpf_get_stackid(map, trace, flags, max_depth); } return ret; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v5 2/2] bpf: fix stackmap overflow check in __bpf_get_stackid() 2025-08-26 21:23 ` [PATCH bpf-next v5 2/2] bpf: fix stackmap overflow check in __bpf_get_stackid() Arnaud Lecomte @ 2025-08-29 17:29 ` Alexei Starovoitov 2025-08-29 18:49 ` Song Liu 0 siblings, 1 reply; 7+ messages in thread From: Alexei Starovoitov @ 2025-08-29 17:29 UTC (permalink / raw) To: Arnaud Lecomte Cc: Song Liu, Yonghong Song, Martin KaFai Lau, Andrii Nakryiko, Alexei Starovoitov, bpf, Daniel Borkmann, Eduard, Hao Luo, John Fastabend, Jiri Olsa, KP Singh, LKML, Stanislav Fomichev, syzbot+c9b724fbb41cf2538b7b, syzkaller-bugs On Tue, Aug 26, 2025 at 2:24 PM Arnaud Lecomte <contact@arnaud-lcm.com> wrote: > > Syzkaller reported a KASAN slab-out-of-bounds write in __bpf_get_stackid() > when copying stack trace data. The issue occurs when the perf trace > contains more stack entries than the stack map bucket can hold, > leading to an out-of-bounds write in the bucket's data array. > > Changes in v2: > - Fixed max_depth names across get stack id > > Changes in v4: > - Removed unnecessary empty line in __bpf_get_stackid > > Link to v4: https://lore.kernel.org/all/20250813205506.168069-1-contact@arnaud-lcm.com/ > > Reported-by: syzbot+c9b724fbb41cf2538b7b@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=c9b724fbb41cf2538b7b > Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com> > Acked-by: Yonghong Song <yonghong.song@linux.dev> > --- > kernel/bpf/stackmap.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c > index 796cc105eacb..ef8269ab8d6f 100644 > --- a/kernel/bpf/stackmap.c > +++ b/kernel/bpf/stackmap.c > @@ -247,7 +247,7 @@ get_callchain_entry_for_task(struct task_struct *task, u32 max_depth) > } > > static long __bpf_get_stackid(struct bpf_map *map, > - struct perf_callchain_entry *trace, u64 flags) > + struct perf_callchain_entry *trace, u64 flags, u32 max_depth) > { > struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); > struct stack_map_bucket *bucket, *new_bucket, *old_bucket; > @@ -263,6 +263,8 @@ static long __bpf_get_stackid(struct bpf_map *map, > > trace_nr = trace->nr - skip; > trace_len = trace_nr * sizeof(u64); > + trace_nr = min(trace_nr, max_depth - skip); > + The patch might have fixed this particular syzbot repro with OOB in stackmap-with-buildid case, but above two line looks wrong. trace_len is computed before being capped by max_depth. So non-buildid case below is using memcpy(new_bucket->data, ips, trace_len); so OOB is still there? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v5 2/2] bpf: fix stackmap overflow check in __bpf_get_stackid() 2025-08-29 17:29 ` Alexei Starovoitov @ 2025-08-29 18:49 ` Song Liu 2025-08-30 0:28 ` Alexei Starovoitov 0 siblings, 1 reply; 7+ messages in thread From: Song Liu @ 2025-08-29 18:49 UTC (permalink / raw) To: Alexei Starovoitov Cc: Arnaud Lecomte, Yonghong Song, Martin KaFai Lau, Andrii Nakryiko, Alexei Starovoitov, bpf, Daniel Borkmann, Eduard, Hao Luo, John Fastabend, Jiri Olsa, KP Singh, LKML, Stanislav Fomichev, syzbot+c9b724fbb41cf2538b7b, syzkaller-bugs On Fri, Aug 29, 2025 at 10:29 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: [...] > > > > static long __bpf_get_stackid(struct bpf_map *map, > > - struct perf_callchain_entry *trace, u64 flags) > > + struct perf_callchain_entry *trace, u64 flags, u32 max_depth) > > { > > struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); > > struct stack_map_bucket *bucket, *new_bucket, *old_bucket; > > @@ -263,6 +263,8 @@ static long __bpf_get_stackid(struct bpf_map *map, > > > > trace_nr = trace->nr - skip; > > trace_len = trace_nr * sizeof(u64); > > + trace_nr = min(trace_nr, max_depth - skip); > > + > > The patch might have fixed this particular syzbot repro > with OOB in stackmap-with-buildid case, > but above two line looks wrong. > trace_len is computed before being capped by max_depth. > So non-buildid case below is using > memcpy(new_bucket->data, ips, trace_len); > > so OOB is still there? +1 for this observation. We are calling __bpf_get_stackid() from two functions: bpf_get_stackid and bpf_get_stackid_pe. The check against max_depth is only needed from bpf_get_stackid_pe, so it is better to just check here. I have got the following on top of patch 1/2. This makes more sense to me. PS: The following also includes some clean up in __bpf_get_stack. I include those because it also uses stack_map_calculate_max_depth. Does this look better? Thanks, Song diff --git c/kernel/bpf/stackmap.c w/kernel/bpf/stackmap.c index 796cc105eacb..08554fb146e1 100644 --- c/kernel/bpf/stackmap.c +++ w/kernel/bpf/stackmap.c @@ -262,7 +262,7 @@ static long __bpf_get_stackid(struct bpf_map *map, return -EFAULT; trace_nr = trace->nr - 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); @@ -297,6 +297,7 @@ static long __bpf_get_stackid(struct bpf_map *map, return -EEXIST; } } else { + trace_len = trace_nr * sizeof(u64); if (hash_matches && bucket->nr == trace_nr && memcmp(bucket->data, ips, trace_len) == 0) return id; @@ -322,19 +323,17 @@ static long __bpf_get_stackid(struct bpf_map *map, BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, u64, flags) { - u32 max_depth = map->value_size / stack_map_data_size(map); - u32 skip = flags & BPF_F_SKIP_FIELD_MASK; + u32 elem_size = stack_map_data_size(map); bool user = flags & BPF_F_USER_STACK; struct perf_callchain_entry *trace; bool kernel = !user; + u32 max_depth; if (unlikely(flags & ~(BPF_F_SKIP_FIELD_MASK | BPF_F_USER_STACK | BPF_F_FAST_STACK_CMP | BPF_F_REUSE_STACKID))) return -EINVAL; - max_depth += skip; - if (max_depth > sysctl_perf_event_max_stack) - max_depth = sysctl_perf_event_max_stack; + max_depth = stack_map_calculate_max_depth(map->value_size, elem_size, flags); trace = get_perf_callchain(regs, 0, kernel, user, max_depth, false, false); @@ -375,6 +374,7 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx, bool kernel, user; __u64 nr_kernel; int ret; + u32 elem_size, max_depth; /* perf_sample_data doesn't have callchain, use bpf_get_stackid */ if (!(event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)) @@ -393,11 +393,12 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx, return -EFAULT; nr_kernel = count_kernel_ip(trace); - + elem_size = stack_map_data_size(map); if (kernel) { __u64 nr = trace->nr; - trace->nr = nr_kernel; + max_depth = stack_map_calculate_max_depth(map->value_size, elem_size, flags); + trace->nr = min_t(u32, nr_kernel, max_depth); ret = __bpf_get_stackid(map, trace, flags); /* restore nr */ @@ -410,6 +411,8 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx, return -EFAULT; flags = (flags & ~BPF_F_SKIP_FIELD_MASK) | skip; + max_depth = stack_map_calculate_max_depth(map->value_size, elem_size, flags); + trace->nr = min_t(u32, trace->nr, max_depth); ret = __bpf_get_stackid(map, trace, flags); } return ret; @@ -428,7 +431,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, struct perf_callchain_entry *trace_in, void *buf, u32 size, u64 flags, bool may_fault) { - u32 trace_nr, copy_len, elem_size, num_elem, max_depth; + u32 trace_nr, copy_len, elem_size, max_depth; bool user_build_id = flags & BPF_F_USER_BUILD_ID; bool crosstask = task && task != current; u32 skip = flags & BPF_F_SKIP_FIELD_MASK; @@ -465,13 +468,15 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, if (may_fault) rcu_read_lock(); /* need RCU for perf's callchain below */ - if (trace_in) + if (trace_in) { trace = trace_in; - else if (kernel && task) + trace->nr = min_t(u32, trace->nr, max_depth); + } else if (kernel && task) { trace = get_callchain_entry_for_task(task, max_depth); - else + } else { trace = get_perf_callchain(regs, 0, kernel, user, max_depth, crosstask, false); + } if (unlikely(!trace) || trace->nr < skip) { if (may_fault) @@ -479,9 +484,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, goto err_fault; } - num_elem = size / elem_size; trace_nr = trace->nr - skip; - trace_nr = (trace_nr <= num_elem) ? trace_nr : num_elem; copy_len = trace_nr * elem_size; ips = trace->ip + skip; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v5 2/2] bpf: fix stackmap overflow check in __bpf_get_stackid() 2025-08-29 18:49 ` Song Liu @ 2025-08-30 0:28 ` Alexei Starovoitov 2025-08-30 17:13 ` Lecomte, Arnaud 0 siblings, 1 reply; 7+ messages in thread From: Alexei Starovoitov @ 2025-08-30 0:28 UTC (permalink / raw) To: Song Liu Cc: Arnaud Lecomte, Yonghong Song, Martin KaFai Lau, Andrii Nakryiko, Alexei Starovoitov, bpf, Daniel Borkmann, Eduard, Hao Luo, John Fastabend, Jiri Olsa, KP Singh, LKML, Stanislav Fomichev, syzbot+c9b724fbb41cf2538b7b, syzkaller-bugs On Fri, Aug 29, 2025 at 11:50 AM Song Liu <song@kernel.org> wrote: > > On Fri, Aug 29, 2025 at 10:29 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > [...] > > > > > > static long __bpf_get_stackid(struct bpf_map *map, > > > - struct perf_callchain_entry *trace, u64 flags) > > > + struct perf_callchain_entry *trace, u64 flags, u32 max_depth) > > > { > > > struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); > > > struct stack_map_bucket *bucket, *new_bucket, *old_bucket; > > > @@ -263,6 +263,8 @@ static long __bpf_get_stackid(struct bpf_map *map, > > > > > > trace_nr = trace->nr - skip; > > > trace_len = trace_nr * sizeof(u64); > > > + trace_nr = min(trace_nr, max_depth - skip); > > > + > > > > The patch might have fixed this particular syzbot repro > > with OOB in stackmap-with-buildid case, > > but above two line looks wrong. > > trace_len is computed before being capped by max_depth. > > So non-buildid case below is using > > memcpy(new_bucket->data, ips, trace_len); > > > > so OOB is still there? > > +1 for this observation. > > We are calling __bpf_get_stackid() from two functions: bpf_get_stackid > and bpf_get_stackid_pe. The check against max_depth is only needed > from bpf_get_stackid_pe, so it is better to just check here. Good point. > I have got the following on top of patch 1/2. This makes more sense to > me. > > PS: The following also includes some clean up in __bpf_get_stack. > I include those because it also uses stack_map_calculate_max_depth. > > Does this look better? yeah. It's certainly cleaner to avoid adding extra arg to __bpf_get_stackid() ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v5 2/2] bpf: fix stackmap overflow check in __bpf_get_stackid() 2025-08-30 0:28 ` Alexei Starovoitov @ 2025-08-30 17:13 ` Lecomte, Arnaud 2025-09-01 1:10 ` Alexei Starovoitov 0 siblings, 1 reply; 7+ messages in thread From: Lecomte, Arnaud @ 2025-08-30 17:13 UTC (permalink / raw) To: Alexei Starovoitov, Song Liu Cc: Yonghong Song, Martin KaFai Lau, Andrii Nakryiko, Alexei Starovoitov, bpf, Daniel Borkmann, Eduard, Hao Luo, John Fastabend, Jiri Olsa, KP Singh, LKML, Stanislav Fomichev, syzbot+c9b724fbb41cf2538b7b, syzkaller-bugs On 30/08/2025 02:28, Alexei Starovoitov wrote: > On Fri, Aug 29, 2025 at 11:50 AM Song Liu <song@kernel.org> wrote: >> On Fri, Aug 29, 2025 at 10:29 AM Alexei Starovoitov >> <alexei.starovoitov@gmail.com> wrote: >> [...] >>>> static long __bpf_get_stackid(struct bpf_map *map, >>>> - struct perf_callchain_entry *trace, u64 flags) >>>> + struct perf_callchain_entry *trace, u64 flags, u32 max_depth) >>>> { >>>> struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); >>>> struct stack_map_bucket *bucket, *new_bucket, *old_bucket; >>>> @@ -263,6 +263,8 @@ static long __bpf_get_stackid(struct bpf_map *map, >>>> >>>> trace_nr = trace->nr - skip; >>>> trace_len = trace_nr * sizeof(u64); >>>> + trace_nr = min(trace_nr, max_depth - skip); >>>> + >>> The patch might have fixed this particular syzbot repro >>> with OOB in stackmap-with-buildid case, >>> but above two line looks wrong. >>> trace_len is computed before being capped by max_depth. >>> So non-buildid case below is using >>> memcpy(new_bucket->data, ips, trace_len); >>> >>> so OOB is still there? >> +1 for this observation. >> >> We are calling __bpf_get_stackid() from two functions: bpf_get_stackid >> and bpf_get_stackid_pe. The check against max_depth is only needed >> from bpf_get_stackid_pe, so it is better to just check here. > Good point. Nice catch, thanks ! > >> I have got the following on top of patch 1/2. This makes more sense to >> me. >> >> PS: The following also includes some clean up in __bpf_get_stack. >> I include those because it also uses stack_map_calculate_max_depth. >> >> Does this look better? > yeah. It's certainly cleaner to avoid adding extra arg to > __bpf_get_stackid() > Are Song patches going to be applied then ? Or should I raise a new revision of the patch with Song's modifications with a Co-developped tag ? Thanks for your guidance in advance, Arnaud ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v5 2/2] bpf: fix stackmap overflow check in __bpf_get_stackid() 2025-08-30 17:13 ` Lecomte, Arnaud @ 2025-09-01 1:10 ` Alexei Starovoitov 0 siblings, 0 replies; 7+ messages in thread From: Alexei Starovoitov @ 2025-09-01 1:10 UTC (permalink / raw) To: Lecomte, Arnaud Cc: Song Liu, Yonghong Song, Martin KaFai Lau, Andrii Nakryiko, Alexei Starovoitov, bpf, Daniel Borkmann, Eduard, Hao Luo, John Fastabend, Jiri Olsa, KP Singh, LKML, Stanislav Fomichev, syzbot+c9b724fbb41cf2538b7b, syzkaller-bugs On Sat, Aug 30, 2025 at 10:13 AM Lecomte, Arnaud <contact@arnaud-lcm.com> wrote: > > > On 30/08/2025 02:28, Alexei Starovoitov wrote: > > On Fri, Aug 29, 2025 at 11:50 AM Song Liu <song@kernel.org> wrote: > >> On Fri, Aug 29, 2025 at 10:29 AM Alexei Starovoitov > >> <alexei.starovoitov@gmail.com> wrote: > >> [...] > >>>> static long __bpf_get_stackid(struct bpf_map *map, > >>>> - struct perf_callchain_entry *trace, u64 flags) > >>>> + struct perf_callchain_entry *trace, u64 flags, u32 max_depth) > >>>> { > >>>> struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); > >>>> struct stack_map_bucket *bucket, *new_bucket, *old_bucket; > >>>> @@ -263,6 +263,8 @@ static long __bpf_get_stackid(struct bpf_map *map, > >>>> > >>>> trace_nr = trace->nr - skip; > >>>> trace_len = trace_nr * sizeof(u64); > >>>> + trace_nr = min(trace_nr, max_depth - skip); > >>>> + > >>> The patch might have fixed this particular syzbot repro > >>> with OOB in stackmap-with-buildid case, > >>> but above two line looks wrong. > >>> trace_len is computed before being capped by max_depth. > >>> So non-buildid case below is using > >>> memcpy(new_bucket->data, ips, trace_len); > >>> > >>> so OOB is still there? > >> +1 for this observation. > >> > >> We are calling __bpf_get_stackid() from two functions: bpf_get_stackid > >> and bpf_get_stackid_pe. The check against max_depth is only needed > >> from bpf_get_stackid_pe, so it is better to just check here. > > Good point. > Nice catch, thanks ! > > > >> I have got the following on top of patch 1/2. This makes more sense to > >> me. > >> > >> PS: The following also includes some clean up in __bpf_get_stack. > >> I include those because it also uses stack_map_calculate_max_depth. > >> > >> Does this look better? > > yeah. It's certainly cleaner to avoid adding extra arg to > > __bpf_get_stackid() > > > Are Song patches going to be applied then ? Or should I raise a new > revision > of the patch with Song's modifications with a Co-developped tag ? Pls resubmit and retest with a tag. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-09-01 1:10 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-26 21:22 [PATCH bpf-next v5 1/2] bpf: refactor max_depth computation in bpf_get_stack() Arnaud Lecomte 2025-08-26 21:23 ` [PATCH bpf-next v5 2/2] bpf: fix stackmap overflow check in __bpf_get_stackid() Arnaud Lecomte 2025-08-29 17:29 ` Alexei Starovoitov 2025-08-29 18:49 ` Song Liu 2025-08-30 0:28 ` Alexei Starovoitov 2025-08-30 17:13 ` Lecomte, Arnaud 2025-09-01 1:10 ` Alexei Starovoitov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).