Re: [PATCH v10 8/9] perf scripts: Add perf-stuck, to tell where a running perf is stuck

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf: PID reuse race condition (TOCTOU) allows attaching GDB to arbitrary process
- [Medium] perf: terminal escape sequence injection via unsanitized progress log
--

commit 4d621a8585d05e62b3a0241c056ece1b77cc057c
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf scripts: Add perf-stuck, to tell where a running perf is stuck

A perf that takes forever is hard to tell apart from one stuck in a
loop, and there is no way to see where without attaching gdb.
perf-stuck.sh samples a running process' /proc entries and its progress
line at a fixed interval, and with -g runs gdb when no progress is made
across two samples.

> diff --git a/tools/perf/scripts/perf-stuck.sh b/tools/perf/scripts/perf-stuck.sh
> new file mode 100755
> index 0000000000000..0bbcef6c82a4b
> --- /dev/null
> +++ b/tools/perf/scripts/perf-stuck.sh
[ ... ]
> +while :; do
> +	if [ ! -d /proc/"$pid" ]; then
> +		echo "$(date +%T) process gone"
> +		break
> +	fi

[Severity: Medium]
Could this simple directory existence check allow a race condition if the PID
terminates and gets recycled by another process?

If an attacker recycles the PID exactly during the sleep interval, gdb could be
attached to their process unwittingly, exposing the debugging session to
untrusted data when it later runs timeout below.

[ ... ]
> +	# Progress updates overwrite in place, \r separated, so the log is
> +	# one ever longer line: take the last update from a bounded tail
> +	# window and drop the \r's, so printing it doesn't reset the
> +	# terminal cursor on the state line.
> +	progress=
> +	if [ -n "$progress_log" ] && [ -s "$progress_log" ]; then
> +		progress=$(tail -c 8192 -- "$progress_log" | tr '\r\n' '\n\n' | tail -1)
> +	fi

[Severity: Medium]
Should the control characters in this string be explicitly stripped?

The cmdline variable is stripped of control characters, but progress only
replaces carriage returns and newlines.

If an untrusted log is monitored, could this allow terminal escape sequence
injection?

[ ... ]
> +	printf '%s state=%s cpu=+%d (%d.%02ds) rss=%dkB stack=%s %s %s\n' \
> +	       "$(date +%T)" "$state" "$cpu_delta" \
> +	       $(( cpu_delta / hz )) $(( (cpu_delta % hz) * 100 / hz )) \
> +	       "$rss" "$stack_txt" "$stuck_txt" "${progress:-(no progress log)}"

The unsanitized string is written directly to the terminal context here.

> +
> +	if [ -n "$use_gdb" ] && [ -z "$gdb_done" ] && [ "$stuck" -ge 2 ]; then
> +		gdb_log=$(mktemp /tmp/perf-stuck-gdb.XXXXXX)
> +		# The gdb script makes inferior calls, and a call into a perf
> +		# wedged in a loop never returns, so bound the run; SIGINT
> +		# releases the inferior instead of leaving perf stopped.
> +		timeout --signal=INT 30 gdb -p "$pid" -batch -x "$gdb_cmds" -ex bt \
> +		    -ex 'perf-die-chain-all' -ex perf-dso -ex detach > "$gdb_log" 2>&1

This is where gdb could be attached to a recycled PID.

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