Re: [PATCH 06/13] perf tools: Use snprintf() for root_dir path construction

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete Fix / Stack Buffer Overflow in `machines__findnew` and `machines__create_guest_kernel_maps`
- [High] Logic Error: Masked initialization failure in `machine__init` leads to crashes.
--

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

    perf tools: Use snprintf() for root_dir path construction
    
    get_kernel_version() in machine.c and dso__load_guest_kernel_sym() in
    symbol.c use sprintf() to construct paths by prepending root_dir to
    "/proc/version" and "/proc/kallsyms" respectively. Both write into
    PATH_MAX stack buffers, but root_dir comes from --guestmount or KVM
    configuration and is not length-checked.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index da1ad58758afd..58fa57e3d1a15 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1336,7 +1336,7 @@ static char *get_kernel_version(const char *root_dir)
>  	char *name, *tmp;
>  	const char *prefix = "Linux version ";
>  
> -	sprintf(version, "%s/proc/version", root_dir);
> +	snprintf(version, sizeof(version), "%s/proc/version", root_dir);

[Severity: High]
This isn't a bug introduced by this patch, but the fix appears to be
incomplete. While this replaces sprintf() with snprintf() for root_dir,
there are two other unpatched sprintf() calls in this file that write
the unchecked symbol_conf.guestmount into a char path[PATH_MAX] buffer:

In machines__findnew():
    sprintf(path, "%s/%d", symbol_conf.guestmount, pid);

In machines__create_guest_kernel_maps():
    sprintf(path, "%s/%s/proc/kallsyms",
            symbol_conf.guestmount,
            namelist[i]->d_name);

Could these also overflow the PATH_MAX stack buffer if a very long string
is provided to the --guestmount option?

[Severity: High]
This is a pre-existing issue, but I noticed a potential logic error in
machine__init() in this file regarding the error path. If an allocation
fails (e.g., strdup(root_dir)), the code jumps to the 'out' label:

out:
	if (err) {
		zfree(&machine->kmaps);
		zfree(&machine->root_dir);
		zfree(&machine->mmap_name);
	}
	return 0;

Does this intentionally return 0 instead of err? It appears this masks
the initialization failure from callers like machines__add(), leading
to crashes later when they use the partially uninitialized machine struct.

>  	file = fopen(version, "r");
>  	if (!file)
>  		return NULL;

[ ... ]

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