Re: [PATCH v7 4/9] coredump: add coredump socket

[Christian Brauner <brauner@kernel.org>]

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@kernel.org> 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.