Re: [PATCH 2/9] perf machine: Use snprintf() for guestmount 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] Aborting processing of all remaining guest maps when a single directory lacks proc/kallsyms.
- [High] Uninitialized errno checked against ERANGE after strtol() causes valid directory names to be erroneously skipped.
--

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

perf machine: Use snprintf() for guestmount path construction

This commit prevents a potential stack buffer overflow by replacing
sprintf() with snprintf() when building paths using the user-provided
guestmount configuration.

Fixes: a1645ce12adb6c9c ("perf: 'perf kvm' tool for monitoring guest performance from host")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Zhang, Yanmin <yanmin_zhang@linux.intel.com>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 9329d319bd033..0d2ebf6a84bcf 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c

[ ... ]

> @@ -1260,9 +1260,9 @@ int machines__create_guest_kernel_maps(struct machines *machines)
>  					 namelist[i]->d_name);
>  				continue;
>  			}

[Severity: High]
This isn't a bug introduced by this patch, but does the strtol() parsing in
machines__create_guest_kernel_maps() handle errno correctly?

Since errno is not explicitly cleared to 0 before the strtol() call right
above this continue statement, if a previous directory name caused an
overflow, errno will remain set to ERANGE.

This would cause the (errno == ERANGE) check to evaluate to true for all
subsequent valid directory PIDs, erroneously skipping them. Could we set
errno = 0 before parsing?

> -			sprintf(path, "%s/%s/proc/kallsyms",
> -				symbol_conf.guestmount,
> -				namelist[i]->d_name);
> +			snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
> +				 symbol_conf.guestmount,
> +				 namelist[i]->d_name);
>  			ret = access(path, R_OK);
>  			if (ret) {
>  				pr_debug("Can't access file %s\n", path);

[Severity: High]
This is also a pre-existing issue, but does this access failure in
machines__create_guest_kernel_maps() intend to abort the entire
initialization loop?

If access() fails here, the code executes a goto failure. This means one
inaccessible guest directory or missing proc/kallsyms will abort the loop
and skip all subsequent valid guest map directories.

Earlier in the loop, invalid directory names use continue to safely skip
just that entry. Would it be better to continue instead of aborting the
processing of all remaining maps?

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