Date:   Mon, 14 Feb 2022 12:07:25 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Robert Święcki <robert@...ecki.net>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Jann Horn <jannh@...gle.com>, Will Drewry <wad@...omium.org>,
        linux-api@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [RFC] Get siginfo from unreaped task

On Sun, Feb 13, 2022 at 09:52:12AM +0100, Christian Brauner wrote:
> On Sat, Feb 12, 2022 at 06:32:08PM -0800, Andy Lutomirski wrote:
> > 
> > > On Feb 12, 2022, at 3:24 AM, Robert Święcki <robert@...ecki.net> wrote:
> > > 
> > > sob., 12 lut 2022 o 05:28 Kees Cook <keescook@...omium.org> napisał(a):
> > >> 
> > >> Make siginfo available through PTRACE_GETSIGINFO after process death,
> > >> without needing to have already used PTRACE_ATTACH. Uses 48 more bytes
> > >> in task_struct, though I bet there might be somewhere else we could
> > >> stash a copy of it?
> > > 
> > > An alternative way of accessing this info could be abusing the
> > > waitid() interface, with some additional, custom to Linux, flag
> > > 
> > > waitid(P_ALL, 0, &si, __WCHILDSIGINFO);
> > > 
> > > which would change what is put into si.
> > > 
> > > But maybe ptrace() is better, because it's mostly incompatible with
> > > other OSes anyway on the behavior/flag level, while waitd() seems to
> > > be POSIX/BSD standard, even if Linux specifies some additional flags.
> > > 
> > > 
> > 
> > I had a kind of opposite thought, which is that it would be very nice
> > to be able to get all the waitid() data without reaping a process or
> > even necessarily being its parent.  Maybe these can be combined?  A
> > new waitid() option like you’re suggesting could add siginfo (and
> > might need permissions).  And we could have a different waitid() flag
> > that says “maybe not my child, don’t reap” (and also needs
> > permissions).
> > 
> > Although the “don’t reap” thing is fundamentally racy. What a sane
> > process manager actually wants is an interface to read all this info
> > from a pidfd, which means it all needs to get stuck in struct pid. And
> 
> /me briefly pops out from vacation
> 
> Agreed and not just siginfo I would expect(?). We already came to that
> conclusion when we first introduced them.
> 
> > task_struct needs a completion or wait queue so you can actually wait
> > for a pidfd to exit (unless someone already did this — I had patches a
> > while back).  And this would be awesome.
> 
> Currently, you can wait for a pidfd to exit via polling and you can use
> a pidfd to pass it to waitid(P_PIDFD, pidfd, ...).
> 
> /me pops back into vacation

Right, so waitid already has all the infrastructure for this, so I think
adding it there makes a lot of sense. Here's what I've got:



diff --git a/include/linux/sched.h b/include/linux/sched.h
index f5b2be39a78c..e40789e801ef 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1178,6 +1178,7 @@ struct task_struct {
 #endif
 	/* Ptrace state: */
 	unsigned long			ptrace_message;
+	kernel_siginfo_t		death_siginfo;
 	kernel_siginfo_t		*last_siginfo;
 
 	struct task_io_accounting	ioac;
diff --git a/kernel/signal.c b/kernel/signal.c
index 9b04631acde8..41f6ba6b7aa7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2825,6 +2825,10 @@ bool get_signal(struct ksignal *ksig)
 		}
 
 	fatal:
+		/* Allow siginfo to be queried until reaped. */
+		copy_siginfo(&current->death_siginfo, &ksig->info);
+		current->last_siginfo = &current->death_siginfo;
+
 		spin_unlock_irq(&sighand->siglock);
 		if (unlikely(cgroup_task_frozen(current)))
 			cgroup_leave_frozen(true);
diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
index 85b809fc9f11..7258cd4510ba 100644
--- a/include/uapi/linux/wait.h
+++ b/include/uapi/linux/wait.h
@@ -9,6 +9,7 @@
 #define WCONTINUED	0x00000008
 #define WNOWAIT		0x01000000	/* Don't reap, just poll status.  */
 
+#define __WCHILDSIGINFO	0x10000000	/* Report child's siginfo. */
 #define __WNOTHREAD	0x20000000	/* Don't wait on children of other threads in this group */
 #define __WALL		0x40000000	/* Wait on all children, regardless of type */
 #define __WCLONE	0x80000000	/* Wait only on non-SIGCHLD children */
diff --git a/kernel/exit.c b/kernel/exit.c
index d54efddd378b..70ecb996cecd 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -953,6 +953,7 @@ struct waitid_info {
 	uid_t uid;
 	int status;
 	int cause;
+	kernel_siginfo_t siginfo;
 };
 
 struct wait_opts {
@@ -964,7 +965,7 @@ struct wait_opts {
 	int			wo_stat;
 	struct rusage		*wo_rusage;
 
-	wait_queue_entry_t		child_wait;
+	wait_queue_entry_t	child_wait;
 	int			notask_error;
 };
 
@@ -1012,11 +1013,16 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 	int state, status;
 	pid_t pid = task_pid_vnr(p);
 	uid_t uid = from_kuid_munged(current_user_ns(), task_uid(p));
-	struct waitid_info *infop;
+	struct waitid_info *infop = wo->wo_info;
 
 	if (!likely(wo->wo_flags & WEXITED))
 		return 0;
 
+	/* Before WNOWAIT so a copy can be extracted without reaping. */
+	if (unlikely(wo->wo_flags & __WCHILDSIGINFO)) {
+		if (infop && p->last_siginfo)
+			copy_siginfo(&infop->siginfo, p->last_siginfo);
+	}
 	if (unlikely(wo->wo_flags & WNOWAIT)) {
 		status = (p->signal->flags & SIGNAL_GROUP_EXIT)
 			? p->signal->group_exit_code : p->exit_code;
@@ -1121,7 +1127,6 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 		release_task(p);
 
 out_info:
-	infop = wo->wo_info;
 	if (infop) {
 		if ((status & 0x7f) == 0) {
 			infop->cause = CLD_EXITED;
@@ -1564,7 +1569,7 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
 	unsigned int f_flags = 0;
 
 	if (options & ~(WNOHANG|WNOWAIT|WEXITED|WSTOPPED|WCONTINUED|
-			__WNOTHREAD|__WCLONE|__WALL))
+			__WNOTHREAD|__WCLONE|__WALL|__WCHILDSIGINFO))
 		return -EINVAL;
 	if (!(options & (WEXITED|WSTOPPED|WCONTINUED)))
 		return -EINVAL;
@@ -1638,6 +1645,10 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
 	if (!infop)
 		return err;
 
+	/* __WCHILDSIGINFO */
+	if (info->siginfo.signo)
+		return copy_siginfo_to_user(infop, &info->siginfo);
+
 	if (!user_write_access_begin(infop, sizeof(*infop)))
 		return -EFAULT;
 
@@ -1781,6 +1792,12 @@ COMPAT_SYSCALL_DEFINE5(waitid,
 	if (!infop)
 		return err;
 
+	/* __WCHILDSIGINFO */
+	if (info->siginfo.signo)
+		return copy_siginfo_to_user32(
+				(struct compat_siginfo __user *)infop,
+				&info->siginfo);
+
 	if (!user_write_access_begin(infop, sizeof(*infop)))
 		return -EFAULT;
 


One usability question I have is:

- if the process just exited normally, should it return an empty
  siginfo, or should it ignore __WCHILDSIGINFO? (I have it ignoring it
  above.)


-- 
Kees Cook

Powered by blists - more mailing lists