Re: [PATCH v6 5/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:
- [High] Integer underflow leading to out-of-bounds null byte write in `perf_mem__region_scnprintf`.
- [Low] Unnecessary full header inclusion of `session.h` violates subsystem guidelines.

Pre-existing issues:
- [High] Buffer overflow due to unbounded `strcat` in memory reporting functions.
--

commit 81377b3a6f0865259ee97043de4011f0758a4918
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf tools: Show memory region in perf-script subcommand

This commit adds the ability to display the memory region in 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/util/mem-events.h b/tools/perf/util/mem-events.h
> index daa22748f9fea..4ebb8109fc3c4 100644
> --- a/tools/perf/util/mem-events.h
> +++ b/tools/perf/util/mem-events.h
> @@ -4,6 +4,7 @@
>  
>  #include <stdbool.h>
>  #include <linux/types.h>
> +#include "session.h"

[Severity: Low]
Could this header inclusion be replaced with a forward declaration of
struct perf_session instead?

Including the full session.h here might cause unnecessary namespace pollution
and increased build dependencies. The subsystem guidelines for posix libc and
musl compatibility suggest preferring forward declarations when only pointer
handles are used in headers.

[ ... ]

> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index 4fd48fd200552..8ce4996cad8d3 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -604,8 +604,77 @@ int perf_mem__blk_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
>  	return l;
>  }
>  
> -int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
> +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]
Is there a possibility of an integer underflow here if sz is evaluated
as 0?

If the previous formatting functions perfectly exhaust the 200-byte buffer
and return exactly 200, sz would be 0 when this function is called. The
subtraction sz -= 1 would wrap to a maximum size_t value, leading to an
out-of-bounds null byte write at out[0] and allowing the subsequent
scnprintf calls to overwrite adjacent memory.

[ ... ]

> +int perf_script__meminfo_scnprintf(char *out, size_t sz,
> +				   const struct mem_info *mem_info,
> +				   struct perf_session *session)
> +{
> +	struct perf_env *env;
>  	int i = 0;
>  
>  	i += scnprintf(out, sz, "|OP ");
> @@ -620,6 +689,21 @@ 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);

[Severity: High]
This is a pre-existing issue, but do the perf_mem__lvl_scnprintf,
perf_mem__snp_scnprintf, and perf_mem__tlb_scnprintf functions called here
have a buffer overflow regression?

Looking at their implementations, they use unbounded string concatenations:

strcat(out, " or ");
l += 4;

Since strcat ignores the remaining buffer size sz and simply writes until
it appends a null byte, could a crafted perf.data with multiple bits set in
mem_lvl, mem_snoop, or mem_tlb write well past the end of the array if the
available space is smaller than the appended string?

Furthermore, since l += 4 artificially inflates the returned length beyond
sz, could this cause subsequent calls in this sequence to receive wrapped
negative sizes for sz - i?

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