Re: [PATCH v7 1/7] perf tests: Create a structure for shell tests

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Stack buffer overflow when constructing file paths with readlink().
- [High] Unsanitized file paths are passed to system(), leading to test failures and arbitrary command execution.
- [Medium] Inconsistent symlink traversal in `is_directory_fd` can cause infinite recursion.
--

commit 3485f6ce4f46698279bf2224bae7f656e8fcbc73
Author: Jakub Brnak <jbrnak@redhat.com>

perf tests: Create a structure for shell tests

This commit implements a new structure for shell test suites, grouping them
by subdirectories within perf/tests/shell instead of placing them all into a
single list.

> diff --git a/tools/perf/tests/tests-scripts.c b/tools/perf/tests/tests-scripts.c
> index f18c4cd337c8f..680dfe650f146 100644
> --- a/tools/perf/tests/tests-scripts.c
> +++ b/tools/perf/tests/tests-scripts.c
> @@ -151,14 +200,64 @@ static char *strdup_check(const char *str)
>  	return newstr;
>  }
>  
> -static int shell_test__run(struct test_suite *test, int subtest __maybe_unused)
> +/* Check if dirent entry is a directory using file descriptor, handling DT_UNKNOWN case */
> +static bool is_directory_fd(int dir_fd, struct dirent *ent)
>  {
> -	const char *file = test->priv;
> +	struct stat st;
> +
> +	if (ent->d_type == DT_DIR)
> +		return true;
> +
> +	if (ent->d_type != DT_UNKNOWN)
> +		return false;
> +
> +	/* Need to stat the file to determine if it's a directory */
> +	if (fstatat(dir_fd, ent->d_name, &st, 0) != 0)
> +		return false;

Could this cause inconsistent symlink traversal depending on the filesystem?
If the filesystem returns DT_UNKNOWN, fstatat() without the AT_SYMLINK_NOFOLLOW
flag will follow symlinks and report them as directories. This differs from the
fast path which correctly skips symlinks because their d_type is DT_LNK.
Could this lead to infinite recursion if a symlink loops back to a parent
directory?

> +
> +	return S_ISDIR(st.st_mode);
> +}

[ ... ]

> +static int shell_test__run(struct test_suite *test, int subtest)
> +{
> +	const char *file;
>  	int err;
>  	char *cmd = NULL;
>  
> +	/* Get absolute file path */
> +	if (subtest >= 0)
> +		file = test->test_cases[subtest].name;
> +	else	/* Single test case */
> +		file = test->test_cases[0].name;
> +
>  	if (asprintf(&cmd, "%s%s", file, verbose ? " -v" : "") < 0)
>  		return TEST_FAIL;
> +
>  	err = system(cmd);

Since file contains the absolute path to the test script, what happens if the
kernel source tree is located in a directory path containing spaces or shell
metacharacters? Will system() fail to execute the path properly?

[ ... ]

> @@ -167,65 +266,117 @@ static int shell_test__run(struct test_suite *test, int subtest __maybe_unused)
>  	return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
>  }
>  
> -static void append_script(int dir_fd, const char *name, char *desc,
> -			  struct test_suite ***result,
> -			  size_t *result_sz)
> +static struct test_suite *prepare_test_suite(int dir_fd)
>  {
> -	char filename[PATH_MAX], link[128];
> -	struct test_suite *test_suite, **result_tmp;
> -	struct test_case *tests;
> +	char dirpath[PATH_MAX], link[128];
>  	ssize_t len;
> -	char *exclusive;
> +	struct test_suite *test_suite = NULL;
> +	struct shell_test_info *test_info;
>  
> +	/* Get dir absolute path */
>  	snprintf(link, sizeof(link), "/proc/%d/fd/%d", getpid(), dir_fd);
> -	len = readlink(link, filename, sizeof(filename));
> +	len = readlink(link, dirpath, sizeof(dirpath));
>  	if (len < 0) {
>  		pr_err("Failed to readlink %s", link);
> -		return;
> +		return NULL;
>  	}
> -	filename[len++] = '/';
> -	strcpy(&filename[len], name);
> +	dirpath[len++] = '/';
> +	dirpath[len] = '\0';

If readlink() fills the entire dirpath array, returning a length of PATH_MAX,
won't these assignments write out of bounds and corrupt the stack?

[ ... ]

> +static void append_script_to_suite(int dir_fd, const char *name, char *desc,
> +					struct test_suite *test_suite, size_t *tc_count)
> +{
> +	char file_name[PATH_MAX], link[128];
> +	struct test_case *tests;
> +	ssize_t len;
> +	char *exclusive;

[ ... ]

> +	/* Get path to the test script */
> +	snprintf(link, sizeof(link), "/proc/%d/fd/%d", getpid(), dir_fd);
> +	len = readlink(link, file_name, sizeof(file_name));
> +	if (len < 0) {
> +		pr_err("Failed to readlink %s", link);
> +		free(desc);
> +		return;
> +	}
> +	file_name[len++] = '/';
> +	strcpy(&file_name[len], name);

Similarly here, if readlink() returns PATH_MAX, file_name[len++] will write past
the end of the array. Also, won't strcpy() overflow the buffer if there isn't
enough space left in file_name to hold the script name?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416111419.385010-1-jbrnak@redhat.com?part=1