Re: [PATCH v11 17/19] perf env: Add helper to lazily compute the os_release

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `perf_env__os_release` helper incorrectly identifies pipe mode as live mode due to uninitialized `env->arch`, prematurely poisoning the environment with the host's OS release.

Pre-existing issues:
- [High] Passing a NULL pointer to `snprintf` with a `%s` format specifier causes Undefined Behavior and crashes.
- [High] Destroying and locking uninitialized synchronization primitives (mutex and rwlock).
--

commit 18f3f0f76a0f321e841ef41fc691a35a33c63e31
Author: Ian Rogers <irogers@google.com>

perf env: Add helper to lazily compute the os_release

In live mode the os_release isn't being initialized, make a lazy
initialization helper that assumes when the os_release isn't
initialized this is live mode.

[ ... ]

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 4de07cc7ef5da..61b30f08493ae 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
[ ... ]
> @@ -361,6 +361,44 @@ bool perf_arch_is_big_endian(const char *arch)
>  	return false;
>  }
>  
> +const char *perf_env__os_release(struct perf_env *env)
> +{
> +	struct utsname uts;
> +	int ret;
> +	const char *release;
> +
> +	if (!env)
> +		return perf_version_string;
> +
> +	mutex_lock(&env->lock);
> +	if (env->os_release) {
> +		release = env->os_release;
> +		goto out;
> +	}
> +
> +	/*
> +	 * If env->arch is set, this is an offline target environment.
> +	 * If the os_release is not populated in the file, we do not want
> +	 * to poison it with the host's release which would break guest checks.
> +	 */
> +	if (env->arch) {
> +		release = NULL;
> +		goto out;
> +	}

[Severity: High]
Does this correctly handle pipe mode?

In pipe mode, feature events are synthesized and embedded in the event
stream, so they are not parsed during perf_session__new(). When tools like
perf report invoke symbol__init() and vmlinux_path__init() before
processing the event stream, env->arch is strictly NULL.

This causes the helper to erroneously assume live mode and permanently
populate env->os_release with the host's release, which can break guest
checks and cross-platform symbol resolution.

[Severity: High]
This is a pre-existing issue, but looking at perf_env__exit() in
tools/perf/util/env.c, is it safe to destroy and lock uninitialized
synchronization primitives?

In functions like x86__is_amd_cpu(), a stack-allocated struct perf_env is
zero-initialized and never properly set up. When perf_env__exit(&env) is
called later, it blindly calls mutex_destroy(&env->lock) and invokes
perf_env__purge_bpf(env), which performs down_write(&env->bpf_progs.lock).

Operating on completely zero-filled mutexes and rwlocks can lead to
undefined behavior and immediate aborts on strict libc implementations.

[ ... ]
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 2ce512f08a1d6..077d19af52408 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
[ ... ]
> @@ -2242,7 +2242,7 @@ static int vmlinux_path__init(struct perf_env *env)
>  		return 0;
>  
>  	if (env) {
> -		kernel_version = env->os_release;
> +		kernel_version = perf_env__os_release(env);

[Severity: High]
This isn't a bug introduced by this patch, but if
perf_env__os_release(env) returns NULL (which happens intentionally for
offline targets lacking the HEADER_OSRELEASE feature), kernel_version
becomes NULL.

Shortly after this assignment, kernel_version is unconditionally passed
into snprintf via the %s format specifier:

    snprintf(bf, sizeof(bf), vmlinux_paths_upd[i], kernel_version);

Could passing a NULL pointer to %s cause undefined behavior and potential
crashes in hardened builds or on strict libc implementations?

>  	} else {
>  		if (uname(&uts) < 0)
>  			goto out_fail;

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