linux-kernel - Re: [PATCH] tty: fix data race in n_tty_receive_buf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 4 Jan 2018 14:37:16 +0000
From:   Alan Cox <gnomes@...rguk.ukuu.org.uk>
To:     "Kohli, Gaurav" <gkohli@...eaurora.org>
Cc:     jslaby@...e.com, gregkh@...uxfoundation.org, mikey@...ling.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH] tty: fix data race in n_tty_receive_buf_common

On Thu, 4 Jan 2018 19:16:46 +0530
"Kohli, Gaurav" <gkohli@...eaurora.org> wrote:

> > Which tty driver ? serial/msm_serial.c ?  
> 
> We are using our internal driver, msm_geni_serial.c

Can you make that code available otherwise it's impossible to see what
the problem might be.

> >    
> > Ok no what I need to see is a trace of what each CPU is doing at the
> > point you detect the problem. That way we can see what the path that
> > races is.  
> Below is stack trace running by init in our case on one core
> -006|n_tty_open(
>      |    tty = 0xFFFFFFFF477AC880 -> (
>      |      disc_data = 0xFFFFFF80197AD000,
> 
>      |      port = 0xFFFFFFFFEDE40000))
>      |  ldata = 0xFFFFFF80197AD000
> 
>      |  trace_printk_fmt = 0xFFFFFF9F275125F8
> -007|tty_ldisc_open.isra.3(
>      |    tty = 0xFFFFFFFF477AC880)
> -008|tty_ldisc_setup(
> 
> -009|tty_init_dev(
>      |    driver = 0xFFFFFFFFEDE2A480,
>      |    idx = 0)
> 
> -010|tty_open_by_driver(inline)
> -010|tty_open(

So core 1 is opening the tty from user space and that's a normal looking
trace for an open of a port that was closed

> 
> Core 2:
> -000|n_tty_receive_buf_common(
>      |    tty = 0xFFFFFFFF477AC880,
> 
>      |  ?)
>      |  ldata_=_0x0
>      |  __func__ = (110, 95, 116, 116, 121, 95, 114, 101, 99, 101, 105, 
> 118, 101, 95, 98, 117, 102, 95, 99, 111, 109, 109, 111, 110, 0)
>      |  __u = (__val = 7079195495121566464, __c = (0))
>      |  c = 127
>      |  ldata = 0xFFFFFFFFF40DF97C
> 
>      |  c = 0
>      |  ldata = 0xFFFFFF9F26F46000
> 
> -001|n_tty_receive_buf2(
>      |    tty = 0xFFFFFFFF477AC880,
> 
> -002|tty_ldisc_receive_buf(inline)
> -002|receive_buf(inline)
> -002|flush_to_ldisc(

This is probably the important bit. As you say we are doing a flush to
ldisc for a port even though it is not open.

That's starting to make more sense. Becausee your driver is the console
tty_port_shutdown doesn't stop everything (so console printk still
works), and that means you can receive data and we have a window on
reopening a tty that is only in use as a console where port->tty is valid
but ldisc is not.

I wonder what Jiri thinks but my first thougt is that tty_init_dev in
fact needs to do

	tty_ldisc_lock(tty, 5 * HZ);
	tty_ldisc_setup(tty);
	tty_ldisc_unlock(tty)

with the relevant error handling so that the flush_to_ldisc waits and
either hits 'no ldisc' or 'ldisc valid'

Alan