Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

tty: tty_jobctrl: fix pid memleak in disassociate_ctty()

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:
[<ffffffff814774e6>] kmem_cache_alloc+0x5c6/0x9b0
[<ffffffff81177342>] alloc_pid+0x72/0x570
[<ffffffff81140ac4>] copy_process+0x1374/0x2470
[<ffffffff81141d77>] kernel_clone+0xb7/0x900
[<ffffffff81142645>] __se_sys_clone+0x85/0xb0
[<ffffffff8114269b>] __x64_sys_clone+0x2b/0x30
[<ffffffff83965a72>] do_syscall_64+0x32/0x80
[<ffffffff83a00085>] 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(&current->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(&current->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 <yiyang13@huawei.com>
Co-developed-by: GUO Zihua <guozihua@huawei.com>
Signed-off-by: GUO Zihua <guozihua@huawei.com>
Link: https://lore.kernel.org/r/20230831023329.165737-1-yiyang13@huawei.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

authored by

Yi Yang and committed by
Greg Kroah-Hartman
11e7f27b 305a5dd7

+11 -6
+11 -6
drivers/tty/tty_jobctrl.c
··· 300 300 return; 301 301 } 302 302 303 - spin_lock_irq(&current->sighand->siglock); 304 - put_pid(current->signal->tty_old_pgrp); 305 - current->signal->tty_old_pgrp = NULL; 306 - tty = tty_kref_get(current->signal->tty); 307 - spin_unlock_irq(&current->sighand->siglock); 308 - 303 + tty = get_current_tty(); 309 304 if (tty) { 310 305 unsigned long flags; 311 306 ··· 314 319 tty_unlock(tty); 315 320 tty_kref_put(tty); 316 321 } 322 + 323 + /* If tty->ctrl.pgrp is not NULL, it may be assigned to 324 + * current->signal->tty_old_pgrp in a race condition, and 325 + * cause pid memleak. Release current->signal->tty_old_pgrp 326 + * after tty->ctrl.pgrp set to NULL. 327 + */ 328 + spin_lock_irq(&current->sighand->siglock); 329 + put_pid(current->signal->tty_old_pgrp); 330 + current->signal->tty_old_pgrp = NULL; 331 + spin_unlock_irq(&current->sighand->siglock); 317 332 318 333 /* Now clear signal->tty under the lock */ 319 334 read_lock(&tasklist_lock);