On Thu, May 15, 2025 at 10:56:26PM +0200, Jann Horn wrote:
> On Thu, May 15, 2025 at 12:04 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > Extend the PIDFD_INFO_COREDUMP ioctl() with the new PIDFD_INFO_COREDUMP
> > mask flag. This adds the fields @coredump_mask and @coredump_cookie to
> > struct pidfd_info.
>
> FWIW, now that you're using path-based sockets and override_creds(),
> one option may be to drop this patch and say "if you don't want
> untrusted processes to directly connect to the coredumping socket,
> just set the listening socket to mode 0000 or mode 0600"...
>
> > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> [...]
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index e1256ebb89c1..bfc4a32f737c 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> [...]
> > @@ -876,8 +880,34 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> > goto close_fail;
> > }
> >
> > + /*
> > + * Set the thread-group leader pid which is used for the
> > + * peer credentials during connect() below. Then
> > + * immediately register it in pidfs...
> > + */
> > + cprm.pid = task_tgid(current);
> > + retval = pidfs_register_pid(cprm.pid);
> > + if (retval) {
> > + sock_release(socket);
> > + goto close_fail;
> > + }
> > +
> > + /*
> > + * ... and set the coredump information so userspace
> > + * has it available after connect()...
> > + */
> > + pidfs_coredump(&cprm);
> > +
> > + /*
> > + * ... On connect() the peer credentials are recorded
> > + * and @cprm.pid registered in pidfs...
>
> I don't understand this comment. Wasn't "@cprm.pid registered in
> pidfs" above with the explicit `pidfs_register_pid(cprm.pid)`?
I'll answer both questions in one go below...
>
> > + */
> > retval = kernel_connect(socket, (struct sockaddr *)(&addr),
> > addr_len, O_NONBLOCK | SOCK_COREDUMP);
> > +
> > + /* ... So we can safely put our pidfs reference now... */
> > + pidfs_put_pid(cprm.pid);
>
> Why can we safely put the pidfs reference now but couldn't do it
> before the kernel_connect()? Does the kernel_connect() look up this
> pidfs entry by calling something like pidfs_alloc_file()? Or does that
> only happen later on, when the peer does getsockopt(SO_PEERPIDFD)?
AF_UNIX sockets support SO_PEERPIDFD as you know. Users such as dbus or
systemd want to be able to retrieve a pidfd for the peer even if the
peer has already been reaped. To support this AF_UNIX ensures that when
the peer credentials are set up (connect(), listen()) the corresponding
@pid will also be registered in pidfs. This ensures that exit
information is stored in the inode if we hand out a pidfd for a reaped
task. IOW, we only hand out pidfds for reaped task if at the time of
reaping a pidfs entry existed for it.
Since we're setting coredump information on the pidfd here we're calling
pidfs_register_pid() even before connect() sets up the peer credentials
so we're sure that the coredump information is stored in the inode.
Then we delay our pidfs_put_pid() call until the connect() took it's own
reference and thus continues pinning the inode. IOW, connect() will also
call pidfs_register_pid() but it will ofc just increment the reference
count ensuring that our pidfs_put_pid() doesn't drop the inode.
If we immediately did a pidfs_put_pid() before connect() we'd loose the
coredump information.
>
> > if (retval) {
> > if (retval == -EAGAIN)
> > coredump_report_failure("Coredump socket %s receive queue full", addr.sun_path);
> [...]
> > diff --git a/fs/pidfs.c b/fs/pidfs.c
> > index 3b39e471840b..d7b9a0dd2db6 100644
> > --- a/fs/pidfs.c
> > +++ b/fs/pidfs.c
> [...]
> > @@ -280,6 +299,13 @@ static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
> > }
> > }
> >
> > + if (mask & PIDFD_INFO_COREDUMP) {
> > + kinfo.mask |= PIDFD_INFO_COREDUMP;
> > + smp_rmb();
>
> I assume I would regret it if I asked what these barriers are for,
> because the answer is something terrifying about how we otherwise
> don't have a guarantee that memory accesses can't be reordered between
> multiple subsequent syscalls or something like that?
No, not really. It's just so that when someone calls PIDFD_GET_INFO with
PIDFD_INFO_COREDUMP but one gotten from the coredump socket that they
don't see half-initialized information. I can just use WRITE_ONCE() for
that.
>
> checkpatch complains about the lack of comments on these memory barriers.
I'll just use WRITE_ONCE().
>
> > + kinfo.coredump_cookie = READ_ONCE(pidfs_i(inode)->__pei.coredump_cookie);
> > + kinfo.coredump_mask = READ_ONCE(pidfs_i(inode)->__pei.coredump_mask);
> > + }
> > +
> > task = get_pid_task(pid, PIDTYPE_PID);
> > if (!task) {
> > /*
> [...]
> > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > index a9d1c9ba2961..053d2e48e918 100644
> > --- a/net/unix/af_unix.c
> > +++ b/net/unix/af_unix.c
> [...]
> > @@ -742,6 +743,7 @@ static void unix_release_sock(struct sock *sk, int embrion)
> >
> > struct unix_peercred {
> > struct pid *peer_pid;
> > + u64 cookie;
>
> Maybe add a comment here documenting that for now, this is assumed to
> be used exclusively for coredump sockets.
I think we should just drop it.
