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 4 potential issue(s) to consider:

New issues:
- [High] Silent path truncation due to unchecked snprintf return value can cause data corruption.
- [Medium] The patch claims to fix get_kernel_version() by switching it to snprintf(), but the provided code changes do not modify this function.

Pre-existing issues:
- [High] Unchecked failure of strlist__new() leads to a NULL pointer dereference in strlist__has_entry().
- [High] Unchecked failure of machine__findnew_thread() leads to a NULL pointer dereference.
--

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

perf machine: Use snprintf() for guestmount path construction

This commit updates machines__findnew() and
machines__create_guest_kernel_maps() to use snprintf() instead of sprintf()
for constructing guestmount paths. This change prevents stack buffer
overflows caused by unchecked user configuration strings.

>     machines__findnew(), machines__create_guest_kernel_maps(), and
>     get_kernel_version() use sprintf() to build paths by prepending
>     symbol_conf.guestmount or root_dir.  All write into PATH_MAX stack

[Severity: Medium]
Is the commit message description for get_kernel_version() accurate?

The commit message states that get_kernel_version() uses sprintf() and is
being switched to snprintf() to prevent overflow. However, the provided code
changes do not modify this function. It appears get_kernel_version() already
uses snprintf() in the baseline.

[ ... ]

> 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: High]
Could this unchecked snprintf() lead to silent data corruption?

If symbol_conf.guestmount is long enough, snprintf() could silently truncate
the trailing digits of the guest PID. If the resulting shorter PID 
coincidentally exists (e.g., guestmount/12 instead of guestmount/1234), 
access() will falsely succeed. This would assign the wrong root_dir to the 
guest, silently corrupting profiling data by attributing maps to the wrong 
guest.

>  		if (access(path, R_OK)) {
>  			static struct strlist *seen;

[Severity: High]
This is a pre-existing issue, but does this code handle a failed allocation
correctly?

If access() fails, a static seen list is initialized:

tools/perf/util/machine.c:machines__findnew() {
    ...
    if (!seen)
        seen = strlist__new(NULL, NULL);

    if (!strlist__has_entry(seen, path)) {
    ...
}

If strlist__new() fails due to out-of-memory, it returns a null pointer. The
immediate subsequent check !strlist__has_entry(seen, path) unconditionally
passes this null pointer, which causes an immediate segmentation fault when
accessing underlying structures.

[ ... ]

> @@ -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: High]
Could an unchecked snprintf() here also result in a falsely successful
access() check?

Similar to the issue above, if the path is truncated, it might inadvertently
match an existing shorter path instead of properly failing the access() test.

[ ... ]

While reviewing this file, I noticed another pre-existing issue unrelated to
the patch diff:

[Severity: High]
This isn't a bug introduced by this patch, but can machine__findnew_thread()
return a null pointer here?

tools/perf/util/machine.c:machine__process_fork_event() {
    struct thread *parent = machine__findnew_thread(machine,
                                                    event->fork.ppid,
                                                    event->fork.ptid);
    ...
    if (thread__pid(parent) != (pid_t)event->fork.ppid) {
        ...
}

If memory allocation fails within thread creation, machine__findnew_thread()
returns a null pointer. Unconditionally evaluating thread__pid(parent) accesses
fields of the null object, which results in a segmentation fault.

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