From: Eric W. Biederman <hidden> Date: 2017-06-06 19:08:47
All,
I am posting this patches in the hope of some review of the strategy I
am taking and to let the individual patches be reviewed.
The intersection of wait, exit, ptrace, exec, and CLONE_THREAD does not
work correctly in the linux kernel.
An easy entry point into seeing the problem is that exec can wait
forever holding a mutex while waiting for userspace to call waitpid on a
ptraced thread. This is bad enough it has been observed to cause
deadlocks in real world situations today.
Another easy entry point is to see that a multi-threaded setuid won't
change the credentials on a zombie thread group leader. Which can allow
sending signals to a process that the credential change should forbid.
This is in violation of posix and the semantics we attempt to enforce in
linux.
I have been silent the last couple of weeks reading up on the history
and seeing if I could come up with a solution to the buggy semantics and
great big piles of technical debt that exists in this area of the
kernel. It unfortunately requires understanding all of the pieces to
come up with a satisfactory long term solution to this problem. My goal
is to fix enough of the issues that futher cleanups and bug fixes won't
require such comprehensive knowledge.
In large the solution I have found is to:
- Fix the semantics of wait.
This makes it clear what the semantics the rest of the fixes need to
maintain.
- To move everything possible from release_task to do_exit.
Moving code from release_task to do_exit means that zombie
threads can no longer be used to access shared thread group state,
removing the problem of stale credentials on zombie threads.
Much of the state that is freed in release_task instead of do_exit
is freed there because originally during the CLONE_THREAD development
there was no where else to put code that freed thread group state.
And my changes largely move state freeing back where it used to
be in 2.4 and earlier.
- To remove the concept of thread group leader (and allowing the first
thread to exit).
The concept of thread group leader keeps leading people into false
mental models of what the kernel actually does.
There is a lot of code that makes the mistake of assuming the thread
group leader is always alive. When the code doesn't make that mistake
it requires additional code complexity to deal with zombie thread
group leaders. As seen in the signal code which still doesn't
handle zombie thread group leaders correctly from a permission
checking perspective.
- To promote tgid to a first class concept in the kernel.
This is necessary if there isn't a thread group leader, and a
significant part of the functional changes I am making.
- To only allow sending signals through living threads.
This is a subset of removing the concept of thread group leader. But
worth calling out because signal handling is the most significant
offender.
My first batch of changes includes some trivial cleanups and then
includes the small semantic changes (AKA user visible) I noticed that
are necessary to give userspace consistent semantics.
The full set of changes that I have come up with is at:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git exit-cleanups-for-testing
Unless someone has a better suggestion I intend to collect this up as
a topic branch and merge this through my tree.
Review on the details and of the concept would be very much appreciated.
Eric W. Biederman (26):
alpha: Remove unused TASK_GROUP_LEADER
cgroup: Don't open code tasklist_empty()
signal: Do not perform permission checks when sending pdeath_signal
signal: Make group_send_sig_info static
exit: Remove the pointless clearing of SIGPENDING in __exit_signal
rlimit: Remove unnecessary grab of tasklist_lock
pidns: Improve the error handling in alloc_pid
exit: Make the runqueue rcu safe
signal: Don't allow sending SIGKILL or SIGSTOP to init
ptrace: Simplify ptrace_detach & exit_ptrace
wait: Properly implement __WCLONE handling in the presence of exec and ptrace
wait: Directly test for the two cases where wait_task_zombie is called
wait: Remove unused delay_group_leader
wait: Move changing of ptrace from wait_consider_task into wait_task_stopped
wait: Don't delay !ptrace_reparented leaders
exit: Fix reporting a ptraced !reparented leader has exited
exit: Rework the exit states for ptracees
wait: Fix WSTOPPED on a ptraced child
wait: Simpler code for clearing notask_error in wait_consider_task
wait: Don't pass the list to wait_consider_task
wait: Optmize waitpid
exit: Fix auto-wait of ptraced children
signal: Fix SIGCONT before group stop completes.
signal: In ptrace_stop improve identical signal detection
signal: In ptrace_stop use CLD_TRAPPED in all ptrace signals
pidns: Ensure zap_pid_ns_processes always terminates
arch/alpha/kernel/asm-offsets.c | 1 -
include/linux/sched.h | 7 +-
include/linux/sched/signal.h | 29 ++--
include/linux/sched/task.h | 4 +-
include/linux/signal.h | 1 -
kernel/cgroup/cgroup.c | 2 +-
kernel/exit.c | 375 ++++++++++++++++------------------------
kernel/fork.c | 3 +-
kernel/pid.c | 8 +-
kernel/pid_namespace.c | 2 +-
kernel/ptrace.c | 44 ++---
kernel/sched/core.c | 2 +-
kernel/sched/fair.c | 2 +-
kernel/signal.c | 122 +++++++------
kernel/sys.c | 13 +-
15 files changed, 267 insertions(+), 348 deletions(-)
Eric
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:11:24
This definition is for assembly code and no assembly code uses it.
Remove the definition so that when making future changes we don't
have to worry if alpha assembly code uses task->group_leader.
Signed-off-by: "Eric W. Biederman" <redacted>
---
arch/alpha/kernel/asm-offsets.c | 1 -
1 file changed, 1 deletion(-)
@@ -4440,7 +4440,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early)/* At system boot, before all subsystems have been*registered,notaskshavebeenforked,sowedon't*needtoinvokeforkcallbackshere.*/-BUG_ON(!list_empty(&init_task.tasks));+BUG_ON(!tasklist_empty());BUG_ON(online_css(css));
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:11:54
This fixes and old old regression. When Roland switched from
sending pdeath_signal with send_sig() to send_group_sig_info
it gained a permission check, and started taking the tasklist
lock. Roland earlier fixed the double taking of the tasklist
lock in 3f2a0d1df938 ("[PATCH] fix pdeath_signal SMP locking")
but pdeath_signal still performs an unnecessary permission
check.
Ordinarily I would be hesitant at fixing an ancient regression but
a permission check for our parent sending to us is almost never
likely to fail (so it is unlikely anyone has noticed), and it
is stupid. It makes absolutely no sense to see if our
parent has permission to send us a signal we requested be
sent to us.
As this is more permisssive there is no chance anything will break.
The information of if our parent is living is available elsewhere
getppid, tkill, and proc with no special permissions so this should
not be an information leak.
See https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/
for the bitkeeper era history that I refer to.
Fixes: da334d91ff70 ("[PATCH] linux-2.5.66-signal-cleanup.patch")
Signed-off-by: "Eric W. Biederman" <redacted>
---
kernel/exit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
On Tue, Jun 6, 2017 at 12:03 PM, Eric W. Biederman
[off-list ref] wrote:
As this is more permisssive there is no chance anything will break.
Actually, I do worry about the security issues here.
The thing is, the parent may be some system daemon that wants to catch
SIGCHLD, but we've used prctl and changed pdeath_signal to something
else (like SIGSEGV or something).
Do we really want to be able to kill a system daemon that we couldn't
use kill() on directly, just because that system daemon spawned us?
So I think those permission checks may actually be a good idea.
Although possibly they should be in prctl()..
Linus
From: Richard Weinberger <hidden> Date: 2017-06-06 21:42:12
Eric,
On Tue, Jun 6, 2017 at 9:03 PM, Eric W. Biederman [off-list ref] wrote:
This fixes and old old regression. When Roland switched from
sending pdeath_signal with send_sig() to send_group_sig_info
it gained a permission check, and started taking the tasklist
lock. Roland earlier fixed the double taking of the tasklist
lock in 3f2a0d1df938 ("[PATCH] fix pdeath_signal SMP locking")
but pdeath_signal still performs an unnecessary permission
check.
Ordinarily I would be hesitant at fixing an ancient regression but
a permission check for our parent sending to us is almost never
likely to fail (so it is unlikely anyone has noticed), and it
is stupid. It makes absolutely no sense to see if our
parent has permission to send us a signal we requested be
sent to us.
Another side effect of this change is that audit_signal_info() is no longer
being called. AFAICT this is a user space visible change.
--
Thanks,
//richard
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:12:03
There is no reason to take the tasklist lock here. The sighand
structure is never referenced and and tsk->signal is guaranteed
to stick around until tsk is freed. Further update_rlimit_cpu
does not need the tasklist_lock. And the rlim_lock is used
to guarantee mutual exclusion.
Signed-off-by: "Eric W. Biederman" <redacted>
---
kernel/sys.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
@@ -1380,13 +1380,6 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,return-EPERM;}-/* protect tsk->signal and tsk->sighand from disappearing */-read_lock(&tasklist_lock);-if(!tsk->sighand){-retval=-ESRCH;-gotoout;-}-rlim=tsk->signal->rlim+resource;task_lock(tsk->group_leader);if(new_rlim){
@@ -1425,8 +1418,7 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,new_rlim->rlim_cur!=RLIM_INFINITY&&IS_ENABLED(CONFIG_POSIX_TIMERS))update_rlimit_cpu(tsk,new_rlim->rlim_cur);-out:-read_unlock(&tasklist_lock);+returnretval;}
Hi Eric,
I'll try very much to read this series tomorrow, can't do this today...
On 06/06, Eric W. Biederman wrote:
quoted hunk
@@ -1380,13 +1380,6 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource, return -EPERM; }- /* protect tsk->signal and tsk->sighand from disappearing */- read_lock(&tasklist_lock);- if (!tsk->sighand) {- retval = -ESRCH;- goto out;- }
Yes, the comment is wrong.
However we do need read_lock(tasklist_lock) to access ->group_leader. And the
->sighand != NULL check ensures that ->group_leader is the valid pointer.
Also, update_rlimit_cpu() is not safe without tasklist / sighand-check.
We can probably change this code to rely on rcu.
Oleg.
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:12:17
Cause any failure to allocate pid 1 to permanently disable pid
allocations for the pid namespace.
Before the pid becomes pid 1 there ns->last_pid and other state
remains unchanged so it is safe to try again...
Signed-off-by: "Eric W. Biederman" <redacted>
---
kernel/pid.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:12:27
Even to init SIGKILL and SIGSTOP are alwasys delivered if they are
sent, so don't allow tracing an init task allow them.
Signed-off-by: "Eric W. Biederman" <redacted>
---
kernel/signal.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:12:39
Call __ptrace_unlink not __ptrace_detach. As it is guaranteed that
ptrace_detach will never be called on a process that has or may exit.
Rename __ptrace_detach __exit_ptrace as exit_ptrace is now it's only
caller and the corrected name is less confusing.
Signed-off-by: "Eric W. Biederman" <redacted>
---
kernel/ptrace.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:12:41
Add a rcu_usage to task_struct and use it to reuse the delayed rcu put
logic from release_task in finish_task_switch. This guarantees that
there will be an rcu interval before usage drops to zero for any task
on the run queue. Making it safe to unconditionally call
get_task_struct in a rcu critical section for any task on the run
queue.
This guarantee in turn allows the fair scheduluer to use ordinary rcu
primitives to access tasks on the run queue and makes the magic functions
task_rcu_dereference and try_get_task_struct completely unnecessary.
Signed-off-by: "Eric W. Biederman" <redacted>
---
include/linux/sched.h | 1 +
include/linux/sched/task.h | 4 +--
kernel/exit.c | 83 ++++------------------------------------------
kernel/fork.c | 3 +-
kernel/sched/core.c | 2 +-
kernel/sched/fair.c | 2 +-
6 files changed, 12 insertions(+), 83 deletions(-)
@@ -492,6 +492,7 @@ struct task_struct {volatilelongstate;void*stack;atomic_tusage;+atomic_trcu_usage;/* Per task flags (PF_*), defined further below: */unsignedintflags;unsignedintptrace;
Add a rcu_usage to task_struct and use it to reuse the delayed rcu put
logic from release_task in finish_task_switch.
I didn't really read this patch yet, but it first glance it should work and
you can also remove the ->exit_state check/limitation in rcuwait_wait_event().
Oleg.
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:13:24
Handling of signals does not happen once do_exit is called as a
process never again exits from the kernel. So remove this ancient hold
over from something else.
Furthermore this is TIF_SIGPENDING on a zombie. Which will never
be scheduled on a cpu again.
Setting sigpending=0 was silly in 2.4 when exit_sighand did it and was
called from do_exit. When exit_sighand started being called from
release_task the code went from silly to completely pointless.
History tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/
Fixes: 6dfc88977e42 ("[PATCH] shared thread signals")
Signed-off-by: Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
kernel/exit.c | 1 -
1 file changed, 1 deletion(-)
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:14:42
When a task exits and uses do_notify_parent to send tsk->exit_signal
or SIGCHLD this has one of two possible meanings.
- The ptraced task has exited
- The thread group has exited
Linux resolves this ambiguity by preferring the thread group exit
interpretation if it is possible. As the exit of a thread group
containing a task is a superset of the exit of a task.
The code attempts to implement this in it's selection of signal
before calling do_notify_parent for a ptraced task. Unfortunately
it fails to properly handle the case when the thread_group is still
running (but it's leader has exited).
Fix this for a ptraced child leader by skipping the notification
instead of changing the exit signal. If we skip sending the signal in
exit_noitfy, when the last of the other threads are reaped
release_task will send the signal for us.
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
kernel/exit.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:14:48
The list a task is found on is not really relevant to wait, so stop passing
the list in and write the conditions in such a way as to not take the list
into consideration. This is technically user visible due to __WNOTHREAD
but is very unlikely to matter in practice.
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
kernel/exit.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
@@ -1356,11 +1355,13 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,*andreapingwillbecascadedtotherealparentwhenthe*ptracerdetaches.*/-if((exit_state==EXIT_TRACEE)&&ptrace)+if((exit_state==EXIT_TRACEE)&&same_thread_group(current,p->parent))returnwait_task_zombie(wo,exit_state,p);/* Is this task past the point where ptrace cares? */-if(unlikely((exit_state==EXIT_TRACED)&&ptrace))+if(unlikely((exit_state==EXIT_TRACED)&&+!(same_thread_group(current,p->real_parent)&&+thread_group_leader(p))))return0;/*
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:14:51
Rewrite the condition for what contitues a clone child. AKA a child
that reports to it's parent using something other than SIGCHLD.
If the parent has called exec since the child has started that child
will alwasy report to it's parent with SIGCHLD.
If the parent is only ptracing the child the child will always
report to the parent with SIGCHLD.
This implements the documented semantics and subsumes the fix Oleg
made in 4.7 to make __WCLONE unnecessary when ptracing a child. It was
just a bug in the check for __WCLONE support that made that necessary
the ``semantic'' change necessary.
Around v2.3.23 notify_parent was fixed to send SIGCHLD if the parent
had exec'd. Fixing half a bug but wait was not fixed to see children
in that case.
Not handling either the ptrace or the parent exec case appears to
go all of the way back to 1.0.
Fixes: bf959931ddb8 ("wait/ptrace: assume __WALL if the child is traced")
Fixes: v1.0
Fixes: v2.3.23
Signed-off-by: "Eric W. Biederman" <redacted>
---
kernel/exit.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:14:53
When ptracing waitpid(pid) has two possible meanings.
- Wait for the task with tid == pid to exit
- Wait for the thread group with tgid == pid to exit
Linux resolves this ambiguity by prefering the thread group
interpretation if it is possible. As the exit of a thread group
containing a task is the superset of the exit of a task.
Delaying reaping of thread group leaders for ptraced children
is how the code implements this resolution.
For ptraced tasks that are not also children this ambiguity does
not exit. The only possible valid meaning of waitpid(pid) is
to wait for the task with tid == pid to exit. As such there is
no need to delay reaping ptraced tasks when the tasks are
not also children of the tracer.
Change this carefully as the real parent does need to delay reaping
of these tasks.
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
kernel/exit.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -1144,7 +1144,8 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)/* If parent wants a zombie, don't release it now */state=EXIT_ZOMBIE;-if(do_notify_parent(p,p->exit_signal))+if(thread_group_empty(p)&&+do_notify_parent(p,p->exit_signal))state=EXIT_DEAD;p->exit_state=state;write_unlock_irq(&tasklist_lock);
@@ -1366,8 +1367,7 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,*ptracerdetaches.*/if((exit_state==EXIT_ZOMBIE)&&ptrace&&-(!thread_group_leader(p)||-(ptrace_reparented(p)&&thread_group_empty(p))))+(!thread_group_leader(p)||ptrace_reparented(p)))returnwait_task_zombie(wo,p);if(unlikely(exit_state==EXIT_TRACE)){
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:15:12
When two signals are generated make it the thread level signal that is
possibly surpessed. The reasoning is that the group level signal
always needs to be generated to report a group stop when that happens
and it is possibly doing double duty as both the group level and the
thread level signal. So if the thread level signal is identical to
the group level signal it does not need to be sent (or even attempted
to be sent).
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
kernel/signal.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
@@ -1813,6 +1813,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)__acquires(¤t->sighand->siglock){boolgstop_done=false;+boolidentical=false;if(arch_ptrace_stop_needed(exit_code,info)){/*
@@ -1852,8 +1853,16 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)*couldbeclearnow.WeactasifSIGCONTisreceivedafter*TASK_TRACEDisentered-ignoreit.*/-if(why==CLD_STOPPED&&(current->jobctl&JOBCTL_STOP_PENDING))-gstop_done=task_participate_group_stop(current);+if(why==CLD_STOPPED){+if(current->jobctl&JOBCTL_STOP_PENDING)+gstop_done=task_participate_group_stop(current);++/* Will the two generated signals be identical? */+identical=gstop_done&&+!(current->jobctl&JOBCTL_TRAP_NOTIFY)&&+!ptrace_reparented(current)&&+(task_pid(current)==task_tgid(current));+}/* any trap clears pending STOP trap, STOP trap clears NOTIFY */task_clear_jobctl_pending(current,JOBCTL_TRAP_STOP);
@@ -1874,10 +1883,11 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)*knowabouteverystopwhiletherealparentisonly*interestedinthecompletionofgroupstop.Thestates*forthetwodon'tinteractwitheachother.Notify-*separatelyunlessthey'regonnabeduplicates.+*separatelyunlesstheyaregoingtobeidentical.*/-do_notify_parent_cldstop(current,true,why);-if(gstop_done&&ptrace_reparented(current))+if(!identical)+do_notify_parent_cldstop(current,true,why);+if(gstop_done)do_notify_parent_cldstop(current,false,why);/*
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:16:04
Now that which list a task is found on does not matter there is no
reason to walk the child lists for waitpid when task_pid can directly
find the child.
Add a new helper do_wait_pid that finds the target task via pid_task
and verifies it is on one of the lists for the thread we are
reaping.
This is more efficient in two ways. It skips the list traversal
so uninteresting tasks don't slow things down. It guarantees
a task will only be visited once if p->parent == p->real_parent.
Except for the increase in efficiency this results in no user
visible behavioral differences.
Signed-off-by: "Eric W. Biederman" <redacted>
---
kernel/exit.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
@@ -1438,6 +1438,18 @@ static int ptrace_do_wait(struct wait_opts *wo, struct task_struct *tsk)return0;}+staticintdo_wait_pid(structwait_opts*wo,structtask_struct*tsk)+{+structtask_struct*p=pid_task(wo->wo_pid,wo->wo_type);++/* Not on one of this tasks child lists? */+if((tsk!=p->parent)&&+((tsk!=p->real_parent)||!thread_group_leader(p)))+return0;++returnwait_consider_task(wo,p);+}+staticintchild_wait_callback(wait_queue_t*wait,unsignedmode,intsync,void*key){
@@ -1486,11 +1498,15 @@ static long do_wait(struct wait_opts *wo)read_lock(&tasklist_lock);tsk=current;do{-retval=do_wait_thread(wo,tsk);-if(retval)-gotoend;+if(wo->wo_type==PIDTYPE_PID){+retval=do_wait_pid(wo,tsk);+}else{+retval=do_wait_thread(wo,tsk);+if(retval)+gotoend;-retval=ptrace_do_wait(wo,tsk);+retval=ptrace_do_wait(wo,tsk);+}if(retval)gotoend;
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:16:23
Start with reaping pending zombies and then return if there is nothing
wait_task_consider can ever do with the task.
What remains are living tasks and delayed child zombie thread group
leaders waiting for their thread group to exit. Leaving something for
WEXITED to find later.
As long as WEXITED can happen the code is guaranteed not to block
indefinitely, as at least the exit event will wake it up. The only
reason not to clear notask_error is if it is impossible for wait
to find something to report. For zombie thread group leaders it
is possible that living threads will report group wide continued
or stopped states.
Which means we can now safely and practically always clear notask_error.
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
kernel/exit.c | 62 ++++++++++++++++++++++-------------------------------------
1 file changed, 23 insertions(+), 39 deletions(-)
@@ -1359,47 +1359,31 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,if((exit_state==EXIT_TRACEE)&&ptrace)returnwait_task_zombie(wo,exit_state,p);-if(unlikely(exit_state==EXIT_TRACED)){-/*-*ptrace==0meanswearethenaturalparent.Inthiscase-*weshouldclearnotask_error,debuggerwillnotifyus.-*/-if(likely(!ptrace))-wo->notask_error=0;+/* Is this task past the point where ptrace cares? */+if(unlikely((exit_state==EXIT_TRACED)&&ptrace))return0;-}-if(exit_state==EXIT_ZOMBIE){-/*-*Allowaccesstostopped/continuedstateviazombieby-*fallingthrough.Clearingofnotask_erroriscomplex.-*-*When!@ptrace:-*-*IfWEXITEDisset,notask_errorshouldnaturallybe-*cleared.Ifnot,subsetofWSTOPPED|WCONTINUEDisset,-*so,iftherearelivesubthreads,thereareeventsto-*waitfor.Ifallsubthreadsaredead,it'sstillsafe-*toclear-thisfunctionwillbecalledagaininfinite-*amounttimeonceallthesubthreadsarereleasedand-*willthenreturnwithoutclearing.-*-*When@ptrace:-*-*Stoppedstateisper-taskandthuscan'tchangeoncethe-*targettaskdies.Onlycontinuedandexitedcanhappen.-*Clearnotask_errorifWCONTINUED|WEXITED.-*/-if((!ptrace&&(!p->ptrace||ptrace_reparented(p)))||-(wo->wo_flags&(WCONTINUED|WEXITED)))-wo->notask_error=0;-}else{-/*-*@pisaliveandit'sgonnastop,continueorexit,so-*therealwaysissomethingtowaitfor.-*/-wo->notask_error=0;-}+/*+*Athispoint@pisaliveorthezombieofadelayed+*childthreadgroupleaderthathasnotbeenreapedyet.+*+*Allowaccesstostopped/continuedstateviazombieby+*fallingthrough.Clearingofnotask_errorislogicallycomplex.+*+*When@pisaliveandit'sgonnastop,continueorexit,+*sotherealwaysissomethingtowaitfor.+*+*When@pisazombie+*+*IfWEXITEDisset,notask_errorshouldnaturallybe+*cleared.Ifnot,subsetofWSTOPPED|WCONTINUEDisset,+*so,iftherearelivesubthreads,thereareeventsto+*waitfor.Ifallsubthreadsaredead,it'sstillsafe+*toclear-thisfunctionwillbecalledagaininfinite+*amounttimeonceallthesubthreadsarereleasedand+*willthenreturnwithoutclearing.+*/+wo->notask_error=0;/**Waitforstopped.
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:16:49
In November of 2005 Oleg fixed a kernel crash with commit 7ed0175a462c
("[PATCH] Don't auto-reap traced children"). Oleg's patch was the fix
for CVE-2005-3784 where one description says:
The auto-reap of child processes in Linux kernel 2.6 before 2.6.15
includes processes with ptrace attached, which leads to a dangling
ptrace reference and allows local users to cause a denial of
service (crash) and gain root privileges.
Not reaping the zombies resulted in zombies on the ptrace list when
threads that ignored them exited. Resulting in Roland authoring
666f164f4fbf ("fix dangling zombie when new parent ignores children").
Which winds up auto-waiting for those zombies not when the tasks exit
and become zombies but when the ptracer exits.
As the kernel is already auto-waiting zombies for ptraced children
rewrite the code to use the same code paths for auto-waiting as we use
for all other children.
This is a user visible change so something might care but as auto-wait
at exit semantics are not documented anywhere, are in direct violation
of what SIG_IGN and SA_NOCLDWAIT are documented by posix to do, and
added to avoid a kernel crash, I don't expect there will be problems.
Fixes: 7ed0175a462c ("[PATCH] Don't auto-reap traced children")
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
kernel/exit.c | 10 +++++++---
kernel/ptrace.c | 23 +----------------------
kernel/signal.c | 2 +-
3 files changed, 9 insertions(+), 26 deletions(-)
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:16:51
When ptracing waitpid(pid, WUNTRACED) has two possible meanings.
- Wait for ptrace stops from the task with tid == pid
- Wait for ordinary stops from the process with tgid == pid
The only sensible behavior and the Linux behavior in 2.2 and
2.4 has been to consume both ptrace stops and group stops
in this case. It looks like when Oleg disentangled thread
stops and group stops in 2.6.30 fixing a lot of other issues
the case when we want to reap both was overlooked.
Consume both the group and the ptrace stop state when
waitpid(pid, WUNTRACED) could be asking for both, and
the wait status for both is idenitical. This keeps
us from double reporting the stop and causing confusion.
This is very slight user visible change and is only visible
in the unlikely case a ptracer specifies WUNTRACED aka
WSTOPPED.
Write this code in such a way that it doesn't matter which
list we are traversing when we find a child whose stop states
we care about.
Fixes: 90bc8d8b1a38 ("do_wait: fix waiting for the group stop with the dead leader")
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
kernel/exit.c | 82 +++++++++++++++++++++++++++++------------------------------
1 file changed, 40 insertions(+), 42 deletions(-)
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:16:54
With the freeing and slaying of zombies moved earlier in wait_consider_task
changing of the ptrace value only effects the clearing of notask_error and
wait_task_stopped. Move the changing of ptrace into wait_task_stopped.
The value of ptrace coming into the code clearing notask_error is left
at it's original value.
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
kernel/exit.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:18:13
Today initiating a group stop with SIGSTOP, SIGTSTP, SIGTTIN, or
SIGTTOU and then have it contined with SIGCONT before the group stop
is complete results in SIGCHLD being sent with a si_status of 0. The
field si_status is defined by posix to be the stop signal. Which means
we wind up violating posix and any reasonable meaning of siginfo
delivered with SIGCHLD by reporting this partial group stop.
A partial group stop will result in even stranger things when seen in
the context of ptrace. If all of the threads are ptraced they should
all enter a ptrace stop but the cancelation of the group stop before
the group stop completes results in only some of the threads entering
a ptrace stop.
Fix this by performing a delayed thread group wakeup that will take
effect as the last thread is signaling that the group stop is
complete.
This makes reasoning about the code much simpler, fixes partial
group stop interactions with ptrace, and the signal set for
a group stop that sees SIGCONT before it completes. For
a similar cost in code.
I looked into the history to see if I could understand where this
problematic behavior came from, and the source of the behavior goes
back to the original development of thread groups in the kernel. A
bug fix was made to improve the handling of SIGCONT that introduced
group_stop_count and the resuming of partial stops that makes no sense
today in the context of ptrace SIGSTOP handling.
At the time that was not a problem because stops when being ptraced
were in ordinary TASK_STOPPED stops. It potentially became a problem
shortly thereafter when ptrace stops became TASK_TRACED stops which
can not be gotten out of with SIGCONT. Howeve for it to actually
become a problem the code had to wait until recently when 5224fa3660ad
("ptrace: Make do_signal_stop() use ptrace_stop() if the task is being ptraced")
was merged for ptraced processes to stop with in TASK_TRACED when
they were ptraced.
The practical issue with sending an invalid si_status appears to have
been introduced much later after the locking changed to make it
necessary to send signals to the parent from the destination process
itself. When Oleg unified partial and full stop handling
group_exit_code wound up being cleared on both code paths. Not just
the for the full stop case. That in turn introduced the invalid
si_status of 0. As until that point group_exit_code always held the
signal when do_notify_parent_cldstop was called for stop signals.
Historical tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
Fixes: 5224fa3660ad ("ptrace: Make do_signal_stop() use ptrace_stop() if the task is being ptraced")
Fixes: fc321d2e60d6 ("handle_stop_signal: unify partial/full stop handling")
Reference: e442055193e4 ("signals: re-assign CLD_CONTINUED notification from the sender to reciever")
Reference: cece79ae3a39 ("[PATCH] cleanup ptrace stops and remove notify_parent")
Reference: ca3f74aa7baa ("[PATCH] waitid system call")
Reference: ebf5ebe31d2c ("[PATCH] signal-fixes-2.5.59-A4")
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
include/linux/sched/signal.h | 26 +++++++------
kernel/signal.c | 89 ++++++++++++++++++++++++--------------------
2 files changed, 63 insertions(+), 52 deletions(-)
@@ -817,7 +826,13 @@ static bool prepare_signal(int sig, struct task_struct *p, bool force)for_each_thread(p,t)flush_sigqueue_mask(&flush,&t->pending);}elseif(sig==SIGCONT){-unsignedintwhy;+if(signal->group_stop_count){+/* Let the stop complete before continuing. This+*ensurestherearewelldefininedinteractionswith+*ptrace.+*/+signal->flags|=SIGNAL_WAKEUP_PENDING;+}/**Removeallstopsignalsfromallqueues,wakeallthreads.*/
@@ -825,35 +840,21 @@ static bool prepare_signal(int sig, struct task_struct *p, bool force)flush_sigqueue_mask(&flush,&signal->shared_pending);for_each_thread(p,t){flush_sigqueue_mask(&flush,&t->pending);+if(signal->flags&SIGNAL_WAKEUP_PENDING)+continue;task_clear_jobctl_pending(t,JOBCTL_STOP_PENDING);-if(likely(!(t->ptrace&PT_SEIZED)))-wake_up_state(t,__TASK_STOPPED);-else-ptrace_trap_notify(t);+wake_up_stopped_thread(t);}-/*-*NotifytheparentwithCLD_CONTINUEDifwewerestopped.-*-*Ifwewereinthemiddleofagroupstop,wepretendit-*wasalreadyfinished,andthencontinued.SinceSIGCHLD-*doesn'tqueuewereportonlyCLD_STOPPED,asifthenext-*CLD_CONTINUEDwasdropped.-*/-why=0;-if(signal->flags&SIGNAL_STOP_STOPPED)-why|=SIGNAL_CLD_CONTINUED;-elseif(signal->group_stop_count)-why|=SIGNAL_CLD_STOPPED;--if(why){+/* Notify the parent with CLD_CONTINUED if we were stopped. */+if(signal->flags&SIGNAL_STOP_STOPPED){/**Thefirstthreadwhichreturnsfromdo_signal_stop()-*willtake->siglock,noticeSIGNAL_CLD_MASK,and-*notifyitsparent.Seeget_signal_to_deliver().+*willtake->siglock,noticeSIGNAL_CLD_CONTINUED,and+*notifyitsparent.Seeget_signal().*/-signal_set_stop_flags(signal,why|SIGNAL_STOP_CONTINUED);-signal->group_stop_count=0;+signal_set_stop_flags(signal,+SIGNAL_STOP_CONTINUED|SIGNAL_CLD_CONTINUED);signal->group_exit_code=0;}}
@@ -1691,6 +1692,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig)staticvoiddo_notify_parent_cldstop(structtask_struct*tsk,boolfor_ptracer,intwhy){+structsignal_struct*sig=tsk->signal;structsiginfoinfo;unsignedlongflags;structtask_struct*parent;
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:18:15
Create two new exit states EXIT_TRACEE and EXIT_TRACED replacing
the two states "(EXIT_ZOMBIE && (!thread_group_leader(p) || !ptrace_reparented))
and EXIT_TRACE. With EXIT_ZOMBIE replacing the state:
"(EXIT_ZOMBIE && thread_group_leader(p) && !ptrace_reparented)".
Rework the code to take advantage of the certain knowledge of
exit state progression:
EXIT_TRACEE -> EXIT_TRACED -> EXIT_ZOMBIE -> EXIT_DEAD
This makes the code more readable/maintainable by using simple states
rather than complicated expressions. The values of both of the new
states contain EXIT_ZOMBIE so all of these states appear to userspace
as zombies.
Signed-off-by: "Eric W. Biederman" <redacted>
---
include/linux/sched.h | 6 +++++-
kernel/exit.c | 51 +++++++++++++++++++++++----------------------------
kernel/ptrace.c | 31 +++++++++++++++++--------------
3 files changed, 45 insertions(+), 43 deletions(-)
@@ -580,8 +580,7 @@ static void reparent_leader(struct task_struct *father, struct task_struct *p,p->exit_signal=SIGCHLD;/* If it has exited notify the new parent about this child's death. */-if(!p->ptrace&&-p->exit_state==EXIT_ZOMBIE&&thread_group_empty(p)){+if(p->exit_state==EXIT_ZOMBIE&&thread_group_empty(p)){if(do_notify_parent(p,p->exit_signal)){p->exit_state=EXIT_DEAD;list_add(&p->ptrace_entry,dead);
@@ -1132,7 +1132,7 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)if(!retval)retval=pid;-if(state==EXIT_TRACE){+if(state==EXIT_TRACED){write_lock_irq(&tasklist_lock);/* We dropped tasklist, ptracer could die and untrace */ptrace_unlink(p);
@@ -1335,8 +1335,7 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,{/**Wecanracewithwait_task_zombie()fromanotherthread.-*EnsurethatEXIT_ZOMBIE->EXIT_DEAD/EXIT_TRACEtransition-*can'tconfusethechecksbelow.+*Ensurethatexit_statetransitioncan'tconfusethechecksbelow.*/intexit_state=ACCESS_ONCE(p->exit_state);intret;
@@ -1349,11 +1348,8 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,returnret;/* zombie child process? */-if((exit_state==EXIT_ZOMBIE)&&-!ptrace_reparented(p)&&-thread_group_leader(p)&&-thread_group_empty(p))-returnwait_task_zombie(wo,p);+if((exit_state==EXIT_ZOMBIE)&&thread_group_empty(p))+returnwait_task_zombie(wo,exit_state,p);/**Azombieptraceethatisnotachildofit'sptracer's
@@ -1361,11 +1357,10 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,*andreapingwillbecascadedtotherealparentwhenthe*ptracerdetaches.*/-if((exit_state==EXIT_ZOMBIE)&&ptrace&&-(!thread_group_leader(p)||ptrace_reparented(p)))-returnwait_task_zombie(wo,p);+if((exit_state==EXIT_TRACEE)&&ptrace)+returnwait_task_zombie(wo,exit_state,p);-if(unlikely(exit_state==EXIT_TRACE)){+if(unlikely(exit_state==EXIT_TRACED)){/**ptrace==0meanswearethenaturalparent.Inthiscase*weshouldclearnotask_error,debuggerwillnotifyus.
@@ -497,27 +497,30 @@ static int ignoring_children(struct sighand_struct *sigh)*/staticbool__exit_ptrace(structtask_struct*tracer,structtask_struct*p){-booldead;+intstate=p->exit_state;__ptrace_unlink(p);-if(p->exit_state!=EXIT_ZOMBIE)-returnfalse;--dead=!thread_group_leader(p);--if(!dead&&thread_group_empty(p)){-if(!same_thread_group(p->real_parent,tracer))-dead=do_notify_parent(p,p->exit_signal);-elseif(ignoring_children(tracer->sighand)){+if(state==EXIT_ZOMBIE){+/* Honor the parents request to autoreap children */+if(thread_group_empty(p)&&+ignoring_children(tracer->sighand)){+state=EXIT_DEAD;__wake_up_parent(p,tracer);-dead=true;+}+}+elseif(state==EXIT_TRACEE){+state=EXIT_DEAD;+if(thread_group_leader(p)){+state=EXIT_ZOMBIE;+if(thread_group_empty(p)&&+do_notify_parent(p,p->exit_signal))+state=EXIT_DEAD;}}/* Mark it as in the process of being reaped. */-if(dead)-p->exit_state=EXIT_DEAD;-returndead;+p->exit_state=state;+returnstate==EXIT_DEAD;}staticintptrace_detach(structtask_struct*child,unsignedintdata)
@@ -1342,6 +1342,24 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,if(!ret)returnret;+/* zombie child process? */+if((exit_state==EXIT_ZOMBIE)&&+!ptrace_reparented(p)&&+thread_group_leader(p)&&+thread_group_empty(p))+returnwait_task_zombie(wo,p);++/*+*Azombieptraceethatisnotachildofit'sptracer's+*threadgroupisonlyvisibletoitsptracer.Notification+*andreapingwillbecascadedtotherealparentwhenthe+*ptracerdetaches.+*/+if((exit_state==EXIT_ZOMBIE)&&ptrace&&+(!thread_group_leader(p)||+(ptrace_reparented(p)&&thread_group_empty(p))))+returnwait_task_zombie(wo,p);+if(unlikely(exit_state==EXIT_TRACE)){/**ptrace==0meanswearethenaturalparent.Inthiscase
@@ -1354,33 +1372,17 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,if(likely(!ptrace)&&unlikely(p->ptrace)){/*-*Ifitistracedbyitsrealparent'sgroup,justpretend-*thecallerisptrace_do_wait()andreapthischildifit-*iszombie.-*-*Thisalsohidesgroupstopstatefromrealparent;otherwise-*asinglestopcanbereportedtwiceasgroupandptracestop.-*Ifaptracerwantstodistinguishthesetwoeventsforits-*ownchildrenitshouldcreateaseparateprocesswhichtakes-*theroleofrealparent.+*Hidegroupstopstatefromrealparent;otherwiseasingle+*stopcanbereportedtwiceasgroupandptracestop.Ifa+*ptracerwantstodistinguishthesetwoeventsforitsown+*childrenitshouldcreateaseparateprocesswhichtakesthe+*roleofrealparent.*/if(!ptrace_reparented(p))ptrace=1;}-/* slay zombie? */if(exit_state==EXIT_ZOMBIE){-/* we don't reap group leaders with subthreads */-if(!delay_group_leader(p)){-/*-*Azombieptraceeisonlyvisibletoitsptracer.-*Notificationandreapingwillbecascadedtothe-*realparentwhentheptracerdetaches.-*/-if(unlikely(ptrace)||likely(!p->ptrace))-returnwait_task_zombie(wo,p);-}-/**Allowaccesstostopped/continuedstateviazombieby*fallingthrough.Clearingofnotask_erroriscomplex.
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:19:44
The function zap_pid_processes terminates when the number of pids used
by processes in a pid namespace drops to just those pids used by the
last thread of the dying thread.
Don't allow an init process aka a child_reaper to call
setpgid(0, some_other_processes_pid). That case is already broken today
as it would result in a pid namespace that will hang when the thread group
leader dies. Thankfully I have not received that bug report so it
appears that no one cares and uses that case.
Limiting setpgid ensures that the only two pids in the pid namespace
on the init process that are worth worrying about are the pid and the
tgid. The pgrp will now either match the tgid or it will be from
outside the pid namespace. Likewise the sid will either match the
tgid or be from outside the pid namespace.
To make it clear what is being counted test if the task's tgid is the same
as the the task's pid. In particular the code does not count the
number of processes in a pid namespace, just the number of pids those
processes use. A subtle but important distinction for understanding
the code.
Signed-off-by: "Eric W. Biederman" <redacted>
---
kernel/pid_namespace.c | 2 +-
kernel/sys.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
@@ -208,7 +208,7 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)intnr;intrc;structtask_struct*task,*me=current;-intinit_pids=thread_group_leader(me)?1:2;+intinit_pids=task_pid(me)!=task_tgid(me)?2:1;/* Don't allow any more processes into the pid namespace */disable_pid_allocation(pid_ns);
From: Eric W. Biederman <hidden> Date: 2017-06-06 19:19:49
If the only job of the signal is to report a ptrace level event set
si_code to CLD_TRAPPED instead of possibly CLD_STOPPED.
This causes the siginfo of the signals that are sent to match the
signinfo of the signals returned by waitid.
This is a user visible difference but I don't expect anything will
care.
In fact this is a return to historical linux behavior. In linux 2.4.0
all ptrace stops were reported through do_notify_parent with
CLD_TRAPPED. When do_notify_parent_cldstop was added the CLD_TRAPPED
logic was not included and CLD_TRAPPED for ptrace stops was lost. As
nothing was said about this case I assume it was an oversight.
When waitid was added a little earlier all stops were being
reported with do_notify_parent and all ptrace stops were setting
CLD_TRAPPED. So initially signals and waitid were in sync with
respect to setting CLD_TRAPPED.
It is also worth knowing that posix uses documents CLD_TRAPPED
as "Traced child has trapped."
History Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
Ref: ca3f74aa7baa ("[PATCH] waitid system call")
Fixes: Fixes: ebf5ebe31d2c ("[PATCH] signal-fixes-2.5.59-A4")
Signed-off-by: "Eric W. Biederman" <redacted>
---
kernel/signal.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -1886,9 +1886,9 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)*separatelyunlesstheyaregoingtobeidentical.*/if(!identical)-do_notify_parent_cldstop(current,true,why);+do_notify_parent_cldstop(current,true,CLD_TRAPPED);if(gstop_done)-do_notify_parent_cldstop(current,false,why);+do_notify_parent_cldstop(current,false,CLD_STOPPED);/**Don'twanttoallowpreemptionhere,because
@@ -1912,7 +1912,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)*therealparentofthegroupstopcompletionisenough.*/if(gstop_done)-do_notify_parent_cldstop(current,false,why);+do_notify_parent_cldstop(current,false,CLD_STOPPED);/* tasklist protects us from ptrace_freeze_traced() */__set_current_state(TASK_RUNNING);
Another easy entry point is to see that a multi-threaded setuid won't
change the credentials on a zombie thread group leader. Which can allow
sending signals to a process that the credential change should forbid.
This is in violation of posix and the semantics we attempt to enforce in
linux.
I might be completely wrong on this point (and I haven't looked at the
patches), but I was under the impression that multi-threaded set[ug]id
was implemented in userspace (by glibc's nptl(7) library that uses RT
signals internally to get each thread to update their credentials). And
given that, I wouldn't be surprised (as a user) that zombie threads will
have stale credentials (glibc isn't running in those threads anymore).
Am I mistaken in that belief?
</off-topic>
--
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
On Tue, Jun 6, 2017 at 12:01 PM, Eric W. Biederman
[off-list ref] wrote:
I am posting this patches in the hope of some review of the strategy I
am taking and to let the individual patches be reviewed.
I'm trying to look through these, and finding (as usual) that the
signal handling and exit code is extremely scary from a correctness
and security standpoint.
I really want Oleg to review/ack these. Oleg?
I also would really really want to see the stuff that actually changes
semantics split out.
For example, I feel much less nervous about things like making the
tasklist RCU-safe. So I'd like to see changes like that be separated
out from the much scarier ones. Would that be possible? Hint hint..
Linus