Re: [PATCH] pty: cancel pty slave port buf's work in tty_release

[Keun-O Park]

On Wed, Dec 13, 2017 at 12:23 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Dec 13, 2017 at 09:10:48AM +0400, kpark3469@xxxxxxxxx wrote:
>> From: Sahara <keun-o.park@xxxxxxxxxxxxx>
>
> I need a "Full" name here, I doubt you sign legal documents with just
> the single name, right?

For a long time, I have been using this single name as my signature in
my company, ex-company, and open communities.
If you don't mind, I want to use this single name in this life.


>
>>
>> In case that CONFIG_SLUB_DEBUG is on and pty is used, races between
>> release_one_tty and flush_to_ldisc work threads may happen and lead
>> to use-after-free condition on tty->link->port. Because SLUB_DEBUG
>> is turned on, freed tty->link->port is filled with POISON_FREE value.
>> So far without SLUB_DEBUG, port was filled with zero and flush_to_ldisc
>> could return without a problem by checking if tty is NULL.
>>
>> CPU 0                                 CPU 1
>> -----                                 -----
>> release_tty                           pty_write
>>    cancel_work_sync(tty)                 to = tty->link
>>    tty_kref_put(tty->link)               tty_schedule_flip(to->port)
>>       << workqueue >>                 ...
>>       release_one_tty                 ...
>>          pty_cleanup                  ...
>>             kfree(tty->link->port)       << workqueue >>
>>                                          flush_to_ldisc
>>                                             tty = READ_ONCE(port->itty)
>>                                             tty is 0x6b6b6b6b6b6b6b6b
>>                                             !!PANIC!! access tty->ldisc
>>
>>  Unable to handle kernel paging request at virtual address 6b6b6b6b6b6b6b93
>>  pgd = ffffffc0eb1c3000
>>  [6b6b6b6b6b6b6b93] *pgd=0000000000000000, *pud=0000000000000000
>>  ------------[ cut here ]------------
>>  Kernel BUG at ffffff800851154c [verbose debug info unavailable]
>>  Internal error: Oops - BUG: 96000004 [#1] PREEMPT SMP
>>  CPU: 3 PID: 265 Comm: kworker/u8:9 Tainted: G        W 3.18.31-g0a58eeb #1
>>  Hardware name: Qualcomm Technologies, Inc. MSM 8996pro v1.1 + PMI8996 Carbide (DT)
>>  Workqueue: events_unbound flush_to_ldisc
>>  task: ffffffc0ed610ec0 ti: ffffffc0ed624000 task.ti: ffffffc0ed624000
>>  PC is at ldsem_down_read_trylock+0x0/0x4c
>>  LR is at tty_ldisc_ref+0x24/0x4c
>>  pc : [<ffffff800851154c>] lr : [<ffffff800850f6c0>] pstate: 80400145
>>  sp : ffffffc0ed627cd0
>>  x29: ffffffc0ed627cd0 x28: 0000000000000000
>>  x27: ffffff8009e05000 x26: ffffffc0d382cfa0
>>  x25: 0000000000000000 x24: ffffff800a012f08
>>  x23: 0000000000000000 x22: ffffffc0703fbc88
>>  x21: 6b6b6b6b6b6b6b6b x20: 6b6b6b6b6b6b6b93
>>  x19: 0000000000000000 x18: 0000000000000001
>>  x17: 00e80000f80d6f53 x16: 0000000000000001
>>  x15: 0000007f7d826fff x14: 00000000000000a0
>>  x13: 0000000000000000 x12: 0000000000000109
>>  x11: 0000000000000000 x10: 0000000000000000
>>  x9 : ffffffc0ed624000 x8 : ffffffc0ed611580
>>  x7 : 0000000000000000 x6 : ffffff800a42e000
>>  x5 : 00000000000003fc x4 : 0000000003bd1201
>>  x3 : 0000000000000001 x2 : 0000000000000001
>>  x1 : ffffff800851004c x0 : 6b6b6b6b6b6b6b93
>>
>> Signed-off-by: Sahara <keun-o.park@xxxxxxxxxxxxx>
>
> Same here :)

Same comment. I hope you don't mind this.

>
>> ---
>>  drivers/tty/tty_io.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index dc60aee..a6ca634 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -1476,6 +1476,8 @@ static void release_tty(struct tty_struct *tty, int idx)
>>       if (tty->link)
>>               tty->link->port->itty = NULL;
>>       tty_buffer_cancel_work(tty->port);
>> +     if (tty->link)
>> +             tty_buffer_cancel_work(tty->link->port);
>
> Your oops above is from 3.18, which is a _very_ old kernel version.  Are
> you sure this isn't already fixed in latest kernel release?

Right. Unfortunately I am not sure if this is fixed in latest kernel
with other newly introduced routines.
But, logically it seems the latest kernel also has the same chance to
meet this issue by reading the code.
And, frankly I thought with your broad insight about tty/pty this
could be easily adopted or abandoned.
If not, then probably need more efforts to reproduce the same issue on
latest kernel by implementing pty test code.
Thanks.


>
> thanks,
>
> greg k-h