Re: [PATCH v1 07/12] perf tests: Fix flakiness in trace record and replay test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Deleting a safely created `mktemp` file before reuse creates a Time-Of-Check to Time-Of-Use (TOCTOU) race condition, allowing arbitrary file overwrites.
- [High] The use of a predictable logfile path in `/tmp` combined with unsafe shell redirection allows local attackers to overwrite arbitrary files.
--

commit 9a64d257cf386372ecbfc443356890a8a5a33cd2
Author: Ian Rogers <irogers@google.com>

perf tests: Fix flakiness in trace record and replay test

The perf trace record and replay test fails intermittently on slow or
virtualized hosts because the default recording workload occasionally
completes without scheduling the target nanosleep system calls. Generalize
perf_record_with_retry to support a custom record command prefix via
PERF_RECORD_CMD and retry with scaled sleep durations.

Fixes: 15bcfb96d0dd ("perf test: Add trace record and replay test")

> diff --git a/tools/perf/tests/shell/lib/perf_record.sh b/tools/perf/tests/shell/lib/perf_record.sh
> index c5de252447705..5c7feb294ebbf 100644
> --- a/tools/perf/tests/shell/lib/perf_record.sh
> +++ b/tools/perf/tests/shell/lib/perf_record.sh
> @@ -21,9 +21,14 @@ perf_record_with_retry() {
>    local duration
>    local first_run=true
>    local ret=1
> +  local cmd_prefix="perf record"
> +  if [ -n "${PERF_RECORD_CMD}" ]; then
> +    cmd_prefix="${PERF_RECORD_CMD}"
> +  fi
> +
>    for duration in 0.01 0.1 0.3 1.0 2.0; do
>      rm -f "${perfdata}" "${perfdata}".old

[Severity: High]
Does this rm -f introduce a time-of-check to time-of-use race condition?

In trace_record_replay.sh, the perfdata file is securely created using
mktemp:

tools/perf/tests/shell/trace_record_replay.sh:
file=$(mktemp /tmp/temporary_file.XXXXX)
...
PERF_RECORD_CMD="perf trace record" perf_record_with_retry "${file}" ...

By explicitly deleting this securely created file here in
perf_record_with_retry before executing the command, the script relinquishes
the uniqueness and security guarantees of mktemp.

Since the test enforces running as root, a local attacker monitoring /tmp
could create a symlink with the deleted file's name pointing to an arbitrary
critical system file.

When the perf command later calls open_file_write() in tools/perf/util/data.c
using O_TRUNC and without O_NOFOLLOW, it will follow the symlink and truncate
the attacker's target file.

> -    perf record "$@" -o "${perfdata}" ${testprog_base} ${duration} > "$logfile" 2>&1
> +    ${cmd_prefix} "$@" -o "${perfdata}" ${testprog_base} ${duration} > "$logfile" 2>&1

[Severity: High]
Could this shell redirection allow for arbitrary file overwrites via symlink
attacks?

This issue was introduced in an earlier patch in this series when
perf_record_with_retry was added. The logfile path is predictable:

tools/perf/tests/shell/lib/perf_record.sh:perf_record_with_retry() {
    logfile="/tmp/__perf_record_retry.$(id -u).$BASHPID.log"
}

Since the process ID space is limited, a local attacker could pre-create
symlinks for likely process IDs pointing to sensitive system files.

When the script executes and the shell performs redirection without
noclobber enabled, it will blindly follow the symlink as root and
overwrite the target file.

>      local record_exit=$?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616012744.4049193-1-irogers@google.com?part=7