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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing cleanup of stale temporary files in FIXTURE_SETUP can bypass the polling synchronization in the notify_signal_pipe test, leading to false test successes.
- [Medium] The test suite uses predictable, hardcoded paths in /tmp/ and opens them with O_CREAT | O_TRUNC without O_EXCL, creating a vulnerability to symlink truncation attacks.
- [Medium] The test globally changes /proc/sys/kernel/core_pattern to point to a test-specific helper. If the test terminates abnormally, this configuration is never restored.
--

--- Patch [3]: [PATCH 3/4] selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump ---
commit c11ee8bbfd73149ce9b69d88e45aeab900d75512
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: Medium]
Could this predictable hardcoded path in /tmp/ be vulnerable to a symlink
truncation attack? The helper is spawned by the kernel (typically as root)
and opens this path without O_EXCL. If an unprivileged user pre-creates this
path as a symlink, it might truncate important system files.

> +	if (fd_core_file < 0) {
> +		fprintf(stderr, "%s: open failed: %m\n", argv[0]);
> +		return EXIT_FAILURE;
> +	}
[ ... ]
> 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..e63c06a81562f
> --- /dev/null
> +++ b/tools/testing/selftests/coredump/coredump_notify_signal_test.c
> @@ -0,0 +1,245 @@
[ ... ]
> +FIXTURE_SETUP(coredump)
> +{
> +	FILE *file;
> +	int ret;
[ ... ]
> +	self->fd_tmpfs_detached = create_detached_tmpfs();
> +	ASSERT_GE(self->fd_tmpfs_detached, 0);
> +
> +	ret = fclose(file);
> +	ASSERT_EQ(0, ret);
> +
> +	unlink(NOTIFY_SIGNAL_TRIGGER);
> +	ASSERT_EQ(mkfifo(NOTIFY_SIGNAL_TRIGGER, 0600), 0);
> +}

[Severity: Medium]
Should FIXTURE_SETUP() also unlink NOTIFY_SIGNAL_CORE_FILE? If a previous
test run fails or is interrupted before teardown, a stale file could be left
in /tmp/. The polling loop in notify_signal_pipe() would immediately break
when it sees this stale file instead of waiting for the new coredump, leading
to a false test success.

[ ... ]
> +TEST_F(coredump, notify_signal_pipe)
> +{
> +	char pattern[PATH_MAX], helper[PATH_MAX], *p;
> +	struct stat st;
[ ... ]
> +	ASSERT_LT(snprintf(pattern, sizeof(pattern), "|%s", helper),
> +		  (int)sizeof(pattern));
> +	ASSERT_TRUE(set_core_pattern(pattern));

[Severity: Medium]
What happens if the test terminates abnormally (e.g. via SIGKILL) after
modifying core_pattern here? FIXTURE_TEARDOWN() would not run, leaving the
global /proc/sys/kernel/core_pattern permanently pointing to the test's
temporary helper binary. Could this break core dumps for the entire system?

[ ... ]
> diff --git a/tools/testing/selftests/coredump/coredump_test_helpers.c b/tools/testing/selftests/coredump/coredump_test_helpers.c
> index 2a20faf9cb0ad..20fcc5b644c32 100644
> --- a/tools/testing/selftests/coredump/coredump_test_helpers.c
> +++ b/tools/testing/selftests/coredump/coredump_test_helpers.c
> @@ -1,11 +1,18 @@
[ ... ]
> +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: Medium]
Is this another potential symlink truncation vulnerability? Like the helper
above, this opens a predictable, hardcoded path in /tmp/ with O_CREAT | O_TRUNC
but without O_EXCL.

> +	if (fd < 0)
> +		return 0;

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