From f1cda3c5dd4d4dd52f52a8eb8653b023ebdd3836 Mon Sep 17 00:00:00 2001 From: Yi Yang Date: Thu, 31 Aug 2023 10:33:29 +0800 Subject: [PATCH] tty: tty_jobctrl: fix pid memleak in disassociate_ctty() [ Upstream commit 11e7f27b79757b6586645d87b95d5b78375ecdfc ] There is a pid leakage: ------------------------------ unreferenced object 0xffff88810c181940 (size 224): comm "sshd", pid 8191, jiffies 4294946950 (age 524.570s) hex dump (first 32 bytes): 01 00 00 00 00 00 00 00 00 00 00 00 ad 4e ad de .............N.. ff ff ff ff 6b 6b 6b 6b ff ff ff ff ff ff ff ff ....kkkk........ backtrace: [] kmem_cache_alloc+0x5c6/0x9b0 [] alloc_pid+0x72/0x570 [] copy_process+0x1374/0x2470 [] kernel_clone+0xb7/0x900 [] __se_sys_clone+0x85/0xb0 [] __x64_sys_clone+0x2b/0x30 [] do_syscall_64+0x32/0x80 [] entry_SYSCALL_64_after_hwframe+0x61/0xc6 It turns out that there is a race condition between disassociate_ctty() and tty_signal_session_leader(), which caused this leakage. The pid memleak is triggered by the following race: task[sshd] task[bash] ----------------------- ----------------------- disassociate_ctty(); spin_lock_irq(¤t->sighand->siglock); put_pid(current->signal->tty_old_pgrp); current->signal->tty_old_pgrp = NULL; tty = tty_kref_get(current->signal->tty); spin_unlock_irq(¤t->sighand->siglock); tty_vhangup(); tty_lock(tty); ... tty_signal_session_leader(); spin_lock_irq(&p->sighand->siglock); ... if (tty->ctrl.pgrp) //tty->ctrl.pgrp is not NULL p->signal->tty_old_pgrp = get_pid(tty->ctrl.pgrp); //An extra get spin_unlock_irq(&p->sighand->siglock); ... tty_unlock(tty); if (tty) { tty_lock(tty); ... put_pid(tty->ctrl.pgrp); tty->ctrl.pgrp = NULL; //It's too late ... tty_unlock(tty); } The issue is believed to be introduced by commit c8bcd9c5be24 ("tty: Fix ->session locking") who moves the unlock of siglock in disassociate_ctty() above "if (tty)", making a small window allowing tty_signal_session_leader() to kick in. It can be easily reproduced by adding a delay before "if (tty)" and at the entrance of tty_signal_session_leader(). To fix this issue, we move "put_pid(current->signal->tty_old_pgrp)" after "tty->ctrl.pgrp = NULL". Fixes: c8bcd9c5be24 ("tty: Fix ->session locking") Signed-off-by: Yi Yang Co-developed-by: GUO Zihua Signed-off-by: GUO Zihua Link: https://lore.kernel.org/r/20230831023329.165737-1-yiyang13@huawei.com Signed-off-by: Greg Kroah-Hartman Signed-off-by: Sasha Levin --- drivers/tty/tty_jobctrl.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c index 0d04287da098..ef8741c3e662 100644 --- a/drivers/tty/tty_jobctrl.c +++ b/drivers/tty/tty_jobctrl.c @@ -300,12 +300,7 @@ void disassociate_ctty(int on_exit) return; } - spin_lock_irq(¤t->sighand->siglock); - put_pid(current->signal->tty_old_pgrp); - current->signal->tty_old_pgrp = NULL; - tty = tty_kref_get(current->signal->tty); - spin_unlock_irq(¤t->sighand->siglock); - + tty = get_current_tty(); if (tty) { unsigned long flags; @@ -320,6 +315,16 @@ void disassociate_ctty(int on_exit) tty_kref_put(tty); } + /* If tty->ctrl.pgrp is not NULL, it may be assigned to + * current->signal->tty_old_pgrp in a race condition, and + * cause pid memleak. Release current->signal->tty_old_pgrp + * after tty->ctrl.pgrp set to NULL. + */ + spin_lock_irq(¤t->sighand->siglock); + put_pid(current->signal->tty_old_pgrp); + current->signal->tty_old_pgrp = NULL; + spin_unlock_irq(¤t->sighand->siglock); + /* Now clear signal->tty under the lock */ read_lock(&tasklist_lock); session_clear_tty(task_session(current));