Re: [PATCH v2 4/6] perf tools: Show memory region in perf-script subcommand

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Silent truncation in `data_src__fprintf` due to inadequately sized destination buffer.

Pre-existing issues:
- [High] Stack buffer overflow in `perf_script__meminfo_scnprintf` and its helpers due to unsafe string operations and integer underflow.
- [High] Out-of-bounds array read in `perf_mem__lvl_scnprintf` when accessing `mem_hops` without bounds checking.
--

--- Patch [4]: [PATCH v2 4/6] perf tools: Show memory region in perf-script subcommand ---
commit 032c0a587669b230205736e968c969992f006ba6
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf tools: Show memory region in perf-script subcommand

This commit adds the memory region to the output of the perf-script
subcommand. The memory region is extracted from the mem_region field
of the memory information data source.

Link: https://lore.kernel.org/all/20260114011750.350569-1-dapeng1.mi@linux.intel.com/

> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 9ac29bdc3cd54..58aaf1dbf6eda 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -2081,8 +2081,8 @@ static int evlist__max_name_len(struct evlist *evlist)
>  static int data_src__fprintf(u64 data_src, FILE *fp)
>  {
>  	struct mem_info *mi = mem_info__new();
> -	char decode[100];
> -	char out[100];
> +	char decode[200];
> +	char out[200];
>  	static int maxlen;
>  	int len;
>  
> @@ -2090,10 +2090,10 @@ static int data_src__fprintf(u64 data_src, FILE *fp)
>  		return -ENOMEM;
>  
>  	mem_info__data_src(mi)->val = data_src;
> -	perf_script__meminfo_scnprintf(decode, 100, mi);
> +	perf_script__meminfo_scnprintf(decode, 200, mi);
>  	mem_info__put(mi);
>  
> -	len = scnprintf(out, 100, "%16" PRIx64 " %s", data_src, decode);
> +	len = scnprintf(out, 200, "%16" PRIx64 " %s", data_src, decode);

[Severity: Medium]
Is the out buffer sized correctly to prevent truncation?

The commit message mentions increasing the buffers to avoid silent
truncation, but the out buffer is exactly the same size as the decode
buffer. Since scnprintf() adds a 17-character prefix before appending the
decode string, won't the output still be truncated if the decode string
utilizes its full capacity?

>  	if (maxlen < len)
>  		maxlen = len;

[ ... ]

> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index 4fd48fd200552..ff08f1fee0e33 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -604,6 +604,72 @@ int perf_mem__blk_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
>  	return l;
>  }
>  
> +static int perf_mem__region_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
> +{
> +	size_t l = 0;
> +	u64 mem = PERF_MEM_REGION_NA;
> +
> +	sz -= 1; /* -1 for null termination */
> +	out[0] = '\0';

[Severity: High]
This isn't a bug introduced by this patch, but could this write out of
bounds if sz is 0?

In perf_script__meminfo_scnprintf(), earlier calls like
perf_mem__lvl_scnprintf() can write past the sz limit because they use
strcat(out, " or ") without checking the boundary, and they return lengths
larger than the remaining size.

This allows the accumulated index i to exceed sz. When
perf_mem__region_scnprintf() is eventually called, sz - i can underflow
or evaluate to 0.

If sz is 0, sz -= 1 will underflow, and out[0] = '\0' will write a null
byte out of bounds relative to the 200-byte stack buffer. Can a crafted
perf.data file trigger a stack overflow this way?

> +
> +	if (mem_info)
> +		mem = mem_info__const_data_src(mem_info)->mem_region;
> +
> +	switch (mem) {

[ ... ]

> @@ -620,6 +686,8 @@ int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *
>  	i += perf_mem__lck_scnprintf(out + i, sz - i, mem_info);
>  	i += scnprintf(out + i, sz - i, "|BLK ");
>  	i += perf_mem__blk_scnprintf(out + i, sz - i, mem_info);
> +	i += scnprintf(out + i, sz - i, "|Region ");
> +	i += perf_mem__region_scnprintf(out + i, sz - i, mem_info);
>  
>  	return i;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds read
in perf_mem__lvl_scnprintf() which is called earlier in this function.

The mem_hops string array is defined with 5 elements. However,
data_src.mem_hops is a 3-bit bitfield, meaning it can take integer values
from 0 to 7.

If a crafted perf.data file sets data_src.mem_hops to 5, 6, or 7, does this
cause an out-of-bounds pointer read when accessing
mem_hops[data_src.mem_hops]?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714004359.179451-1-thomas.falcon@intel.com?part=4