Re: [PATCH v8 01/43] selftests/dyndbg: Add kselftest script to verify dynamic-debug

[sashiko-bot@kernel.org]

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 <jim.cromie@gmail.com>

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.

> 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.

[ ... ]

> +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

[ ... ]

> +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.

[ ... ]

> +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

[ ... ]

> +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.

[ ... ]

> +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' | {

[ ... ]

> +#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.

[ ... ]

> +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