From: Jann Horn <jannh@xxxxxxxxxx>
Date: Thu, 15 May 2025 22:54:14 +0200
> > + /*
> > + * 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.
>
> 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?
Good catch, yes, sock_release() is not needed here.
>
> > + 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
> make sense to either give this highly privileged bit a separate value
> that can never come from userspace, or explicitly strip it away in
> __sys_connect_file() just to be safe.
I had the same thought, but I think it's fine to leave the code as
is for now. We can revisit it later once someone reports a strange
regression, which will be most unlikely.
