Re: [PATCH v1 2/2] perf test: Add inject ASLR test

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] Hardcoded and insecure PERF binary path
- [Medium] Unhandled grep pipeline aborts script via set -e
- [Medium] Architecture assumption in kernel address parsing breaks 32-bit systems
- [Medium] Improper cleanup of temporary files and directories
--

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

perf test: Add inject ASLR test

Add a new shell test inject_aslr.sh to verify the perf inject --aslr
feature.

> diff --git a/tools/perf/tests/shell/inject_aslr.sh b/tools/perf/tests/shell/inject_aslr.sh
> new file mode 100755
> index 00000000000000..17544fe9ef6c02
> --- /dev/null
> +++ b/tools/perf/tests/shell/inject_aslr.sh
> @@ -0,0 +1,381 @@
> +#!/bin/bash
> +# perf inject --aslr test
> +# SPDX-License-Identifier: GPL-2.0
> +
> +shelldir=$(dirname "$0")
> +# shellcheck source=lib/perf_has_symbol.sh
> +. "${shelldir}"/lib/perf_has_symbol.sh
> +
> +sym="noploop"
> +
> +# Set path to built perf
> +PERF="/tmp/perf3/perf"

Is this hardcoded path safe and portable? If the test is run as root, could a
local user place a malicious executable at /tmp/perf3/perf and escalate
privileges?

Also, won't this break the test in standard CI environments?

[ ... ]

> +cleanup() {
> +  # Check if temp_dir is set and looks sane before removing
> +  if [[ "${temp_dir}" =~ ^/tmp/perf-test-aslr\. ]]; then
> +    rm -rf "${temp_dir}"
> +  fi
> +  trap - EXIT TERM INT
> +}
> +
> +trap_cleanup() {
> +  echo "Unexpected signal in ${FUNCNAME[1]}"
> +  cleanup
> +  exit 1
> +}
> +trap trap_cleanup TERM INT

Because the script uses set -e, should this also trap EXIT? Without an EXIT
trap, if a command fails and triggers an exit, temp_dir might be leaked.

[ ... ]

> +test_basic_aslr() {
> +  echo "Test basic ASLR remapping"
> +  local data
> +  data=$(mktemp /tmp/perf.data.basic.XXXXXX)

Should these temporary files be created inside temp_dir instead of directly
in /tmp?

[ ... ]

> +  if [ -z "$orig_addr" ]; then
> +    echo "Basic ASLR test [Failed - no noploop samples in original file]"
> +    err=1
> +  elif [ -z "$new_addr" ]; then
> +    echo "Basic ASLR test [Failed - could not find remapped address]"
> +    err=1
> +  elif [ "$orig_addr" = "$new_addr" ]; then
> +    echo "Basic ASLR test [Failed - addresses are not remapped]"
> +    err=1
> +  else
> +    echo "Basic ASLR test [Success]"
> +    rm -f "${data}" "${data2}" "${data}.old" "${data2}.old"
> +  fi
> +}

If the test fails and err=1 is set, won't these temporary files be permanently
leaked?

This pattern appears in multiple test functions. Also, in
test_dropped_samples, if the test returns early due to unsupported
--phys-data, these files are also leaked.

[ ... ]

> +test_kernel_aslr() {
> +  echo "Test kernel ASLR remapping"
> +  local kdata
> +  kdata=$(mktemp /tmp/perf.data.kernel.XXXXXX)
> +  local kdata2
> +  kdata2=$(mktemp /tmp/perf.data2.kernel.XXXXXX)
> +  local log_file
> +  log_file=$(mktemp /tmp/kernel_record.log.XXXXXX)
> +
> +  # Try to record kernel samples
> +  if ! ${PERF} record -e task-clock:k -o "${kdata}" ${prog} > "${log_file}" 2>&1; then
> +    echo "Skipping kernel ASLR test as recording failed (maybe no permissions)"
> +    rm -f "${kdata}" "${log_file}"
> +    return
> +  fi
> +
> +  # Check for warning about kernel map restriction
> +  if grep -q "Couldn't record kernel reference relocation symbol" "${log_file}"; then
> +    echo "Skipping kernel ASLR test as kernel map could not be recorded (permissions restricted)"
> +    rm -f "${kdata}" "${log_file}"
> +    return
> +  fi
> +
> +  ${PERF} inject -v --aslr -i "${kdata}" -o "${kdata2}"
> +
> +  # Check if kernel addresses are remapped.
> +  orig_addr=$(${PERF} script -i "${kdata}" | awk '{for(i=1;i<=NF;i++) if($i ~ /^ffff/) {print $i; exit}}')
> +  new_addr=$(${PERF} script -i "${kdata2}" | awk '{for(i=1;i<=NF;i++) if($i ~ /^ffff/) {print $i; exit}}')

Will this parsing work on 32-bit architectures? Kernel space typically resides
at different offsets (such as 0xc0000000 or 0x80000000) on 32-bit platforms,
so hardcoding ^ffff might cause the test to spuriously fail there.

[ ... ]

> +test_kernel_report_aslr() {

[ ... ]

> +  ${PERF} report -i "${data_clean}" --stdio > "${report1}"
> +  ${PERF} report -i "${kdata2}" --stdio > "${report2}"
> +
> +  # Strip headers and compare lines with percentages
> +  grep '%' "${report1}" | grep -v '^#' > "${report1_clean}"
> +  grep '%' "${report2}" | grep -v '^#' > "${report2_clean}"

What happens if report1 contains no matching lines?

The second grep -v would receive empty input and exit with a status code of 1.
Since the script uses set -e and doesn't have || true on this pipeline, won't
this cause the script to abort immediately and silently?

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