Re: [PATCH v1 04/12] perf tests: Add robust record retry helper and use subsecond workloads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Predictable temporary file path in a world-writable directory allows local privilege escalation via symlink attack.
--

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

perf tests: Add robust record retry helper and use subsecond workloads

This commit introduces a retry helper in tests/shell/lib/perf_record.sh to
prevent transient failures during perf record tests. It updates several
existing tests to use this helper and scales durations up if initial runs
fail.

> diff --git a/tools/perf/tests/shell/lib/perf_record.sh b/tools/perf/tests/shell/lib/perf_record.sh
> new file mode 100644
> --- /dev/null
> +++ b/tools/perf/tests/shell/lib/perf_record.sh
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +perf_record_with_retry() {
[ ... ]
> +  local logfile
> +  logfile="/tmp/__perf_record_retry.$(id -u).$BASHPID.log"

[Severity: High]
Could this predictable filename in a world-writable directory allow a symlink
attack?

Because BASHPID is easily guessable, a local attacker could pre-create a
symlink with this name.

[ ... ]
> +  local duration
> +  local first_run=true
> +  local ret=1
> +  for duration in 0.01 0.1 0.3 1.0 2.0; do
> +    rm -f "${perfdata}" "${perfdata}".old
> +    perf record "$@" -o "${perfdata}" ${testprog_base} ${duration} > "$logfile" 2>&1

Since these tests are often run as root to access performance counters,
could this shell redirection follow the attacker's symlink and overwrite
arbitrary system files?

Using mktemp instead would safely generate and create this temporary file.

[ ... ]
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
[ ... ]
> +test_uid() {
>    echo "Uid sampling test"
> -  if ! perf record -aB --synth=no --uid "$(id -u)" -o "${perfdata}" ${testprog} \
> -    > "${script_output}" 2>&1
> -  then
> -    if grep -q "libbpf.*EPERM" "${script_output}"
> +  local logfile
> +  logfile="/tmp/__perf_record_retry.$(id -u).$BASHPID.log"

[Severity: High]
Does this duplicate the predictable path logic from perf_record_with_retry()?

If the shared helper is updated to use mktemp, this hardcoded path check
in test_uid() will also need to be updated to match the securely generated
filename.

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