RE: [PATCH RFC] tty: tty_jobctrl: fix pid memleak in tty_signal_session_leader()

[yiyang (D)]

Ping 

-----Original Message-----
From: yiyang (D) 
Sent: 2023年7月3日 16:03
To: gregkh@xxxxxxxxxxxxxxxxxxx; jirislaby@xxxxxxxxxx
Cc: jannh@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Fengtao (fengtao, Euler) <fengtao40@xxxxxxxxxx>; Guozihua (Scott) <guozihua@xxxxxxxxxx>; yiyang (D) <yiyang13@xxxxxxxxxx>
Subject: [PATCH RFC] tty: tty_jobctrl: fix pid memleak in tty_signal_session_leader()

There is a leaked pid in tty.

unreferenced object 0xffff889362619440 (size 112):
  comm "sudo", pid 3603376, jiffies 4462415649 (age 71614.172s)
  hex dump (first 32 bytes):
    01 00 00 00 00 00 00 00 00 00 00 00 0f 00 40 da  ..............@.
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000fd13ed06>] alloc_pid+0x85/0x6b0
    [<000000007c449cf0>] copy_process+0xf60/0x2840
    [<000000008c3ae147>] kernel_clone+0x11a/0x510
    [<000000005d9b1265>] __se_sys_clone+0xcd/0x110
    [<000000009d4d672e>] do_syscall_64+0x33/0x40
    [<000000002fc0b8b9>] entry_SYSCALL_64_after_hwframe+0x61/0xc6

Race condition between disassociate_ctty() and tty_signal_session_leader()
was found, which would cause a leakage of tty_old_pgrp. The race condition
is described as follows:

CPU1:				CPU2:
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_signal_session_leader()
			{
			   spin_lock_irq(&p->sighand->siglock);
			   ...
			   spin_lock(&tty->ctrl_lock);
			   tty_pgrp = get_pid(tty->pgrp);
			   if (tty->pgrp)
	An extra get>>       p->signal->tty_old_pgrp = get_pid(tty->pgrp);
			   spin_unlock(&tty->ctrl_lock);
			   spin_unlock_irq(&p->sighand->siglock);
			}

  if (tty) {
    tty_lock(tty);
    spin_lock_irqsave(&tty->ctrl_lock, flags);
    ...
    tty->pgrp = NULL;
    spin_unlock_irqrestore(&tty->ctrl_lock, flags);
    tty_unlock(tty);
    tty_kref_put(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)".

To fix this issue, we check whether the session leader is exiting before
assigning a new tty_old_pgrp.

Fixes: c8bcd9c5be24 ("tty: Fix ->session locking")
Signed-off-by: Yi Yang <yiyang13@xxxxxxxxxx>
Co-developed-by: GUO Zihua <guozihua@xxxxxxxxxx>
Signed-off-by: GUO Zihua <guozihua@xxxxxxxxxx>
---
 drivers/tty/tty_jobctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c
index 0d04287da098..f9a144aaedfc 100644
--- a/drivers/tty/tty_jobctrl.c
+++ b/drivers/tty/tty_jobctrl.c
@@ -220,7 +220,7 @@ int tty_signal_session_leader(struct tty_struct *tty, int exit_session)
 			put_pid(p->signal->tty_old_pgrp);  /* A noop */
 			spin_lock(&tty->ctrl.lock);
 			tty_pgrp = get_pid(tty->ctrl.pgrp);
-			if (tty->ctrl.pgrp)
+			if (tty->ctrl.pgrp && !(p->flags & PF_EXITING))
 				p->signal->tty_old_pgrp =
 					get_pid(tty->ctrl.pgrp);
 			spin_unlock(&tty->ctrl.lock);
-- 
2.17.1