Re: [PATCH v2 1/4] perf timechart: Release event samples at the end

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added timechart__release() function introduces a double-free vulnerability by unconditionally freeing cs->backtrace for all CPU samples, even though sched_switch() shares the exact same backtrace string pointer between multiple samples.

Pre-existing issues:
- [High] Use-after-free inside cat_backtrace() error path due to freeing the memstream buffer before closing the stream.
--

commit ee81a934b42b228370b97d5f0e7e208b04ba839a
Author: Namhyung Kim <namhyung@kernel.org>

perf timechart: Release event samples at the end
    
Add timechart__release() to free all data structures added during the
sample processing.

> diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
> index 28f33e39895d3..99f5102239963 100644
> --- a/tools/perf/builtin-timechart.c
> +++ b/tools/perf/builtin-timechart.c
[ ... ]
> @@ -1509,6 +1509,73 @@ static void write_svg_file(struct timechart *tchart, const char *filename)
>  	svg_close();
>  }
>  
> +static void timechart__release(struct timechart *tchart)
> +{
> +	struct per_pid *p = tchart->all_data;
[ ... ]
> +		while (c) {
> +			struct per_pidcomm *next_comm = c->next;
> +			struct cpu_sample *cs = c->samples;
> +			struct io_sample *ios = c->io_samples;
> +
> +			while (cs) {
> +				struct cpu_sample *next = cs->next;
> +
> +				zfree(&cs->backtrace);

[Severity: High]
Does unconditionally freeing cs->backtrace cause a double-free?

Looking at sched_switch() in tools/perf/builtin-timechart.c, it passes the
same dynamically allocated backtrace string pointer to pid_put_sample() for
both prev_pid and next_pid:

tools/perf/builtin-timechart.c:sched_switch() {
    ...
    pid_put_sample(tchart, prev_pid, TYPE_RUNNING, cpu,
                   prev_p->current->state_since, timestamp,
                   backtrace);
    ...
    pid_put_sample(tchart, next_pid, p->current->state, cpu,
                   p->current->state_since, timestamp,
                   backtrace);
    ...
}

Then pid_put_sample() stores this raw pointer into sample->backtrace without
duplicating it:

tools/perf/builtin-timechart.c:pid_put_sample() {
    ...
    sample->backtrace = backtrace;
    ...
}

Because the pointer is shared among multiple CPU samples, freeing it
unconditionally for every sample here could result in a double-free memory
corruption.

> +				cs->next = NULL;
> +				free(cs);
> +
> +				cs = next;
> +			}
[ ... ]
> @@ -2037,6 +2104,7 @@ int cmd_timechart(int argc, const char **argv)
>  
>  	ret = __cmd_timechart(&tchart, output_name);
>  out:
> +	timechart__release(&tchart);

[Severity: High]
This isn't a bug introduced by this patch, but looking at cat_backtrace() in
the same file, is there a use-after-free in its error path?

When the callchain context is invalid, it attempts to discard the buffer by
freeing p:

tools/perf/builtin-timechart.c:cat_backtrace() {
    ...
    zfree(&p);
    goto exit;
    ...

But the goto exit path closes the stream:

    exit:
        addr_location__exit(&al);
        fclose(f);

        return p;
}

Because fclose(f) operates on the open stream, it accesses its internal
buffer to append a null byte. If p was flushed and freed, wouldn't this
cause a use-after-free?

Additionally, fclose(f) overwrites p with the buffer address, which means
the function might return a freed pointer to the caller.

>  	zfree(&cpus_cstate_start_times);
>  	zfree(&cpus_cstate_state);

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