[PATCH] signal: Use list_del_init_careful() in flush_sigqueue()

[Hyunwoo Kim]

commit fb3bbcfe344e ("exit: change the release_task() paths to call
flush_sigqueue() lockless") moved the ->pending flush from __exit_signal()
to release_task(), where it runs without ->siglock. The justification was:

  after the exiting task passes __exit_signal() lock_task_sighand() can't
  succeed and pid_task(tmr->it_pid) will return NULL

That second half does not hold for the old group leader in a non-leader
exec(). de_thread() calls exchange_tids() before release_task(leader), so
the struct pid held by a SIGEV_THREAD_ID timer created against the leader's
tid now points to the thread which called execve(). pid_task() returns that
thread and lock_task_sighand() on it succeeds. It uses the same sighand the
leader used, so while the flush was still done in __exit_signal(), that one
->siglock serialized the two.

If the timer signal is blocked, its sigqueue stays queued on the leader's
->pending. The next expiry of that timer can then run while release_task()
flushes the queue.

posixtimer_send_sigqueue() checks whether the sigqueue is already queued
with a plain list_empty(), which only reads ->next. list_del_init() is not
atomic and INIT_LIST_HEAD() stores ->next before ->prev, so the check can
pass in between. list_add_tail() queues the entry on the ->pending of the
live thread, and the ->prev store from the flush then overwrites the ->prev
link that list_add_tail() has just set.

__flush_itimer_signals() does not undo that either. With ->prev pointing at
the entry itself, its list_del_init() only stores the same values again, so
the entry is not removed from the list. It is still there after the last
reference is dropped and the timer is freed by RCU, and the list_add_tail()
of a later tgkill() follows that ->prev into the freed timer:

  BUG: KASAN: slab-use-after-free in __send_signal_locked+0xb27/0xba0
  Write of size 8 at addr ffff888007ed80c8 by task poc/79
  ...
  Call Trace:
   __send_signal_locked+0xb27/0xba0
   do_send_sig_info+0xa7/0x160
   do_send_specific+0x76/0xa0
   __x64_sys_tgkill+0x193/0x270
  ...
  Allocated by task 80:
   do_timer_create+0x1a4/0x1030
   __x64_sys_timer_create+0x145/0x190
  ...
  Freed by task 12:
   kmem_cache_free_bulk+0x1f8/0x4a0
   kvfree_rcu_bulk+0x14f/0x1c0
   kfree_rcu_work+0x128/0x1a0
  ...
  Last potentially related work creation:
   kvfree_call_rcu+0x39/0x390
   __flush_itimer_signals+0x211/0x320
   flush_itimer_signals+0x47/0x90
   begin_new_exec+0xa6b/0x28c0
  ...
  The buggy address belongs to the object at ffff888007ed8040
   which belongs to the cache posix_timers_cache of size 384

Use list_del_init_careful(), which stores ->next last. A list_empty() which
sees the entry unqueued is then guaranteed that the flush will not store
into the entry any more.

Fixes: fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Hyunwoo Kim <imv4bel@xxxxxxxxx>
---
 kernel/signal.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index bbc0fd4cc4d7c1..ec9a0a0490d19f 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -482,7 +482,11 @@ void flush_sigqueue(struct sigpending *queue)
 	sigemptyset(&queue->signal);
 	while (!list_empty(&queue->list)) {
 		q = list_entry(queue->list.next, struct sigqueue , list);
-		list_del_init(&q->list);
+		/*
+		 * Pairs with the list_empty() in posixtimer_send_sigqueue().
+		 * release_task() gets here without ->siglock.
+		 */
+		list_del_init_careful(&q->list);
 		__sigqueue_free(q);
 	}
 }
-- 
2.43.0