| Message ID | c2f1b742c2b8f230f8e0ffa10a061116934dcf5b.1735550994.git.namcao@linutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix reading ESP during coredump | expand |
On Mon, Dec 30, 2024 at 11:17:30AM +0100, Nam Cao wrote: > The field "eip" (instruction pointer) and "esp" (stack pointer) of a task > can be read from /proc/PID/stat. These fields can be interesting for > coredump. > > However, these fields were disabled by commit 0a1eb2d474ed ("fs/proc: Stop > reporting eip and esp in /proc/PID/stat"), because it is generally unsafe > to do so. But it is safe for a coredumping process, and therefore > exceptions were made: > > - for a coredumping thread by commit fd7d56270b52 ("fs/proc: Report > eip/esp in /prod/PID/stat for coredumping"). > > - for all other threads in a coredumping process by commit cb8f381f1613 > ("fs/proc/array.c: allow reporting eip/esp for all coredumping > threads"). > > The above two commits check the PF_DUMPCORE flag to determine a coredump thread > and the PF_EXITING flag for the other threads. > > Unfortunately, commit 92307383082d ("coredump: Don't perform any cleanups > before dumping core") moved coredump to happen earlier and before PF_EXITING is > set. Thus, checking PF_EXITING is no longer the correct way to determine > threads in a coredumping process. > > Instead of PF_EXITING, use PF_POSTCOREDUMP to determine the other threads. > > Checking of PF_EXITING was added for coredumping, so it probably can now be > removed. But it doesn't hurt to keep. > > Fixes: 92307383082d ("coredump: Don't perform any cleanups before dumping core") > Cc: stable@vger.kernel.org > Cc: Eric W. Biederman <ebiederm@xmission.com> > Acked-by: Oleg Nesterov <oleg@redhat.com> > Signed-off-by: Nam Cao <namcao@linutronix.de> Thanks for fixing this! Acked-by: Kees Cook <kees@kernel.org>
diff --git a/fs/proc/array.c b/fs/proc/array.c index 55ed3510d2bb..d6a0369caa93 100644 --- a/fs/proc/array.c +++ b/fs/proc/array.c @@ -500,7 +500,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, * a program is not able to use ptrace(2) in that case. It is * safe because the task has stopped executing permanently. */ - if (permitted && (task->flags & (PF_EXITING|PF_DUMPCORE))) { + if (permitted && (task->flags & (PF_EXITING|PF_DUMPCORE|PF_POSTCOREDUMP))) { if (try_get_task_stack(task)) { eip = KSTK_EIP(task); esp = KSTK_ESP(task);
The field "eip" (instruction pointer) and "esp" (stack pointer) of a task can be read from /proc/PID/stat. These fields can be interesting for coredump. However, these fields were disabled by commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in /proc/PID/stat"), because it is generally unsafe to do so. But it is safe for a coredumping process, and therefore exceptions were made: - for a coredumping thread by commit fd7d56270b52 ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping"). - for all other threads in a coredumping process by commit cb8f381f1613 ("fs/proc/array.c: allow reporting eip/esp for all coredumping threads"). The above two commits check the PF_DUMPCORE flag to determine a coredump thread and the PF_EXITING flag for the other threads. Unfortunately, commit 92307383082d ("coredump: Don't perform any cleanups before dumping core") moved coredump to happen earlier and before PF_EXITING is set. Thus, checking PF_EXITING is no longer the correct way to determine threads in a coredumping process. Instead of PF_EXITING, use PF_POSTCOREDUMP to determine the other threads. Checking of PF_EXITING was added for coredumping, so it probably can now be removed. But it doesn't hurt to keep. Fixes: 92307383082d ("coredump: Don't perform any cleanups before dumping core") Cc: stable@vger.kernel.org Cc: Eric W. Biederman <ebiederm@xmission.com> Acked-by: Oleg Nesterov <oleg@redhat.com> Signed-off-by: Nam Cao <namcao@linutronix.de> --- fs/proc/array.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)