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 1 potential issue(s) to consider:
- [Medium] Silent path truncation leading to incorrect guest mappings due to unchecked snprintf return value.
--

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

perf machine: Use snprintf() for guestmount path construction

This commit replaces sprintf() with snprintf() in guestmount path
construction to prevent stack buffer overflows when using unchecked user
configuration.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -333,7 +333,7 @@ struct machine *machines__findnew(struct machines *machines, pid_t pid)
>  	if ((pid != HOST_KERNEL_ID) &&
>  	    (pid != DEFAULT_GUEST_KERNEL_ID) &&
>  	    (symbol_conf.guestmount)) {
> -		sprintf(path, "%s/%d", symbol_conf.guestmount, pid);
> +		snprintf(path, sizeof(path), "%s/%d", symbol_conf.guestmount, pid);

[Severity: Medium]
Is it possible for truncation here in machines__findnew() to accidentally
match another valid guest directory?

The commit message states that access() will fail on a truncated path, but if
the path with a PID like "12345" is truncated at "12", and a guest directory
"12" happens to exist, access() would incorrectly succeed. Could this cause
the tool to silently store the wrong root_dir and analyze the wrong guest's
memory?

>  		if (access(path, R_OK)) {

[ ... ]

> @@ -1260,9 +1260,9 @@ int machines__create_guest_kernel_maps(struct machines *machines)
>  					 namelist[i]->d_name);
>  				continue;
>  			}
> -			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);

[Severity: Medium]
Does this snprintf() call in machines__create_guest_kernel_maps() have the
same silent truncation risk, where a truncated path could accidentally match
a different valid directory?

>  			ret = access(path, R_OK);
>  			if (ret) {
>  				pr_debug("Can't access file %s\n", path);

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