Re: [PATCH v8 01/43] selftests/dyndbg: Add kselftest script to verify dynamic-debug
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-05 18:22:29
Also in:
dri-devel
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] Time-of-Check to Time-of-Use (TOCTOU) vulnerability due to deleting and appending to securely created temporary files. - [Medium] The test script swallows the actual command error output and prints an undefined variable. - [Medium] Cryptographic hash verification of core kernel files makes the selftest overly brittle. - [Medium] The silent soft-pass mode (`K=2`) does not exit successfully as documented, falling through to a failure. - [Medium] The `LACK_DD_BUILTIN` filter in `GOLDEN_RECORDS` is broken because it queries a non-existent 4th column. - [Low] The strict-mode environment variable `K` is used in arithmetic evaluations without sanitization. - [Low] The `ifrmmod` function is identically defined twice in the script. -- commit 21ef8019906e22cad30ff587de97850e4985eee2 Author: Jim Cromie [off-list ref] selftests/dyndbg: Add kselftest script to verify dynamic-debug Adds a kselftest script to attempt full validation of dynamic-debug behavior. The script tests query grammar as documented, responses to bad input, and proper/expected effects on both the control-file display of current state, and on pr_debug logging behavior using a hash-based verification mechanism.
quoted hunk ↗ jump to hunk
diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh new file mode 100755 index 0000000000000..67b568730acc6 --- /dev/null +++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh@@ -0,0 +1,770 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0-only + +# Standard kselftest exit codes +ksft_pass=0 +ksft_fail=1 +ksft_skip=4 + +ESC=$'\033' +RED="${ESC}[0;31m" +GREEN="${ESC}[0;32m" +YELLOW="${ESC}[0;33m" +BLUE="${ESC}[0;34m" +MAGENTA="${ESC}[0;35m" +CYAN="${ESC}[0;36m" +NC="${ESC}[0;0m" +# Environment Controls: +# V=0,1,2 : Verbosity (0=concise summary, 1=verified assertions, 2=full captured outputs) +# K=0 : Strict mode (fails with exit 1 on checksum drift or stale records) +# K=1 : Soft-pass mode (prints DRIFT/STALE diffs, exits 0 with 'fake success') +# K=2 : Silent soft-pass mode (suppresses DRIFT/STALE diffs, exits 0 with 'fake success') +V=${V:=0} +K=${K:=0} + +# Sanitize V to ensure it is a valid integer +if [[ ! "$V" =~ ^[0-9]+$ ]]; then
[Severity: Low] Is there a reason the K variable is not sanitized here similarly to V? If a non-numeric string is provided for K, bash will throw an "integer expression expected" error when it evaluates K in arithmetic conditions later in the script. [ ... ]
quoted hunk ↗ jump to hunk
+function ifrmmod { + [ "${LACK_TMOD:-0}" -eq 1 ] && return + grep -q "^$1 " /proc/modules 2>/dev/null && rmmod $1 +} + +# Clean up any leftover loaded test modules at initialization +ifrmmod test_dynamic_debug_submod +ifrmmod test_dynamic_debug
[ ... ]
quoted hunk ↗ jump to hunk
+function ifrmmod { + [ "${LACK_TMOD:-0}" -eq 1 ] && return + grep -q "^$1 " /proc/modules 2>/dev/null && rmmod $1 +}
[Severity: Low] Does this function need to be re-declared here? It appears to be an exact duplicate of the ifrmmod definition from earlier in the script. [ ... ]
quoted hunk ↗ jump to hunk
+function ddcmd () { + # ddcmd <query_args> [range_pattern] [pass|fail|log] + local args="$1" + local range="$2" + local action="${3:-pass}" + local exp_exit=0 + + [ "$action" = "fail" ] && exp_exit=1 + log_ddcmd "$args" + + # Update cumulative state-machine lineage + if [[ "$args" == *"=_"* ]]; then + CUMULATIVE_DDCMDS="$args" + else + CUMULATIVE_DDCMDS="${CUMULATIVE_DDCMDS}; $args" + fi + + [ "$action" != "pass" ] && log_start + [ -n "$range" ] && capture_before "$range" + + output=$( (echo "$args" > /proc/dynamic_debug/control) 2>&1 ) + handle_exit_code $BASH_LINENO $FUNCNAME $? $exp_exit
[ ... ]
quoted hunk ↗ jump to hunk
+function handle_exit_code() { + local exp_exit_code=0 + [ $# == 4 ] && exp_exit_code=$4 + if [ "$3" -ne $exp_exit_code ]; then + echo -e "${RED}: $BASH_SOURCE:$1 $2() " \ + "expected to exit with code $exp_exit_code, got $3${NC}" + [ "$3" == 1 ] && echo "Error: '$error_msg'"
[Severity: Medium] Is error_msg defined anywhere? It seems ddcmd captures the command output and errors into the "output" variable, but handle_exit_code tries to print an undeclared error_msg variable. This causes the script to print "Error: ''", swallowing the actual error context needed for debugging test failures. [ ... ]
quoted hunk ↗ jump to hunk
+function FT_basic_queries { + v_echo "${GREEN}# BASIC_TESTS ${NC}" + if [ $LACK_DD_BUILTIN -eq 1 ]; then + echo "SKIP - test requires params, which is a builtin module" + return + fi + ddcmd =_ # zero everything + + ddcmd "module params +mf" 'kernel/params.c' + ddcmd "module params +l" 'kernel/params.c'
[Severity: Medium] Does hashing the exact output string representations of pr_debug statements in core files make the test unnecessarily brittle? Any routine upstream modification to the format strings or logic in kernel/params.c will unconditionally invalidate the static hash baselines here, causing CI failures even if the dynamic debug subsystem is functioning perfectly. [ ... ]
+function GOLDEN_RECORDS {
+ cat << 'EOF' | {[ ... ]
quoted hunk ↗ jump to hunk
+#K= de950a3e60669fdd58d0a8c2867a056d FT_basic_queries.5 +#K= 2ff49f0c4d18ec99bcb1c30840fe8afc FT_basic_queries.6 +#K= 9a1b13c32a15363dcf93913308edeea5 FT_basic_queries.7 +EOF + # Read the K-recs and skip those for tests that can't run + while read -r line; do + # Filter built-in if needed + if [ "${LACK_DD_BUILTIN:-0}" -eq 1 ]; then + # Extract pattern (4th field) from #K= line + local pattern=$(echo "$line" | awk '{print $4}') + if [[ "$pattern" == *params* || "$pattern" == *main* ]]; then
[Severity: Medium] Is this correctly filtering the built-in test records? The lines mapped in GOLDEN_RECORDS only contain 3 columns (e.g., "#K= <hash> <label>"). Because there is no 4th column, the pattern variable will always be empty, causing the filter to fail and allowing built-in tests to incorrectly execute on unsupported configurations. [ ... ]
+# Clear any stale seen/unregistered/drifted hashes from previous runs +rm -f "$SEEN_HASHES_FILE" "$UNREG_HASHES_FILE" "$DRIFT_HASHES_FILE"
[Severity: High] Could removing these temporary files introduce a local privilege escalation or arbitrary file overwrite vulnerability? These files are securely created by mktemp earlier, but explicitly removing them before the test runs means that when the script appends to them later (e.g., echo "$fingerprint" >> "$SEEN_HASHES_FILE"), bash will implicitly recreate them without O_EXCL protection. An attacker monitoring the temporary directory could create a symlink with the deleted file's name before the script runs the append redirection, allowing them to overwrite arbitrary files when the test executes as root. [ ... ]
quoted hunk ↗ jump to hunk
+if [ -s "$DRIFT_HASHES_FILE" ]; then + echo -e "${RED}\n# --- Drifted Baselines ---" + cat "$DRIFT_HASHES_FILE" + echo -e "# -------------------------${NC}" + rm -f "$DRIFT_HASHES_FILE" + failed=1 +fi + +# Cleanup +rm -f "$UNREG_HASHES_FILE" "$DRIFT_HASHES_FILE" + +if [ $failed -eq 1 ]; then + [ "$K" -eq 1 ] && echo "fake success" && exit $ksft_pass + exit $ksft_fail +fi
[Severity: Medium] Does this logic correctly handle the silent soft-pass mode (K=2)? If "failed" is 1 and K is 2, the check for K=1 fails and the script falls through to exit with $ksft_fail. This causes the test to fail instead of returning success as documented for K=2. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260905-dd-cmap-part2-clean-v8-0-a4cc0674f6fd@gmail.com?part=1