Re: [PATCH v6 9/9] selftests/ftrace: Walk up to find test.d/functions when a subdirectory is passed

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The directory traversal logic fails to check directory ownership and fails open on `stat` error, allowing arbitrary code execution as root via an attacker-controlled ancestor directory.
--

--- Patch [9]: [PATCH v6 9/9] selftests/ftrace: Walk up to find test.d/functions when a subdirectory is passed ---
commit 71305ea3fc4b8a7294d098d4bcdd8624fd21ca70
Author: Wen Yang <[email protected]>

selftests/ftrace: Walk up to find test.d/functions when a subdirectory is passed

This commit modifies ftracetest to walk up the directory tree from a passed
test directory until it finds a test.d/functions file. This allows monitor
subdirectories to be used directly as the test root without needing dummy
functions shims.

> diff --git a/tools/testing/selftests/ftrace/ftracetest b/tools/testing/selftests/ftrace/ftracetest
> index 0a56bf209f6c0..8f9d9291bf4ce 100755
> --- a/tools/testing/selftests/ftrace/ftracetest
> +++ b/tools/testing/selftests/ftrace/ftracetest

[ ... ]

> @@ -159,9 +159,29 @@ parse_opts() { # opts
>    if [ -n "$OPT_TEST_CASES" ]; then
>      TEST_CASES=$OPT_TEST_CASES
>    fi
> -  if [ -n "$OPT_TEST_DIR" -a -f "$OPT_TEST_DIR"/test.d/functions ]; then
> -    TOP_DIR=$OPT_TEST_DIR
> -    TEST_DIR=$TOP_DIR/test.d
> +  if [ -n "$OPT_TEST_DIR" ]; then
> +    # Walk up from OPT_TEST_DIR to find the nearest ancestor that contains
> +    # test.d/functions.  This allows a monitor subdirectory (e.g.
> +    # verification/test.d/tlob/) to be passed directly without placing a
> +    # dummy functions shim in each new subdirectory.
> +    # Security: only walk within directories owned by root or the current user
> +    # to prevent sourcing a functions file planted in a world-writable ancestor.
> +    dir=$(realpath "$OPT_TEST_DIR" 2>/dev/null || echo "$OPT_TEST_DIR")
> +    while [ "$dir" != "/" ] && [ "$dir" != "//" ]; do
> +      if [ -f "$dir/test.d/functions" ]; then
> +        # Verify the directory is not world-writable to prevent LPE.
> +        dirperms=$(stat -c "%a" "$dir" 2>/dev/null || stat -f "%Lp" "$dir" 2>/dev/null)
> +        case "$dirperms" in
> +          *2|*3|*6|*7) ;; # world-writable: skip this candidate
> +          *)
> +            TOP_DIR=$dir

[Severity: High]
Does this logic actually check directory ownership as mentioned in the comments?

It appears the code only checks if the directory is world-writable, but omits
validating that it is owned by root or the current user. An attacker could
potentially create a directory with standard 0755 permissions, plant a
malicious test.d/functions script, and if a privileged user or automated test
harness runs ftracetest against a subdirectory, the script would traverse up
and source the payload.

Additionally, if the stat command fails for any reason, dirperms will be empty.
Would an empty string match the *) fallback case and cause the script to silently
fail open, accepting even a world-writable directory?

> +            TEST_DIR=$TOP_DIR/test.d
> +            break
> +            ;;
> +        esac
> +      fi
> +      dir=$(dirname "$dir")
> +    done
>    fi
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9