[PATCH] [PATCH V2] tty: memleak in alloc_pid

[PATCH] [PATCH V2] tty: memleak in alloc_pid

[Chen Tingjie]

There is memleak in alloc_pid:
------------------------------
unreferenced object 0xd3453a80 (size 64):
  comm "adbd", pid 1730, jiffies 66363 (age 6586.950s)
  hex dump (first 32 bytes):
    01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 40 c2 f6 d5 00 d3 25 c1 59 28 00 00  ....@.....%.Y(..
  backtrace:
    [<c1a6f15c>] kmemleak_alloc+0x3c/0xa0
    [<c1320546>] kmem_cache_alloc+0xc6/0x190
    [<c125d51e>] alloc_pid+0x1e/0x400
    [<c123d344>] copy_process.part.39+0xad4/0x1120
    [<c123da59>] do_fork+0x99/0x330
    [<c123dd58>] sys_fork+0x28/0x30
    [<c1a89a08>] syscall_call+0x7/0xb
    [<ffffffff>] 0xffffffff

the leak is due to unreleased pid->count, which execute in function:
get_pid()(pid->count++) and put_pid()(pid->count--).

The race condition as following:
task[dumpsys]               task[adbd]
in disassociate_ctty()      in tty_signal_session_leader()
-----------------------     -------------------------
tty = get_current_tty();
// tty is not NULL
...
spin_lock_irq(&current->sighand->siglock);
put_pid(current->signal->tty_old_pgrp);
current->signal->tty_old_pgrp = NULL;
spin_unlock_irq(&current->sighand->siglock);

                            spin_lock_irq(&p->sighand->siglock);
                            ...
                            p->signal->tty = NULL;
                            ...
                            spin_unlock_irq(&p->sighand->siglock);

tty = get_current_tty();
// tty NULL, goto else branch by accident.
if (tty) {
    ...
    put_pid(tty_session);
    put_pid(tty_pgrp);
    ...
} else {
    print msg
}

in task[dumpsys], in disassociate_ctty(), tty is set NULL by task[adbd],
tty_signal_session_leader(), then it goto else branch and lack of
put_pid(), cause memleak.

move spin_unlock(sighand->siglock) after get_current_tty() can avoid
the race and fix the memleak.

Change-Id: Ic960dda039c8f99aad3e0f4d176489a966c62f6a
Signed-off-by: Zhang Jun <jun.zhang@intel.com>
Signed-off-by: Chen Tingjie <tingjie.chen@intel.com>
---
 drivers/tty/tty_io.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 31c4a57..72547a3 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -874,9 +874,8 @@ void disassociate_ctty(int on_exit)
 	spin_lock_irq(&current->sighand->siglock);
 	put_pid(current->signal->tty_old_pgrp);
 	current->signal->tty_old_pgrp = NULL;
-	spin_unlock_irq(&current->sighand->siglock);
 
-	tty = get_current_tty();
+	tty = tty_kref_get(current->signal->tty);
 	if (tty) {
 		unsigned long flags;
 		spin_lock_irqsave(&tty->ctrl_lock, flags);
@@ -893,6 +892,7 @@ void disassociate_ctty(int on_exit)
 #endif
 	}
 
+	spin_unlock_irq(&current->sighand->siglock);
 	/* Now clear signal->tty under the lock */
 	read_lock(&tasklist_lock);
 	session_clear_tty(task_session(current));
-- 
1.7.9.5

Re: [PATCH] [PATCH V2] tty: memleak in alloc_pid

[Greg Kroah-Hartman]

On Mon, Apr 14, 2014 at 03:31:15PM +0800, Chen Tingjie wrote:
> There is memleak in alloc_pid:
> ------------------------------
> unreferenced object 0xd3453a80 (size 64):
>   comm "adbd", pid 1730, jiffies 66363 (age 6586.950s)
>   hex dump (first 32 bytes):
>     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 40 c2 f6 d5 00 d3 25 c1 59 28 00 00  ....@.....%.Y(..
>   backtrace:
>     [<c1a6f15c>] kmemleak_alloc+0x3c/0xa0
>     [<c1320546>] kmem_cache_alloc+0xc6/0x190
>     [<c125d51e>] alloc_pid+0x1e/0x400
>     [<c123d344>] copy_process.part.39+0xad4/0x1120
>     [<c123da59>] do_fork+0x99/0x330
>     [<c123dd58>] sys_fork+0x28/0x30
>     [<c1a89a08>] syscall_call+0x7/0xb
>     [<ffffffff>] 0xffffffff
> 
> the leak is due to unreleased pid->count, which execute in function:
> get_pid()(pid->count++) and put_pid()(pid->count--).
> 
> The race condition as following:
> task[dumpsys]               task[adbd]
> in disassociate_ctty()      in tty_signal_session_leader()
> -----------------------     -------------------------
> tty = get_current_tty();
> // tty is not NULL
> ...
> spin_lock_irq(&current->sighand->siglock);
> put_pid(current->signal->tty_old_pgrp);
> current->signal->tty_old_pgrp = NULL;
> spin_unlock_irq(&current->sighand->siglock);
> 
>                             spin_lock_irq(&p->sighand->siglock);
>                             ...
>                             p->signal->tty = NULL;
>                             ...
>                             spin_unlock_irq(&p->sighand->siglock);
> 
> tty = get_current_tty();
> // tty NULL, goto else branch by accident.
> if (tty) {
>     ...
>     put_pid(tty_session);
>     put_pid(tty_pgrp);
>     ...
> } else {
>     print msg
> }
> 
> in task[dumpsys], in disassociate_ctty(), tty is set NULL by task[adbd],
> tty_signal_session_leader(), then it goto else branch and lack of
> put_pid(), cause memleak.
> 
> move spin_unlock(sighand->siglock) after get_current_tty() can avoid
> the race and fix the memleak.
> 
> Change-Id: Ic960dda039c8f99aad3e0f4d176489a966c62f6a

Why is this line here?

RE: [PATCH] [PATCH V2] tty: memleak in alloc_pid

[Chen, Tingjie]

Hello,

The related code in disassociate_ctty() as following:

---------------------------------------
    /* sometimes current->signal->tty_old_pgrp is NULL    (a stand for tty_old_pgrp not NULL, !a for tty_old_pgrp is NULL) */
    spin_lock_irq(&current->sighand->siglock);
	put_pid(current->signal->tty_old_pgrp);        // when a is NULL, will not actually put_pid() by decrease the count
	current->signal->tty_old_pgrp = NULL;
	spin_unlock_irq(&current->sighand->siglock);
    
    /* sometimes current->signal->tty is NULL     (b stand for signal->tty, !b for signal->tty is NULL) */
	tty = get_current_tty();
	if (tty) {
		unsigned long flags;
		spin_lock_irqsave(&tty->ctrl_lock, flags);
		put_pid(tty->session);
		put_pid(tty->pgrp);
		tty->session = NULL;
		tty->pgrp = NULL;
		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
		tty_kref_put(tty);
	} else {
#ifdef TTY_DEBUG_HANGUP
		printk(KERN_DEBUG "error attempted to write to tty [0x%p]"
		       " = NULL", tty);
#endif
	}
------------------------------------

Commonly there are 2 normal case flow for the code:
1/ in task: getprop: !a --> b

  This case, current->signal->tty_old_pgrp is NULL, but get_current_tty() is not NULL, 
  Really put_pid() as following:
      put_pid(tty->session);
      put_pid(tty->pgrp);

2/ in task: dumpsys: a --> !b

  This case, currnet->signal->tty_old_pgrp is not NULL, but get_current_tty() is NULL,
  Really put_pid() as following:
      put_pid(current->signal->tty_old_pgrp);

There is one abnormal case:
3/ in task: dumpsys: !a -> !b

  This case, because of race, current->signal->tty_old_pgrp and get_current_tty() are NULL,
  Lack of put_pid(), pid->count will not be 0 at last, this pid will be leak.


When read the current->signal->tty_old_pgrp and current->signal->tty,
The 2 operations should be atomic, which protected by spinlock: current->sighand->siglock.
So the sentence: spin_unlock_irq(&current->sighand->siglock);
need to move down after tty has process completed.

Thanks,

-----Original Message-----
From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] 
Sent: Monday, April 14, 2014 7:47 PM
To: Chen, Tingjie
Cc: Jiri Slaby; linux-kernel@vger.kernel.org; Zhang, Jun
Subject: Re: [PATCH] [PATCH V2] tty: memleak in alloc_pid

On Mon, Apr 14, 2014 at 03:31:15PM +0800, Chen Tingjie wrote:
> There is memleak in alloc_pid:
> ------------------------------
> unreferenced object 0xd3453a80 (size 64):
>   comm "adbd", pid 1730, jiffies 66363 (age 6586.950s)
>   hex dump (first 32 bytes):
>     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 40 c2 f6 d5 00 d3 25 c1 59 28 00 00  ....@.....%.Y(..
>   backtrace:
>     [<c1a6f15c>] kmemleak_alloc+0x3c/0xa0
>     [<c1320546>] kmem_cache_alloc+0xc6/0x190
>     [<c125d51e>] alloc_pid+0x1e/0x400
>     [<c123d344>] copy_process.part.39+0xad4/0x1120
>     [<c123da59>] do_fork+0x99/0x330
>     [<c123dd58>] sys_fork+0x28/0x30
>     [<c1a89a08>] syscall_call+0x7/0xb
>     [<ffffffff>] 0xffffffff
> 
> the leak is due to unreleased pid->count, which execute in function:
> get_pid()(pid->count++) and put_pid()(pid->count--).
> 
> The race condition as following:
> task[dumpsys]               task[adbd]
> in disassociate_ctty()      in tty_signal_session_leader()
> -----------------------     -------------------------
> tty = get_current_tty();
> // tty is not NULL
> ...
> spin_lock_irq(&current->sighand->siglock);
> put_pid(current->signal->tty_old_pgrp);
> current->signal->tty_old_pgrp = NULL;
> spin_unlock_irq(&current->sighand->siglock);
> 
>                             spin_lock_irq(&p->sighand->siglock);
>                             ...
>                             p->signal->tty = NULL;
>                             ...
>                             spin_unlock_irq(&p->sighand->siglock);
> 
> tty = get_current_tty();
> // tty NULL, goto else branch by accident.
> if (tty) {
>     ...
>     put_pid(tty_session);
>     put_pid(tty_pgrp);
>     ...
> } else {
>     print msg
> }
> 
> in task[dumpsys], in disassociate_ctty(), tty is set NULL by 
> task[adbd], tty_signal_session_leader(), then it goto else branch and 
> lack of put_pid(), cause memleak.
> 
> move spin_unlock(sighand->siglock) after get_current_tty() can avoid 
> the race and fix the memleak.
> 
> Change-Id: Ic960dda039c8f99aad3e0f4d176489a966c62f6a

Why is this line here?

RE: [PATCH] [PATCH V2] tty: memleak in alloc_pid

[Liu, Chuansheng]

> > Change-Id: Ic960dda039c8f99aad3e0f4d176489a966c62f6a
> 
> Why is this line here?

Tingjie, Greg is asking you the sentence of "Change-Id", which is not
needed, please remove it with one new patch.

RE: [PATCH] [PATCH V2] tty: memleak in alloc_pid

[Chen, Tingjie]

> > Change-Id: Ic960dda039c8f99aad3e0f4d176489a966c62f6a
> >
> > Why is this line here?

> Tingjie, Greg is asking you the sentence of "Change-Id", which is not needed, please remove it with one new patch.

Sorry for mistaken, I will make a new patch for it.