On Thu, May 15, 2025 at 10:54:14PM +0200, Jann Horn wrote:
> On Thu, May 15, 2025 at 12:04 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index a70929c3585b..e1256ebb89c1 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> [...]
> > @@ -393,11 +428,20 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm,
> > * If core_pattern does not include a %p (as is the default)
> > * and core_uses_pid is set, then .%pid will be appended to
> > * the filename. Do not do this for piped commands. */
> > - if (!(cn->core_type == COREDUMP_PIPE) && !pid_in_pattern && core_uses_pid) {
> > - err = cn_printf(cn, ".%d", task_tgid_vnr(current));
> > - if (err)
> > - return err;
> > + if (!pid_in_pattern && core_uses_pid) {
> > + switch (cn->core_type) {
> > + case COREDUMP_FILE:
> > + return cn_printf(cn, ".%d", task_tgid_vnr(current));
> > + case COREDUMP_PIPE:
> > + break;
> > + case COREDUMP_SOCK:
> > + break;
>
> This branch is dead code, we can't get this far down with
> COREDUMP_SOCK. Maybe you could remove the "break;" and fall through to
> the default WARN_ON_ONCE() here. Or better, revert this hunk and
> instead just change the check to check for "cn->core_type ==
> COREDUMP_FILE" (in patch 1), since this whole block is legacy logic
> specific to dumping into files (COREDUMP_FILE).
Ok, folded:
diff --git a/fs/coredump.c b/fs/coredump.c
index 368751d98781..45725465c299 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -393,11 +393,8 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm,
* If core_pattern does not include a %p (as is the default)
* and core_uses_pid is set, then .%pid will be appended to
* the filename. Do not do this for piped commands. */
- if (!(cn->core_type == COREDUMP_PIPE) && !pid_in_pattern && core_uses_pid) {
- err = cn_printf(cn, ".%d", task_tgid_vnr(current));
- if (err)
- return err;
- }
+ if (cn->core_type == COREDUMP_FILE && !pid_in_pattern && core_uses_pid)
+ return cn_printf(cn, ".%d", task_tgid_vnr(current));
return 0;
}
into the first patch.
>
> > + default:
> > + WARN_ON_ONCE(true);
> > + return -EINVAL;
> > + }
> > }
> > +
> > return 0;
> > }
> >
> > @@ -801,6 +845,55 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> > }
> > break;
> > }
> > + case COREDUMP_SOCK: {
> > +#ifdef CONFIG_UNIX
> > + struct file *file __free(fput) = NULL;
> > + struct sockaddr_un addr = {
> > + .sun_family = AF_UNIX,
> > + };
> > + ssize_t addr_len;
> > + struct socket *socket;
> > +
> > + retval = strscpy(addr.sun_path, cn.corename, sizeof(addr.sun_path));
>
> nit: strscpy() explicitly supports eliding the last argument in this
> case, thanks to macro magic:
>
> * The size argument @... is only required when @dst is not an array, or
> * when the copy needs to be smaller than sizeof(@dst).
Ok.
>
> > + if (retval < 0)
> > + goto close_fail;
> > + addr_len = offsetof(struct sockaddr_un, sun_path) + retval + 1;
>
> nit: On a 64-bit system, strscpy() returns a 64-bit value, and
> addr_len is also 64-bit, but retval is 32-bit. Implicitly moving
> length values back and forth between 64-bit and 32-bit is slightly
> dodgy and might generate suboptimal code (it could force the compiler
> to emit instructions to explicitly truncate the value if it can't
> prove that the value fits in 32 bits). It would be nice to keep the
> value 64-bit throughout by storing the return value in a ssize_t.
>
> And actually, you don't have to compute addr_len here at all; that's
> needed for abstract unix domain sockets, but for path-based unix
> domain socket, you should be able to just use sizeof(struct
> sockaddr_un) as addrlen. (This is documented in "man 7 unix".)
Ok, folded:
@@ -845,10 +845,10 @@ void do_coredump(const kernel_siginfo_t *siginfo)
ssize_t addr_len;
struct socket *socket;
- retval = strscpy(addr.sun_path, cn.corename);
- if (retval < 0)
+ addr_len = strscpy(addr.sun_path, cn.corename);
+ if (addr_len < 0)
goto close_fail;
- addr_len = offsetof(struct sockaddr_un, sun_path) + retval + 1;
+ addr_len += offsetof(struct sockaddr_un, sun_path) + 1;
>
> > +
> > + /*
> > + * It is possible that the userspace process which is
> > + * supposed to handle the coredump and is listening on
> > + * the AF_UNIX socket coredumps. Userspace should just
> > + * mark itself non dumpable.
> > + */
> > +
> > + retval = sock_create_kern(&init_net, AF_UNIX, SOCK_STREAM, 0, &socket);
> > + if (retval < 0)
> > + goto close_fail;
> > +
> > + file = sock_alloc_file(socket, 0, NULL);
> > + if (IS_ERR(file)) {
> > + sock_release(socket);
>
> I think you missed an API gotcha here. See the sock_alloc_file() documentation:
>
> * On failure @sock is released, and an ERR pointer is returned.
Thanks, fixed.
>
> So I think basically sock_alloc_file() always consumes the socket
> reference provided by the caller, and the sock_release() in this
> branch is a double-free?
>
> > + goto close_fail;
> > + }
> [...]
> > diff --git a/include/linux/net.h b/include/linux/net.h
> > index 0ff950eecc6b..139c85d0f2ea 100644
> > --- a/include/linux/net.h
> > +++ b/include/linux/net.h
> > @@ -81,6 +81,7 @@ enum sock_type {
> > #ifndef SOCK_NONBLOCK
> > #define SOCK_NONBLOCK O_NONBLOCK
> > #endif
> > +#define SOCK_COREDUMP O_NOCTTY
>
> Hrrrm. I looked through all the paths from which the ->connect() call
> can come, and I think this is currently safe; but I wonder if it would
Yes, I made sure that unknown bits are excluded.
