Re: [PATCH 3/3] DO NOT MERGE: selftests: perf_events: Add device memory callchain unwinding test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Insecure temporary file usage in /tmp creates a privilege escalation or file overwrite vector
- [Medium] Test silently reports a false positive on initialization or perf execution failures
- [Medium] Hardcoded 4KB page alignment breaks on arm64 systems with 16KB or 64KB pages
- [Low] Uninitialized wait status variable `pst` can lead to undefined behavior
- [Low] File descriptor for `/dev/mem` is leaked to the `perf` child process
--

commit 72f5293ddcb8999284919045e0f80f532368b0f1
Author: Fredrik Markstrom <fredrik.markstrom@est.tech>
DO NOT MERGE: selftests: perf_events: Add device memory callchain unwinding test

This commit adds a regression test for device memory callchain unwinding by
mapping an MMIO address via /dev/mem and sampling the process with perf.

diff --git a/tools/testing/selftests/perf_events/test_perf_vmio.c b/tools/testing/selftests/perf_events/test_perf_vmio.c
new file mode 100644
index 000000000000..780c5800dd6b
--- /dev/null
+++ b/tools/testing/selftests/perf_events/test_perf_vmio.c

[ ... ]

> +	env = getenv("MMIO_ADDR");
> +	if (env)
> +		pa = strtoul(env, NULL, 16);
> +
> +	page = pa & ~0xFFFUL;
> +	off = pa - page;

Is it possible for this to break on arm64 systems configured with 16KB or
64KB pages? The bitwise mask assumes a 4KB page size. Could this be changed
to use sysconf(_SC_PAGESIZE) instead?

> +
> +	fd = open("/dev/mem", O_RDWR | O_SYNC);

Does this leak the file descriptor to the perf child process? It is opened
without O_CLOEXEC, so the fd remains open across the execvp() call for perf.

> +	if (fd < 0)
> +		SKIP(return, "cannot open /dev/mem");
> +
> +	/* Fork a spinner child with FP pointing into device memory */
> +	spin_pid = fork();
> +	ASSERT_GE(spin_pid, 0);
> +	if (spin_pid == 0) {
> +		/*
> +		 * mmap /dev/mem in the child so remap_pfn_range populates
> +		 * PTEs directly. fork() does not copy PTEs for VM_PFNMAP
> +		 * regions, so mapping before fork leaves the child with
> +		 * empty page tables — the unwinder would get a translation
> +		 * fault instead of a synchronous external abort.
> +		 */
> +		m = mmap(NULL, off + 4096, PROT_READ | PROT_WRITE,
> +			 MAP_SHARED, fd, page);
> +		if (m == MAP_FAILED)
> +			_exit(1);

If the child process fails to mmap /dev/mem here (for instance, due to
CONFIG_STRICT_DEVMEM or an unaligned offset), it immediately exits. 

Because the parent does not verify the child's status before running perf,
perf will attempt to attach to a dead process and fail with a status other
than 1. The test then incorrectly passes. Can we verify the child process
is alive before running perf, or check its exit status?

[ ... ]

> +	if (perf_pid == 0) {
> +		char *const perf_argv[] = {
> +			"perf", "record", "-g", "--call-graph", "fp",
> +			"-p", pid_str, "--", "sleep", "3", NULL
> +		};
> +
> +		if (chdir("/tmp"))
> +			_exit(1);
> +		execvp(perf_argv[0], perf_argv);

Could this overwrite unintended files by running a root-privileged perf
record in the world-writable /tmp directory without specifying an output
file?

perf record defaults to writing to perf.data in the current directory.
An unprivileged user could pre-create a symlink at /tmp/perf.data pointing
to a sensitive file.

Could the test use mkdtemp() to create a secure directory, or pipe the
output instead?

> +		_exit(1);
> +	}
> +
> +	waitpid(perf_pid, &pst, 0);

Could pst be left uninitialized if waitpid() fails and returns an error
(such as -1 from being interrupted by a signal)? 

This would cause the subsequent WIFEXITED(pst) and WEXITSTATUS(pst) macros
to read uninitialized memory. Can we initialize pst to 0 or check the
waitpid() return value?

> +
> +	kill(spin_pid, SIGKILL);
> +	waitpid(spin_pid, NULL, 0);
> +	close(fd);
> +
> +	if (WIFEXITED(pst) && WEXITSTATUS(pst) == 1)
> +		SKIP(return, "perf not available");
> +
> +	/*
> +	 * The real test is that the kernel survived. If we got here
> +	 * without a synchronous external abort, the guard worked.
> +	 */
> +	TH_LOG("kernel survived perf sampling with FP in device memory");
> +#endif /* __aarch64__ */
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428-master-with-pfix-v3-v1-0-c384d3e53092@est.tech?part=3