Thread (7 messages) 7 messages, 3 authors, 1d ago

Re: [RFC PATCH 3/3] coredump, net: remove `SOCK_COREDUMP`

From: Christian Brauner <brauner@kernel.org>
Date: 2026-07-03 08:11:26
Also in: linux-fsdevel, linux-security-module, lkml

quoted hunk ↗ jump to hunk
The utility of the refactors of the last two commits is demonstrated by
fixing this glaring layer violation: the unix socket implementation
knowing about the coredump socket caller.

Before, when the only way to connect to a socket was via the UAPI
`struct sockaddr_un`, the only way to implement the proper logic that
the kernel needs to resolve the coredump socket path was via hacking it
into the socket implementation.

In addition to being quite ugly, this layer violation is not great for
security. The intent is that `SOCK_COREDUMP` can only be used by the
kernel, and to be clear, I have no reason to believe this is not
correctly enforced today. But because of the many functions with flags
arguments, this is not a locally-enforced invariant. Some change, at
some point, could mess up, and allow user-provided flags to sneak in,
and this strikes me as a mistake waiting to happen. At that point, a
user could exploit this to connect to any socket it likes, bypassing
permission checks.

Now, with the two functions we've just previously factored out, we can
fix the layering. The custom path lookup logic lives with the coredump
caller, where it belongs. Once the right `struct path` is found
(actually just the inode is needed), the coredump caller can resolve a
`struct sock *` from it, and then directly connect to it. With this
change, `SOCK_COREDUMP` is no longer needed at all, and can be deleted.
The layer violation is gone, and the security footgun is gone with it.

As an added bonus, remove `flags` parameters from a number of internal
functions that no longer need them. They were just taking flags
parameters for the sake of `SOCK_COREDUMP`, and so with that gone, those
flag parameters are also no longer needed. If or when there is a new
flag that motivates them, they can be added back.

Tested that `coredump_socket_protocol_test` still passes.

Signed-off-by: John Ericson <redacted>
diff --git a/fs/coredump.c b/fs/coredump.c
index e68a76ff92a3..e1452021218e 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -14,6 +14,7 @@
 #include <linux/perf_event.h>
 #include <linux/highmem.h>
 #include <linux/spinlock.h>
+#include <linux/cred.h>
 #include <linux/key.h>
 #include <linux/personality.h>
 #include <linux/binfmts.h>
@@ -21,6 +22,7 @@
 #include <linux/sort.h>
 #include <linux/sched/coredump.h>
 #include <linux/sched/signal.h>
+#include <linux/sched/task.h>
 #include <linux/sched/task_stack.h>
 #include <linux/utsname.h>
 #include <linux/pid_namespace.h>
@@ -50,7 +52,6 @@
 #include <net/net_namespace.h>
 #include <net/sock.h>
 #include <uapi/linux/pidfd.h>
-#include <uapi/linux/un.h>
 #include <uapi/linux/coredump.h>
 
 #include <linux/uaccess.h>
@@ -668,17 +669,10 @@ static int umh_coredump_setup(struct subprocess_info *info, struct cred *new)
 static bool coredump_sock_connect(struct core_name *cn, struct coredump_params *cprm)
 {
 	struct file *file __free(fput) = NULL;
-	struct sockaddr_un addr = {
-		.sun_family = AF_UNIX,
-	};
-	ssize_t addr_len;
-	int retval;
+	struct path root, path;
 	struct socket *socket;
-
-	addr_len = strscpy(addr.sun_path, cn->corename);
-	if (addr_len < 0)
-		return false;
-	addr_len += offsetof(struct sockaddr_un, sun_path) + 1;
+	struct sock *sk;
+	int retval;
 
 	/*
 	 * It is possible that the userspace process which is supposed
@@ -710,14 +704,37 @@ static bool coredump_sock_connect(struct core_name *cn, struct coredump_params *
 	 */
 	pidfs_coredump(cprm);
 
-	retval = kernel_connect(socket, (struct sockaddr_unsized *)(&addr), addr_len,
-				O_NONBLOCK | SOCK_COREDUMP);
+	/*
+	 * Resolve the socket path relative to init's root and with kernel
+	 * credentials, and with symlinks, magic links and escaping the
+	 * root all forbidden, so the dumping process cannot use its own
+	 * filesystem view to redirect its core to an arbitrary socket.
+	 */
+	task_lock(&init_task);
+	get_fs_root(init_task.fs, &root);
+	task_unlock(&init_task);
+
+	scoped_with_kernel_creds()
+		retval = vfs_path_lookup(root.dentry, root.mnt, cn->corename,
+					 LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
+					 LOOKUP_NO_MAGICLINKS, &path);
+	path_put(&root);
+	if (retval)
+		return false;
+
+	/* Connect directly to the socket bound there, by fd not by name. */
+	sk = unix_lookup_bsd_path(&path, SOCK_STREAM);
+	path_put(&path);
+	if (IS_ERR(sk))
+		return false;
 
+	retval = kernel_unix_connect_direct(sk, socket, O_NONBLOCK);
I don't think dragging the bowels of net/ into fs/coredump.c just for
the sake of getting a purely internal SOCK_COREDUMP flag out of the way
is the right way to go.

The two helpers also make no sense to me and force a bunch of cleanup on
the caller as well.

I suspect the saner thing to do would be to introduce a primitive for
connecting to an AF_UNIX path-based socket directly instead of this
weird dance here, no?

int kernel_unix_connect(const char *path, struct socket *socket, unsigned int o_flags, int type)

then my kthread changes would collapse this into:

scoped_with_init_fs()
	retval = kernel_unix_connect(path, socket, O_NONBLOCK, SOCK_STREAM);
quoted hunk ↗ jump to hunk
+	sock_put(sk);
 	if (retval) {
 		if (retval == -EAGAIN)
-			coredump_report_failure("Coredump socket %s receive queue full", addr.sun_path);
+			coredump_report_failure("Coredump socket %s receive queue full", cn->corename);
 		else
-			coredump_report_failure("Coredump socket connection %s failed %d", addr.sun_path, retval);
+			coredump_report_failure("Coredump socket connection %s failed %d", cn->corename, retval);
 		return false;
 	}
 
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 65c9609ec207..3d6fbb6d2628 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -323,8 +323,7 @@ LSM_HOOK(int, 0, watch_key, struct key *key)
 #endif /* CONFIG_SECURITY && CONFIG_KEY_NOTIFICATIONS */
 
 #if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
-LSM_HOOK(int, 0, unix_find, const struct path *path, struct sock *other,
-	 int flags)
+LSM_HOOK(int, 0, unix_find, const struct path *path, struct sock *other)
I'm not sure why we would go on altering all kinds of LSM hooks as well.

-- 
Christian Brauner [off-list ref]
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help