Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230724033714.175340-1-yiyang13@huawei.com> <cd1019d0-55b8-6276-dd5e-3a7a9264fe06@kernel.org>
In-Reply-To: <cd1019d0-55b8-6276-dd5e-3a7a9264fe06@kernel.org>
From:   Jann Horn <jannh@google.com>
Date:   Mon, 24 Jul 2023 20:04:50 +0200
Message-ID: <CAG48ez1xRr4dtF9skZ=ruXpbi82PutTWGqTgeyk9RAQDFZCDKQ@mail.gmail.com>
Subject: Re: [PATCH v2] tty: tty_jobctrl: fix pid memleak in disassociate_ctty()
To:     Jiri Slaby <jirislaby@kernel.org>
Cc:     Yi Yang <yiyang13@huawei.com>, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, guozihua@huawei.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jul 24, 2023 at 10:28=E2=80=AFAM Jiri Slaby <jirislaby@kernel.org> =
wrote:
> On 24. 07. 23, 5:37, Yi Yang wrote:
> > 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->sigloc=
k);
> >                                 put_pid(current->signal->tty_old_pgrp);
> >                                 current->signal->tty_old_pgrp =3D NULL;
> >                                 tty =3D tty_kref_get(current->signal->t=
ty);
> >                                 spin_unlock_irq(&current->sighand->sigl=
ock);
> > tty_vhangup();
> > tty_lock(tty);
> > ...
> > tty_signal_session_leader();
> > spin_lock_irq(&p->sighand->siglock);
> > ...
> > if (tty->pgrp) //tty->pgrp is not NULL
> > p->signal->tty_old_pgrp =3D get_pid(tty->pgrp); //An extra get
> > spin_unlock_irq(&p->sighand->siglock);
> > ...
> > tty_unlock(tty);
> >                                 if (tty) {
> >                                     tty_lock(tty);
> >                                     ...
> >                                     put_pid(tty->pgrp);
> >                                     tty->pgrp =3D NULL; // It's too lat=
e
> >                                     ...
> >                                     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() "tty_signal_session_leader()".
>
> Funny, the commit effectively reverted c70dbb1e79a1 ("tty: fix memleak
> in alloc_pid") which appears to be fixing exactly what you are reporting
> now again.
>
> > To fix this issue, we move put_pid() after "if (tty)".
> >
> > 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>
> > ---
> > v2:Completely refactor the solution, avoid the use of PF_EXITING flag a=
nd
> > do put_pid() in disassociate_ctty() again instead.
> > ---
> >   drivers/tty/tty_jobctrl.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c
> > index 0d04287da098..17a6565f428b 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(&current->sighand->siglock);
> > -     put_pid(current->signal->tty_old_pgrp);
> > -     current->signal->tty_old_pgrp =3D NULL;
> > -     tty =3D tty_kref_get(current->signal->tty);
> > -     spin_unlock_irq(&current->sighand->siglock);
> > -
> > +     tty =3D get_current_tty();
> >       if (tty) {
> >               unsigned long flags;
> >
> > @@ -320,6 +315,11 @@ void disassociate_ctty(int on_exit)
> >               tty_kref_put(tty);
> >       }
> >
> > +     spin_lock_irq(&current->sighand->siglock);
> > +     put_pid(current->signal->tty_old_pgrp);
> > +     current->signal->tty_old_pgrp =3D NULL;
> > +     spin_unlock_irq(&current->sighand->siglock);
>
> It _appears_ to be correct (the locking of all this is quite hairy). But
> at the very least, this block deserves a comment why we do it the second
> time.

What is "it" in "do it the second time"? Are you referring to calling
get_current_tty()?