From 7d613f9f72ec8f90ddefcae038fdae5adb8404b3 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Wed, 1 Sep 2021 13:21:34 -0500 Subject: signal: Remove the bogus sigkill_pending in ptrace_stop The existence of sigkill_pending is a little silly as it is functionally a duplicate of fatal_signal_pending that is used in exactly one place. Checking for pending fatal signals and returning early in ptrace_stop is actively harmful. It casues the ptrace_stop called by ptrace_signal to return early before setting current->exit_code. Later when ptrace_signal reads the signal number from current->exit_code is undefined, making it unpredictable what will happen. Instead rely on the fact that schedule will not sleep if there is a pending signal that can awaken a task. Removing the explict sigkill_pending test fixes fixes ptrace_signal when ptrace_stop does not stop because current->exit_code is always set to to signr. Cc: stable@vger.kernel.org Fixes: 3d749b9e676b ("ptrace: simplify ptrace_stop()->sigkill_pending() path") Fixes: 1a669c2f16d4 ("Add arch_ptrace_stop") Link: https://lkml.kernel.org/r/87pmsyx29t.fsf@disp2133 Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- kernel/signal.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) (limited to 'kernel/signal.c') diff --git a/kernel/signal.c b/kernel/signal.c index 952741f6d0f9..9f2dc9cf3208 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2182,15 +2182,6 @@ static inline bool may_ptrace_stop(void) return true; } -/* - * Return non-zero if there is a SIGKILL that should be waking us up. - * Called with the siglock held. - */ -static bool sigkill_pending(struct task_struct *tsk) -{ - return sigismember(&tsk->pending.signal, SIGKILL) || - sigismember(&tsk->signal->shared_pending.signal, SIGKILL); -} /* * This must be called with current->sighand->siglock held. @@ -2217,17 +2208,16 @@ static void ptrace_stop(int exit_code, int why, int clear_code, kernel_siginfo_t * calling arch_ptrace_stop, so we must release it now. * To preserve proper semantics, we must do this before * any signal bookkeeping like checking group_stop_count. - * Meanwhile, a SIGKILL could come in before we retake the - * siglock. That must prevent us from sleeping in TASK_TRACED. - * So after regaining the lock, we must check for SIGKILL. */ spin_unlock_irq(¤t->sighand->siglock); arch_ptrace_stop(exit_code, info); spin_lock_irq(¤t->sighand->siglock); - if (sigkill_pending(current)) - return; } + /* + * schedule() will not sleep if there is a pending signal that + * can awaken the task. + */ set_special_state(TASK_TRACED); /* -- cgit v1.2.3 From 4f627af8e6068892cafe031df6c14e8a0aaaa426 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Thu, 2 Sep 2021 16:10:11 -0500 Subject: ptrace: Remove the unnecessary arguments from arch_ptrace_stop Both arch_ptrace_stop_needed and arch_ptrace_stop are called with an exit_code and a siginfo structure. Neither argument is used by any of the implementations so just remove the unneeded arguments. The two arechitectures that implement arch_ptrace_stop are ia64 and sparc. Both architectures flush their register stacks before a ptrace_stack so that all of the register information can be accessed by debuggers. As the question of if a register stack needs to be flushed is independent of why ptrace is stopping not needing arguments make sense. Cc: David Miller Cc: sparclinux@vger.kernel.org Link: https://lkml.kernel.org/r/87lf3mx290.fsf@disp2133 Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- arch/ia64/include/asm/ptrace.h | 4 ++-- arch/sparc/include/asm/ptrace.h | 8 ++++---- include/linux/ptrace.h | 22 +++++++++------------- kernel/signal.c | 4 ++-- 4 files changed, 17 insertions(+), 21 deletions(-) (limited to 'kernel/signal.c') diff --git a/arch/ia64/include/asm/ptrace.h b/arch/ia64/include/asm/ptrace.h index 08179135905c..f15504f75f10 100644 --- a/arch/ia64/include/asm/ptrace.h +++ b/arch/ia64/include/asm/ptrace.h @@ -129,9 +129,9 @@ static inline long regs_return_value(struct pt_regs *regs) extern void ia64_decrement_ip (struct pt_regs *pt); extern void ia64_ptrace_stop(void); - #define arch_ptrace_stop(code, info) \ + #define arch_ptrace_stop() \ ia64_ptrace_stop() - #define arch_ptrace_stop_needed(code, info) \ + #define arch_ptrace_stop_needed() \ (!test_thread_flag(TIF_RESTORE_RSE)) extern void ptrace_attach_sync_user_rbs (struct task_struct *); diff --git a/arch/sparc/include/asm/ptrace.h b/arch/sparc/include/asm/ptrace.h index 71dd82b43cc5..d1419e669027 100644 --- a/arch/sparc/include/asm/ptrace.h +++ b/arch/sparc/include/asm/ptrace.h @@ -26,12 +26,12 @@ static inline bool pt_regs_clear_syscall(struct pt_regs *regs) return (regs->tstate &= ~TSTATE_SYSCALL); } -#define arch_ptrace_stop_needed(exit_code, info) \ +#define arch_ptrace_stop_needed() \ ({ flush_user_windows(); \ get_thread_wsaved() != 0; \ }) -#define arch_ptrace_stop(exit_code, info) \ +#define arch_ptrace_stop() \ synchronize_user_stack() #define current_pt_regs() \ @@ -129,12 +129,12 @@ static inline bool pt_regs_clear_syscall(struct pt_regs *regs) return (regs->psr &= ~PSR_SYSCALL); } -#define arch_ptrace_stop_needed(exit_code, info) \ +#define arch_ptrace_stop_needed() \ ({ flush_user_windows(); \ current_thread_info()->w_saved != 0; \ }) -#define arch_ptrace_stop(exit_code, info) \ +#define arch_ptrace_stop() \ synchronize_user_stack() #define current_pt_regs() \ diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index b5ebf6c01292..8aee2945ff08 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -362,29 +362,25 @@ static inline void user_single_step_report(struct pt_regs *regs) #ifndef arch_ptrace_stop_needed /** * arch_ptrace_stop_needed - Decide whether arch_ptrace_stop() should be called - * @code: current->exit_code value ptrace will stop with - * @info: siginfo_t pointer (or %NULL) for signal ptrace will stop with * * This is called with the siglock held, to decide whether or not it's - * necessary to release the siglock and call arch_ptrace_stop() with the - * same @code and @info arguments. It can be defined to a constant if - * arch_ptrace_stop() is never required, or always is. On machines where - * this makes sense, it should be defined to a quick test to optimize out - * calling arch_ptrace_stop() when it would be superfluous. For example, - * if the thread has not been back to user mode since the last stop, the - * thread state might indicate that nothing needs to be done. + * necessary to release the siglock and call arch_ptrace_stop(). It can be + * defined to a constant if arch_ptrace_stop() is never required, or always + * is. On machines where this makes sense, it should be defined to a quick + * test to optimize out calling arch_ptrace_stop() when it would be + * superfluous. For example, if the thread has not been back to user mode + * since the last stop, the thread state might indicate that nothing needs + * to be done. * * This is guaranteed to be invoked once before a task stops for ptrace and * may include arch-specific operations necessary prior to a ptrace stop. */ -#define arch_ptrace_stop_needed(code, info) (0) +#define arch_ptrace_stop_needed() (0) #endif #ifndef arch_ptrace_stop /** * arch_ptrace_stop - Do machine-specific work before stopping for ptrace - * @code: current->exit_code value ptrace will stop with - * @info: siginfo_t pointer (or %NULL) for signal ptrace will stop with * * This is called with no locks held when arch_ptrace_stop_needed() has * just returned nonzero. It is allowed to block, e.g. for user memory @@ -394,7 +390,7 @@ static inline void user_single_step_report(struct pt_regs *regs) * we only do it when the arch requires it for this particular stop, as * indicated by arch_ptrace_stop_needed(). */ -#define arch_ptrace_stop(code, info) do { } while (0) +#define arch_ptrace_stop() do { } while (0) #endif #ifndef current_pt_regs diff --git a/kernel/signal.c b/kernel/signal.c index 9f2dc9cf3208..c9759ff2cb43 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2200,7 +2200,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, kernel_siginfo_t { bool gstop_done = false; - if (arch_ptrace_stop_needed(exit_code, info)) { + if (arch_ptrace_stop_needed()) { /* * The arch code has something special to do before a * ptrace stop. This is allowed to block, e.g. for faults @@ -2210,7 +2210,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, kernel_siginfo_t * any signal bookkeeping like checking group_stop_count. */ spin_unlock_irq(¤t->sighand->siglock); - arch_ptrace_stop(exit_code, info); + arch_ptrace_stop(); spin_lock_irq(¤t->sighand->siglock); } -- cgit v1.2.3 From 92307383082daff5df884a25df9e283efb7ef261 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Wed, 1 Sep 2021 11:33:50 -0500 Subject: coredump: Don't perform any cleanups before dumping core Rename coredump_exit_mm to coredump_task_exit and call it from do_exit before PTRACE_EVENT_EXIT, and before any cleanup work for a task happens. This ensures that an accurate copy of the process can be captured in the coredump as no cleanup for the process happens before the coredump completes. This also ensures that PTRACE_EVENT_EXIT will not be visited by any thread until the coredump is complete. Add a new flag PF_POSTCOREDUMP so that tasks that have passed through coredump_task_exit can be recognized and ignored in zap_process. Now that all of the coredumping happens before exit_mm remove code to test for a coredump in progress from mm_release. Replace "may_ptrace_stop()" with a simple test of "current->ptrace". The other tests in may_ptrace_stop all concern avoiding stopping during a coredump. These tests are no longer necessary as it is now guaranteed that fatal_signal_pending will be set if the code enters ptrace_stop during a coredump. The code in ptrace_stop is guaranteed not to stop if fatal_signal_pending returns true. Until this change "ptrace_event(PTRACE_EVENT_EXIT)" could call ptrace_stop without fatal_signal_pending being true, as signals are dequeued in get_signal before calling do_exit. This is no longer an issue as "ptrace_event(PTRACE_EVENT_EXIT)" is no longer reached until after the coredump completes. Link: https://lkml.kernel.org/r/874kaax26c.fsf@disp2133 Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/coredump.c | 8 ++++---- include/linux/sched.h | 1 + kernel/exit.c | 19 ++++++++++++------- kernel/fork.c | 3 +-- kernel/signal.c | 27 +-------------------------- mm/oom_kill.c | 2 +- 6 files changed, 20 insertions(+), 40 deletions(-) (limited to 'kernel/signal.c') diff --git a/fs/coredump.c b/fs/coredump.c index 5e0e08a7fb9b..d576287fb88b 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -359,7 +359,7 @@ static int zap_process(struct task_struct *start, int exit_code, int flags) for_each_thread(start, t) { task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK); - if (t != current && t->mm) { + if (t != current && !(t->flags & PF_POSTCOREDUMP)) { sigaddset(&t->pending.signal, SIGKILL); signal_wake_up(t, 1); nr++; @@ -404,8 +404,8 @@ static int zap_threads(struct task_struct *tsk, struct mm_struct *mm, * * do_exit: * The caller holds mm->mmap_lock. This means that the task which - * uses this mm can't pass coredump_exit_mm(), so it can't exit or - * clear its ->mm. + * uses this mm can't pass coredump_task_exit(), so it can't exit + * or clear its ->mm. * * de_thread: * It does list_replace_rcu(&leader->tasks, ¤t->tasks), @@ -500,7 +500,7 @@ static void coredump_finish(struct mm_struct *mm, bool core_dumped) next = curr->next; task = curr->task; /* - * see coredump_exit_mm(), curr->task must not see + * see coredump_task_exit(), curr->task must not see * ->task == NULL before we read ->next. */ smp_mb(); diff --git a/include/linux/sched.h b/include/linux/sched.h index e12b524426b0..f3741f23935e 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1664,6 +1664,7 @@ extern struct pid *cad_pid; #define PF_VCPU 0x00000001 /* I'm a virtual CPU */ #define PF_IDLE 0x00000002 /* I am an IDLE thread */ #define PF_EXITING 0x00000004 /* Getting shut down */ +#define PF_POSTCOREDUMP 0x00000008 /* Coredumps should ignore this task */ #define PF_IO_WORKER 0x00000010 /* Task is an IO worker */ #define PF_WQ_WORKER 0x00000020 /* I'm a workqueue worker */ #define PF_FORKNOEXEC 0x00000040 /* Forked but didn't exec */ diff --git a/kernel/exit.c b/kernel/exit.c index cb1619d8fd64..774e6b5061b8 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -339,23 +339,29 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent) } } -static void coredump_exit_mm(struct mm_struct *mm) +static void coredump_task_exit(struct task_struct *tsk) { struct core_state *core_state; + struct mm_struct *mm; + + mm = tsk->mm; + if (!mm) + return; /* * Serialize with any possible pending coredump. * We must hold mmap_lock around checking core_state - * and clearing tsk->mm. The core-inducing thread + * and setting PF_POSTCOREDUMP. The core-inducing thread * will increment ->nr_threads for each thread in the - * group with ->mm != NULL. + * group without PF_POSTCOREDUMP set. */ + mmap_read_lock(mm); + tsk->flags |= PF_POSTCOREDUMP; core_state = mm->core_state; + mmap_read_unlock(mm); if (core_state) { struct core_thread self; - mmap_read_unlock(mm); - self.task = current; if (self.task->flags & PF_SIGNALED) self.next = xchg(&core_state->dumper.next, &self); @@ -375,7 +381,6 @@ static void coredump_exit_mm(struct mm_struct *mm) freezable_schedule(); } __set_current_state(TASK_RUNNING); - mmap_read_lock(mm); } } @@ -480,7 +485,6 @@ static void exit_mm(void) return; sync_mm_rss(mm); mmap_read_lock(mm); - coredump_exit_mm(mm); mmgrab(mm); BUG_ON(mm != current->active_mm); /* more a memory barrier than a real lock */ @@ -768,6 +772,7 @@ void __noreturn do_exit(long code) profile_task_exit(tsk); kcov_task_exit(tsk); + coredump_task_exit(tsk); ptrace_event(PTRACE_EVENT_EXIT, code); validate_creds_for_do_exit(tsk); diff --git a/kernel/fork.c b/kernel/fork.c index 38681ad44c76..9bd9f2da9e41 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1392,8 +1392,7 @@ static void mm_release(struct task_struct *tsk, struct mm_struct *mm) * purposes. */ if (tsk->clear_child_tid) { - if (!(tsk->signal->flags & SIGNAL_GROUP_COREDUMP) && - atomic_read(&mm->mm_users) > 1) { + if (atomic_read(&mm->mm_users) > 1) { /* * We don't check the error code - if userspace has * not set up a proper pointer then tough luck. diff --git a/kernel/signal.c b/kernel/signal.c index c9759ff2cb43..b0db80acc6ef 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2158,31 +2158,6 @@ static void do_notify_parent_cldstop(struct task_struct *tsk, spin_unlock_irqrestore(&sighand->siglock, flags); } -static inline bool may_ptrace_stop(void) -{ - if (!likely(current->ptrace)) - return false; - /* - * Are we in the middle of do_coredump? - * If so and our tracer is also part of the coredump stopping - * is a deadlock situation, and pointless because our tracer - * is dead so don't allow us to stop. - * If SIGKILL was already sent before the caller unlocked - * ->siglock we must see ->core_state != NULL. Otherwise it - * is safe to enter schedule(). - * - * This is almost outdated, a task with the pending SIGKILL can't - * block in TASK_TRACED. But PTRACE_EVENT_EXIT can be reported - * after SIGKILL was already dequeued. - */ - if (unlikely(current->mm->core_state) && - unlikely(current->mm == current->parent->mm)) - return false; - - return true; -} - - /* * This must be called with current->sighand->siglock held. * @@ -2263,7 +2238,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, kernel_siginfo_t spin_unlock_irq(¤t->sighand->siglock); read_lock(&tasklist_lock); - if (may_ptrace_stop()) { + if (likely(current->ptrace)) { /* * Notify parents of the stop. * diff --git a/mm/oom_kill.c b/mm/oom_kill.c index 295c8bdfd6c8..7877c755ab37 100644 --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -788,7 +788,7 @@ static inline bool __task_will_free_mem(struct task_struct *task) /* * A coredumping process may sleep for an extended period in - * coredump_exit_mm(), so the oom killer cannot assume that + * coredump_task_exit(), so the oom killer cannot assume that * the process will promptly exit and release memory. */ if (sig->flags & SIGNAL_GROUP_COREDUMP) -- cgit v1.2.3