Re: [PATCH v8 01/17] perf env: Add perf_env__e_machine helper and use in perf_env__arch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Uninitialized stack memory is leaked to the perf.data file and cached in the environment.
- [High] Lossy canonicalization of the architecture string breaks 64-bit detection for old and new perf.data files.
- [Medium] If strdup() fails during process_arch(), env->arch is silently left as NULL.
--

commit 8073d9a9766717469f9a62174c92bb53c77a0df4
Author: Ian Rogers <irogers@google.com>

perf env: Add perf_env__e_machine helper and use in perf_env__arch

This commit introduces a helper to lazily compute the e_machine and update the
perf_env's arch calculation. It updates the session to use the new helper and
modifies how the perf data file header extracts the e_machine and e_flags.

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 1e54e2c86360..4ff4caab3b32 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
[ ... ]
> +uint16_t perf_env__e_machine_nocache(struct perf_env *env, uint32_t *e_flags)
> +{
> +	uint16_t e_machine = EM_HOST;
> +
> +	if (env)
> +		e_machine = perf_arch_to_e_machine(env->arch, env->kernel_is_64_bit);
> +
> +	if (e_flags && e_machine == EM_HOST)
> +		*e_flags = EF_HOST;
> +
> +	return e_machine;
> +}
> +
> +uint16_t perf_env__e_machine(struct perf_env *env, uint32_t *e_flags)
> +{
> +	uint16_t e_machine;
> +	uint32_t local_e_flags;

Could local_e_flags be used uninitialized here?

If e_machine != EM_HOST in perf_env__e_machine_nocache(), *e_flags is not
written to, which leaves local_e_flags uninitialized.

> +
> +	if (env && env->e_machine != EM_NONE) {
> +		if (e_flags)
> +			*e_flags = env->e_flags;
> +
> +		return env->e_machine;
> +	}
> +	e_machine = perf_env__e_machine_nocache(env, &local_e_flags);
> +	if (env) {
> +		env->e_machine = e_machine;
> +		env->e_flags = local_e_flags;

If local_e_flags is uninitialized, does this leak stack memory into the
environment?

> +	}
> +	if (e_flags)
> +		*e_flags = local_e_flags;
> +
> +	return e_machine;
>  }
[ ... ]
>  const char *perf_env__arch(struct perf_env *env)
>  {
[ ... ]
> +	/*
> +	 * Lazily compute/allocate arch. The e_machine may have been
> +	 * read from a data file and so may not be EM_HOST.
> +	 */
> +	e_machine = perf_env__e_machine(env, /*e_flags=*/NULL);
> +	arch = e_machine_to_perf_arch(e_machine);
> +	env->arch = strdup(arch);

If strdup() fails due to memory exhaustion, could env->arch be left as NULL?

> +	/*
> +	 * Avoid potential crashes on the arch string if memory allocation in
> +	 * strdup fails and NULL were to be returned.
> +	 */
> +	return env->arch ?: arch;
>  }
>  
>  #if defined(HAVE_LIBTRACEEVENT)
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index f30e48eb3fc3..8d5152bde25d 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
[ ... ]
> -static int write_e_machine(struct feat_fd *ff,
> -			   struct evlist *evlist __maybe_unused)
> +static int write_e_machine(struct feat_fd *ff, struct evlist *evlist)
>  {
>  	/* e_machine expanded from 16 to 32-bits for alignment. */
>  	uint32_t e_flags;

Could e_flags be leaked to the perf.data file here?

If no threads supply a valid ELF machine and the fallback logic invokes
perf_env__e_machine_nocache() returning e_machine != EM_HOST, e_flags
may remain uninitialized stack garbage.

> -	uint32_t e_machine = EM_HOST;
> +	uint32_t e_machine = perf_session__e_machine(evlist->session, &e_flags);
>  	int ret;
>  
>  	ret = do_write(ff, &e_machine, sizeof(e_machine));
>  	if (ret)
>  		return ret;
>  
> -	return do_write(ff, &e_machine, sizeof(e_machine));
> +	return do_write(ff, &e_flags, sizeof(e_flags));

Does this write the potentially uninitialized stack memory to the file header?

>  }
[ ... ]
> +static int process_arch(struct feat_fd *ff, void *data __maybe_unused)
> +{
> +	uint16_t saved_e_machine = ff->ph->env.e_machine;
> +
> +	free(ff->ph->env.arch);
> +	ff->ph->env.arch = do_read_string(ff);
> +	if (!ff->ph->env.arch)
> +		return -ENOMEM;
> +	/*
> +	 * Make the arch string canonical by computing the e_machine from it,
> +	 * then turning the e_machine back into an arch string.
> +	 */
> +	ff->ph->env.e_machine = EM_NONE;
> +	if (perf_env__e_machine(&ff->ph->env, /*e_flags=*/NULL) != EM_NONE) {
> +		zfree(&ff->ph->env.arch);
> +		perf_env__arch(&ff->ph->env);

If strdup() failed inside perf_env__arch(), could ff->ph->env.arch be left
as NULL, while this function incorrectly returns 0 (success) leading to
later NULL pointer dereferences?

Additionally, does this canonicalization strip architecture suffixes like "64"?

For legacy perf.data files lacking the HEADER_E_MACHINE feature,
saved_e_machine would be restored to EM_NONE. If the string is canonicalized
from "x86_64" to "x86" (based on e_machine_to_perf_arch()), subsequent calls to
perf_env__init_kernel_mode() will check str_ends_with(env->arch, "64"). Because
it's now "x86", the kernel_is_64_bit flag will be set to 0.

Does this break 64-bit detection and parsing for 64-bit recordings?

> +	}
> +	ff->ph->env.e_machine = saved_e_machine;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260502065935.291960-1-irogers@google.com?part=1