linux-kernel - Re: [PATCH] posix-cpu-timers: Cleanup CPU timers before freeing them during exec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87wnbhxnz9.fsf@email.froward.int.ebiederm.org>
Date:   Tue, 09 Aug 2022 13:37:14 -0500
From:   "Eric W. Biederman" <ebiederm@...ssion.com>
To:     Thadeu Lima de Souza Cascardo <cascardo@...onical.com>
Cc:     linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        stable@...r.kernel.org
Subject: Re: [PATCH] posix-cpu-timers: Cleanup CPU timers before freeing
 them during exec

Thadeu Lima de Souza Cascardo <cascardo@...onical.com> writes:

> Commit 55e8c8eb2c7b ("posix-cpu-timers: Store a reference to a pid not a
> task") started looking up tasks by PID when deleting a CPU timer.
>
> When a non-leader thread calls execve, it will switch PIDs with the leader
> process. Then, as it calls exit_itimers, posix_cpu_timer_del cannot find
> the task because the timer still points out to the old PID.

I think this description is missing something.

Looking at how clock_pid_type selects which task to go through
to obtain the sighand lock, and the fact that the sighand_struct
can change during exec all make me think that this change isn't
necessarily wrong, I am just trying to understand what is going
on that makes this necessary.

The function cpu_timer_task_rcu should return the one remaining task if
it is process wide timer, as all of the other threads have been reaped
after de_thread.

For a per thread timer for the surviving thread I can see exchange_tids
causing clock_pid_type to returning the threads old pid, and which
exchange_tids attached to a task that de_thread has freed with
release_task.

If that analysis is correct I think your change is safe only
because posix_cpu_timers_exit does not do anything with the pids.

Perhaps it would be better to do something like the diff below.  That
is always call posix_cpu_timers_exit before exchange_tids can run.  That
way there is nothing clever going on for us to stumble over later.

Once long ago I tried to remove the pid swap but unfortunately the
glibc pthread relies on the fact that getpid() == gettid() for the
first thread after exec.  Sigh.

diff --git a/fs/exec.c b/fs/exec.c
index 45914e57c0d5..a2a0b3faf603 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1072,6 +1072,10 @@ static int de_thread(struct task_struct *tsk)
 	if (!thread_group_leader(tsk))
 		sig->notify_count--;

+#ifdef CONFIG_POSIX_TIMERS
+	/* Cleanup the per thread timers before the pid changes */
+	posix_cpu_timers_exit(tsk);
+#endif
 	while (sig->notify_count) {
 		__set_current_state(TASK_KILLABLE);
 		spin_unlock_irq(lock);
diff --git a/kernel/exit.c b/kernel/exit.c
index 4f7424523bac..f7e19b73cf6c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -104,7 +104,6 @@ static void __exit_signal(struct task_struct *tsk)
 	spin_lock(&sighand->siglock);

 #ifdef CONFIG_POSIX_TIMERS
-	posix_cpu_timers_exit(tsk);
 	if (group_dead)
 		posix_cpu_timers_exit_group(tsk);
 #endif
@@ -772,6 +771,12 @@ void __noreturn do_exit(long code)
 	if (tsk->mm)
 		sync_mm_rss(tsk->mm);
 	acct_update_integrals(tsk);
+#ifdef CONFIG_POSIX_TIMERS
+	/* Cleanup the per thread timers before de_thread can change the pid */
+	spin_lock_irq(&tsk->sighand->siglock);
+	posix_cpu_timers_exit(tsk);
+	spin_unlock_irq(&tsk->sighand->siglock);
+#endif
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
 	if (group_dead) {
 		/*

Eric