Re: [PATCH v7 02/29] selftests/dyndbg: Add kselftest script to verify dynamic-debug

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The script uses predictable temporary file names in `/tmp` based on the process ID, introducing a Time-of-Check to Time-of-Use (TOCTOU) symlink vulnerability.
- [Medium] The golden record MD5 hashing of dynamic debug control diffs includes source code line numbers, strictly coupling the test to file topology and causing spurious failures on unrelated code changes.
- [Medium] The framework uses exact MD5 hashes of dmesg slices, leading to severe flakiness when concurrent kernel activity occurs.
- [Medium] The script unconditionally skips all tests on monolithic kernels due to an overly broad initial `lsmod` check.
- [Medium] The dmesg log parsing does not account for `CONFIG_PRINTK_CALLER=y`, causing MD5 hashes to change on every test run due to PID inclusion.
--

--- Patch [2]: [PATCH v7 02/29] selftests/dyndbg: Add kselftest script to verify dynamic-debug ---

commit acdd3977962a0d4f508ca6d6378e26344aaef61d
Author: Jim Cromie [off-list ref]

selftests/dyndbg: Add kselftest script to verify dynamic-debug

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

diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
index 0000000000000..1dafeaabc9768
--- /dev/null
+++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh

@@ -0,0 +1,799 @@

[ ... ]

[ -e /proc/dynamic_debug/control ] || {
    echo -e "${RED}: this test requires CONFIG_DYNAMIC_DEBUG=y ${NC}"
    exit $ksft_skip # nothing to test here, no good reason to fail.
}

lsmod >/dev/null 2>&1 || {
    echo -e "${RED}: lsmod requires /proc/modules ${NC}"
    exit $ksft_skip # maybe later we can do more
}

[Severity: Medium]
Does this early exit prevent the script from running the builtin tests
on a monolithic kernel?

Since the builtin_tests are explicitly designed to validate dynamic_debug
behavior on built-in code without requiring module support, it seems like
skipping the entire script here disables valid testing on kernels compiled
without CONFIG_MODULES=y.

[ ... ]

diff --git a/tools/testing/selftests/dynamic_debug/syslog_hash_validation.sh b/tools/testing/selftests/dynamic_debug/syslog_hash_validation.sh
index 0000000000000..c6d60495d7baa
--- /dev/null
+++ b/tools/testing/selftests/dynamic_debug/syslog_hash_validation.sh

@@ -0,0 +1,384 @@

[ ... ]

# Default APP to DYNDBG if not already set
APP="${APP:-DYNDBG}"
APP_LOWER=$(echo "$APP" | tr '[:upper:]' '[:lower:]')

# Global files for tracking seen, unregistered, and drifted hashes
SEEN_HASHES_FILE="/tmp/${APP_LOWER}_seen_hashes_$$"
UNREG_HASHES_FILE="/tmp/${APP_LOWER}_unreg_hashes_$$"
DRIFT_HASHES_FILE="/tmp/${APP_LOWER}_drift_hashes_$$"

[Severity: High]
Are these predictable file paths in the world-writable /tmp directory
vulnerable to a symlink attack?

If a malicious local user creates a symlink matching this PID pattern
right before the script writes to it, could it allow the script to
overwrite sensitive files, since it blindly appends to these paths later
on?

Using mktemp might be a safer approach here to prevent Time-of-Check
to Time-of-Use exploits.

[ ... ]

function verify_dmesg_slice {
    # Slices dmesg, computes its hash, and verifies it against the database.
    # $1 - unique test key (e.g. normal_513)
    # $2 - optional start marker (defaults to ${APP}_START_${label})
    # $3 - optional end marker (defaults to ${APP}_END_${label})

    local label="$1"
    local app="${APP:-DYNDBG}"
    local start_marker="${2:-${app}_START_${label}_$$}"
    local end_marker="${3:-${app}_END_${label}_$$}"
    local extra_args="dmesg"

    # 1. Capture the log slice (exactly once!)
    local log_slice=$(dmesg | sed -n "/$start_marker/,/$end_marker/p" | \
	grep -E -v "$start_marker|$end_marker" | \
        sed -e 's/^\[[^]]*\] //' )

[Severity: Medium]
Does this regex correctly handle kernels configured with
CONFIG_PRINTK_CALLER=y?

When printk caller info is enabled, a second bracket group containing the
thread ID is present in the dmesg output. Since the PID changes per run,
will the resulting MD5 hash constantly mismatch because the second bracket
was not stripped before hashing?

[Severity: Medium]
Can background kernel activity disrupt the MD5 hashes calculated from
this slice?

If unrelated subsystem prints (like networking, RCU, or USB) occur between
the log_start and log_stop markers, they will be captured here and alter
the exact hash. Should there be a way to filter the dmesg slice to only
include dyndbg-related lines to prevent test flakiness in noisy
environments?

[ ... ]

function verify_after_change {
    # Verifies the transition between the stored 'before' state and the current state
    # $1 - optional unique test key (resolved via stack if empty)

[ ... ]

    # 1. Capture the 'after' state (exactly once!)
    local after_slice=$(slice_by_grep "$BEFORE_CAPTURE_PATTERN" "$BEFORE_CAPTURE_FILE")

    # 2. Generate the unified diff, stripped of volatile diff headers AND hunk line-numbers
    local transition_diff=$(diff -u <(echo "$BEFORE_CAPTURE_SLICE") <(echo "$after_slice") | \
        tail -n +3 | \
        sed -E 's/^@@ -[0-9]+.* \+[0-9]+.* @@/@@/g')

    # 3. Compute its fingerprint
    local fingerprint=$(echo "$transition_diff" | tr -d '\r' | md5sum | cut -d' ' -f1)

[Severity: Medium]
Does the generated transition_diff still include source code line numbers
from the control file?

The sed command strips the chunk headers, but the body text from
/proc/dynamic_debug/control inherently outputs the format filename:lineno.
If unrelated upstream commits add or remove lines in tested files like
kernel/params.c, won't this cause the tests to spuriously fail because
the hashed line numbers drifted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-dd-maint-2-v7-0-010fbe73b311@gmail.com?part=2