Re: [PATCH 3/9] perf thread-stack: Bound wrapped branch stack copy

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Unconditional dereferences of `ts->br_stack_rb` cause a segmentation fault if thread stack initialization fails or if it was initially created without a branch stack.
--

--- Patch [3]: [PATCH 3/9] perf thread-stack: Bound wrapped branch stack copy ---
commit 3efa75c3f04529593a5a6a18971bae8fa6a9617c
Author: Amir Ayupov <aaupov@fb.com>

perf thread-stack: Bound wrapped branch stack copy

This commit fixes a potential destination buffer overrun when copying
wrapped branch stack entries. It correctly limits the number of entries
copied to the remaining destination size.

> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> index 1a3dffa83bde2..51eaedb47bb1d 100644
> --- a/tools/perf/util/thread-stack.c
> +++ b/tools/perf/util/thread-stack.c
> @@ -643,7 +643,7 @@ void thread_stack__br_sample(struct thread *thread, int cpu,

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference earlier in this function.

Could src be NULL when we evaluate src->nr?

tools/perf/util/thread-stack.c:thread_stack__br_sample() {
        ...
        src = ts->br_stack_rb;
        if (!src->nr)
                return;
        ...
}

It looks like this might occur if thread_stack__init() fails to allocate the
branch stack ring buffer, as the partially initialized thread_stack is
already attached to the thread:

tools/perf/util/thread-stack.c:thread_stack__new() {
        ...
        thread__set_ts(thread, new_ts);
        ts = new_ts;
        ...
        if (!ts->stack &&
            thread_stack__init(ts, thread, crp, callstack, br_stack_sz))
                return NULL;
        ...
}

If a caller ignores the error from thread_stack__event(), they might proceed
to call thread_stack__br_sample() or thread_stack__update_br_stack() with a
NULL branch stack ring buffer. Is there a need to check if ts->br_stack_rb
is populated before dereferencing it here?

>  		sz -= nr;
>  		be = &dst->entries[nr];
>  		nr = min(ts->br_stack_pos, sz);
> -		memcpy(be, &src->entries[0], bsz * ts->br_stack_pos);
> +		memcpy(be, &src->entries[0], bsz * nr);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803090640.2412336-1-aaupov@fb.com?part=3