Re: [PATCH perf-tools-next v8 3/6] perf symbol: Introduce vmlinux_path__find() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf symbol: heap overflow in vmlinux_path__init() after vmlinux_path__find()
--

commit 075bbaee1dd0bb499313531ce795ad3e9b4f107b
Author: Aaron Tomlin <atomlin@atomlin.com>

perf symbol: Introduce vmlinux_path__find() helper

This commit introduces vmlinux_path__find() to search through the configured
vmlinux_path entries (or symbol_conf.vmlinux_name if set) and return the path
to the first readable vmlinux file.

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index b61a574ce9a30..a0fc430be13ae 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -2372,6 +2372,36 @@ static int vmlinux_path__init(struct perf_env *env)
>  	return -1;
>  }
>  
> +/**
> + * vmlinux_path__find - Locate the first accessible vmlinux file
> + *
> + * Check symbol_conf.vmlinux_name first if set, then search through the
> + * initialized paths in vmlinux_path.
> + *
> + * Return: A pointer to the path of the first readable vmlinux file, or NULL if
> + * none is accessible.
> + */
> +const char *vmlinux_path__find(void)
> +{
> +	int i;
> +
> +	if (symbol_conf.vmlinux_name && access(symbol_conf.vmlinux_name, R_OK) == 0)
> +		return symbol_conf.vmlinux_name;
> +
> +	if (!vmlinux_path) {
> +		symbol_conf.try_vmlinux_path = true;
> +		if (vmlinux_path__init(NULL) < 0)
> +			return NULL;
> +	}

[Severity: Medium]
Can calling vmlinux_path__find() before symbol__init() cause
vmlinux_path__init() to overflow vmlinux_path[] and leak memory?

When vmlinux_path__find() is called prior to symbol__init(), vmlinux_path is
NULL and symbol_conf.initialized is false. vmlinux_path__find() sets
symbol_conf.try_vmlinux_path = true and calls vmlinux_path__init(NULL), which
allocates the 7-entry vmlinux_path array and populates it via
vmlinux_path__add(), advancing vmlinux_path__nr_entries to 7 (or 2 if
symbol_conf.symfs[0] != 0):

tools/perf/util/symbol.c:vmlinux_path__add() {
	vmlinux_path[vmlinux_path__nr_entries] = strdup(new_entry);
	if (vmlinux_path[vmlinux_path__nr_entries] == NULL)
		return -1;
	++vmlinux_path__nr_entries;

	return 0;
}

When symbol__init() is subsequently called, symbol_conf.initialized is still
false and symbol_conf.try_vmlinux_path is true, so symbol__init() invokes
vmlinux_path__init(env) a second time:

tools/perf/util/symbol.c:symbol__init() {
	...
	if (symbol_conf.try_vmlinux_path && vmlinux_path__init(env) < 0)
		return -1;
	...
}

Because vmlinux_path__init() neither frees an existing vmlinux_path nor
resets vmlinux_path__nr_entries to 0:

tools/perf/util/symbol.c:vmlinux_path__init() {
	...
	vmlinux_path = malloc(sizeof(char *) * (ARRAY_SIZE(vmlinux_paths) +
			      ARRAY_SIZE(vmlinux_paths_upd)));
	if (vmlinux_path == NULL)
		return -1;

	for (i = 0; i < ARRAY_SIZE(vmlinux_paths); i++)
		if (vmlinux_path__add(vmlinux_paths[i]) < 0)
			goto out_fail;
	...
}

Doesn't the second call overwrite vmlinux_path with a new 7-entry buffer
(leaking the initial array and its strdup() strings) and then write the new
strdup() pointers starting at vmlinux_path[7] (or vmlinux_path[2]),
overflowing the heap buffer while leaving vmlinux_path[0..6] uninitialized
for dso__load_vmlinux_path() and vmlinux_path__exit()?

Additionally, if a caller invokes vmlinux_path__find() and skips
symbol__init(), symbol_conf.initialized remains false, so symbol__exit() will
return early without calling vmlinux_path__exit() to free vmlinux_path:

tools/perf/util/symbol.c:symbol__exit() {
	if (!symbol_conf.initialized)
		return;
	...
	vmlinux_path__exit();
	...
}

> +
> +	for (i = 0; i < vmlinux_path__nr_entries; ++i) {
> +		if (access(vmlinux_path[i], R_OK) == 0)
> +			return vmlinux_path[i];
> +	}
> +
> +	return NULL;
> +}

[ ... ]

>
> -- 
> 2.51.2
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018023753.584873-1-atomlin@atomlin.com?part=3