Re: [PATCH v2 3/5] selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Insecure use of hardcoded `/tmp` paths allows arbitrary file overwrites via symlink attacks, leading to Local Privilege Escalation.
- [Low] Incorrect substring replacement using `strstr()` leads to path corruption and erroneous test skipping.
--

--- Patch [3]: [PATCH v2 3/5] selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump ---
commit a5948cd46d81fc56087575c59d9596b04cf72843
Author: Christian Brauner <[email protected]>

selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump

Add a test that verifies that a coredump cannot be cut short by
TIF_NOTIFY_SIGNAL through io_uring running task work for uninterruptible
tasks.

> diff --git a/tools/testing/selftests/coredump/coredump_notify_signal_helper.c b/tools/testing/selftests/coredump/coredump_notify_signal_helper.c
> new file mode 100644
> index 0000000000000..849f5c1ea7365
> --- /dev/null
> +++ b/tools/testing/selftests/coredump/coredump_notify_signal_helper.c
> @@ -0,0 +1,46 @@

[ ... ]

> +int main(int argc, char *argv[])
> +{
> +	int fd_core_file;
> +	ssize_t ret;
> +
> +	fd_core_file = open(NOTIFY_SIGNAL_CORE_TMPFILE,
> +			    O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0600);

[Severity: High]
Could this hardcoded path be exploited via a symlink attack?

Since this helper is spawned as root by the kernel via core_pattern, an
unprivileged local attacker could pre-create a symlink at
/tmp/coredump.notify_signal.core.tmp pointing to a sensitive file like
/etc/shadow.

Because open() is called with O_TRUNC but without O_EXCL or O_NOFOLLOW,
would this blindly follow the symlink and truncate the target file?

> +	if (fd_core_file < 0) {
> +		fprintf(stderr, "%s: open failed: %m\n", argv[0]);
> +		return EXIT_FAILURE;
> +	}
> +
> +	ret = recv_coredump_notify_signal(STDIN_FILENO, fd_core_file, true);

[ ... ]

> diff --git a/tools/testing/selftests/coredump/coredump_notify_signal_test.c b/tools/testing/selftests/coredump/coredump_notify_signal_test.c
> new file mode 100644
> index 0000000000000..4a98ab141c419
> --- /dev/null
> +++ b/tools/testing/selftests/coredump/coredump_notify_signal_test.c
> @@ -0,0 +1,245 @@

[ ... ]

> +TEST_F(coredump, notify_signal_pipe)
> +{
> +	char pattern[PATH_MAX], helper[PATH_MAX], *p;
> +	struct stat st;
> +	int status, i;
> +	pid_t pid;
> +	ssize_t n;
> +
> +	if (!coredump_io_uring_available())
> +		SKIP(return, "io_uring not available");
> +
> +	n = readlink("/proc/self/exe", helper, sizeof(helper) - 1);
> +	ASSERT_GT(n, 0);
> +	helper[n] = '\0';
> +	p = strstr(helper, "coredump_notify_signal_test");
> +	ASSERT_NE(p, NULL);
> +	ASSERT_LE((size_t)(p - helper) + sizeof("coredump_notify_signal_helper"),
> +		  sizeof(helper));
> +	strcpy(p, "coredump_notify_signal_helper");

[Severity: Low]
Can this strstr() matching lead to path corruption if the kernel source
tree is placed in a directory named coredump_notify_signal_test?

By matching the first occurrence of the string rather than the basename,
the directory component would be replaced instead, leading to a failed
access() check and the test being silently skipped.

> +	if (access(helper, X_OK))
> +		SKIP(return, "coredump_notify_signal_helper not built");

[ ... ]

> +TEST_F(coredump, notify_signal_socket)
> +{
> +	pid_t pid, pid_coredump_server;
> +	int ipc_sockets[2], status;
> +	char pattern[PATH_MAX];
> +	char c;

[ ... ]

> +	pid_coredump_server = fork();
> +	ASSERT_GE(pid_coredump_server, 0);
> +	if (pid_coredump_server == 0) {
> +		int fd_server = -1, fd_coredump = -1, fd_core_file = -1;
> +		int exit_code = EXIT_FAILURE;
> +
> +		close(ipc_sockets[0]);
> +
> +		fd_server = create_and_listen_unix_socket(NOTIFY_SIGNAL_SOCKET);
> +		if (fd_server < 0)
> +			goto out;
> +		if (write_nointr(ipc_sockets[1], "1", 1) < 0)
> +			goto out;
> +		close(ipc_sockets[1]);
> +
> +		fd_coredump = accept4(fd_server, NULL, NULL, SOCK_CLOEXEC);
> +		if (fd_coredump < 0)
> +			goto out;
> +
> +		fd_core_file = open(NOTIFY_SIGNAL_CORE_FILE,
> +				    O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC,
> +				    0600);

[Severity: High]
Is this open() vulnerable to the same /tmp symlink attack as the helper
above?

Adding O_NOFOLLOW | O_EXCL might be necessary to ensure we aren't
overwriting arbitrary files if the test runner operates with elevated
privileges.

> +		if (fd_core_file < 0)
> +			goto out;
> +
> +		if (recv_coredump_notify_signal(fd_coredump, fd_core_file,
> +						true) < 0)
> +			goto out;

[ ... ]

> diff --git a/tools/testing/selftests/coredump/coredump_test_helpers.c b/tools/testing/selftests/coredump/coredump_test_helpers.c
> index 2a20faf9cb0ad..f8d416180d5b3 100644
> --- a/tools/testing/selftests/coredump/coredump_test_helpers.c
> +++ b/tools/testing/selftests/coredump/coredump_test_helpers.c
> @@ -381,3 +391,304 @@

[ ... ]

> +static int make_file_mappings(void)
> +{
> +	long pgsz = sysconf(_SC_PAGESIZE);
> +	int fd, i;
> +
> +	fd = open(NOTIFY_SIGNAL_MAPFILE,
> +		  O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC, 0600);

[Severity: High]
Does this file creation face the same symlink overwrite vulnerability as
the other hardcoded /tmp paths?

> +	if (fd < 0)
> +		return 0;
> +	if (ftruncate(fd, (off_t)NOTIFY_SIGNAL_MAP_COUNT * pgsz)) {
> +		close(fd);
> +		return 0;
> +	}

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