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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Processing a crafted perf.data file triggers a stack buffer overflow due to integer underflow in size tracking and unbounded string operations, allowing potential arbitrary code execution.
- [Medium] The destination buffer `out` is too small to hold the formatted memory information string along with its prefix, leading to silent truncation.
--

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

perf tools: Show memory region in perf-script subcommand
    
This commit adds the memory region field to the perf-script subcommand
output. It utilizes the mem_region field introduced for Intel's OMR facility
and increases the output buffer sizes to accommodate the new region string.
    
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..e82ab5cb842e2 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[150];
> +	char out[150];
>  	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, 150, mi);
>  	mem_info__put(mi);
>  
> -	len = scnprintf(out, 100, "%16" PRIx64 " %s", data_src, decode);
> +	len = scnprintf(out, 150, "%16" PRIx64 " %s", data_src, decode);

[Severity: Medium]
This is a pre-existing issue, but I am wondering if the out buffer is large
enough here in data_src__fprintf()? After the 17-byte prefix, only 132 bytes
remain for the decode string. Valid combinations of memory bits can easily
exceed this limit, especially with the addition of the new Region field.
Could this lead to silent truncation?

>  	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 this newly added function
replicates a pre-existing pattern that seems unsafe. If
perf_mem__region_scnprintf() is called with sz equal to 0, sz -= 1 will
underflow to a large unsigned value, and out[0] = '\0' will write out of
bounds.

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

[ ... ]

> @@ -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 could this sequence of calls in
perf_script__meminfo_scnprintf() lead to a stack buffer overflow?

If any previous formatter returns a length that exceeds the available space,
i will become greater than sz. If that happens, the sz - i calculation
passed to subsequent formatters will underflow to a large unsigned value,
completely disabling bounds checking.

For example, perf_mem__lvl_scnprintf() uses strcat in some cases:

    if (printed++) {
        strcat(out, " or ");
        l += 4;
    }

This unbounded strcat can push the return length beyond the allocation,
guaranteeing i exceeds sz in the caller. Should the bounds tracking in
these functions be updated to prevent this underflow from propagating?

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