Re: [PATCH 1/4] perf config: Move perf_config__set_variable() to util/config.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf: data race in bad_config() causes stack UAF
--

commit 861fbcb5738f68f4bf3fa7cf60e666d3e7639e45
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf config: Move perf_config__set_variable() to util/config.c

This commit moves perf_config__set_variable() out of the builtin so opt-in
features can persist their choice from outside it. It also serializes
parsing and rewriting with a mutex since config file access can now run on
more than one thread.

> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 8fe43b032e9af..da4213a704899 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
[ ... ]
> @@ -926,6 +926,84 @@ void perf_config__exit(void)
>  	config_set = NULL;
>  }
>  
> +int perf_config_set__write(struct perf_config_set *set,
> +			   const char *file_name, bool system_config)
[ ... ]
> +int perf_config__set_variable(const char *var, const char *value)
> +{
> +	char path[PATH_MAX];
> +	char *user_config = mkpath(path, sizeof(path), "%s/.perfconfig", getenv("HOME"));
> +	const char *config_filename = config_exclusive_filename ?: user_config;
> +		/*
> +	 * When rewriting the system wide file all entries are marked as coming
> +	 * from it and must be kept, or it would be truncated down to its
> +	 * header.
> +	 */
> +	bool system_config = strcmp(config_filename, perf_etc_perfconfig()) == 0;
> +	struct perf_config_set *set;
> +	int ret = -1;
> +
> +	pthread_mutex_lock(&config_update_mutex);
> +	set = perf_config_set__new();
> +	if (!set)
> +		goto out_err;
> +
> +	if (perf_config_set__collect(set, config_filename, var, value) < 0) {

[Severity: High]
Can this lead to a stack use-after-free during concurrent config parsing?

A writer thread running perf_config__set_variable() assigns the global
config_file_name to point to its local stack buffer (path) under config_mutex
in perf_config_set__collect().

Concurrently, a reader thread iterating cached configurations via perf_config()
might encounter a malformed value and invoke bad_config(). bad_config() reads
config_file_name locklessly:

tools/perf/util/config.c:bad_config() {
    ...
    if (config_file_name)
        pr_warning("bad config value for '%s' in %s, ignoring...\n",
                   name, config_file_name);
    ...
}

If the reader reads the pointer locklessly, and the writer finishes and
returns before the reader executes pr_warning(), won't the reader dereference
the destroyed stack frame for path?

> +		pr_err("Failed to add '%s=%s'\n", var, value);
> +		goto out_err;
> +	}
> +
> +	if (perf_config_set__write(set, config_filename, system_config) < 0) {
> +		pr_err("Failed to set the configs on %s\n", config_filename);
> +		goto out_err;
> +	}
> +
> +	ret = 0;
> +out_err:
> +	perf_config_set__delete(set);
> +	pthread_mutex_unlock(&config_update_mutex);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928220634.2451784-1-acme@kernel.org?part=1