linux-kernel - Re: [PATCH] drivers:tty:pty: Fix a race causing data loss on close

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:   Fri, 2 Oct 2020 14:01:33 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     minyard@....org
Cc:     Jiri Slaby <jirislaby@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Corey Minyard <cminyard@...sta.com>
Subject: Re: [PATCH] drivers:tty:pty: Fix a race causing data loss on close

On Thu, Oct 01, 2020 at 09:16:30PM -0500, minyard@....org wrote:
> From: Corey Minyard <cminyard@...sta.com>
> 
> If you write to a pty master an immediately close the pty master, the
> receiver might get a chunk of data dropped, but then receive some later
> data.  That's obviously something rather unexpected for a user.  It
> certainly confused my test program.
> 
> It turns out that tty_vhangup() gets called from pty_close(), and that
> causes the data on the slave side to be flushed, but due to races more
> data can be copied into the slave side's buffer after that.  Consider
> the following sequence:
> 
> thread1            thread2            thread3
>                    write data into buffer,
>                       n_tty buffer is filled
>                    pty_close()
>                     tty_vhangup()
>                      tty_ldisc_hangup()
>                       n_tty_flush_buffer()
>                        reset_buffer_flags()
> n_tty_read()
>  up_read(&tty->termios_rwsem);
>                         down_read(&tty->termios_rwsem);
>                         clear n_tty buffer contents
>                         up_read(&tty->termios_rwsem);
>  tty_buffer_flush_work()
>   schedules work calling
>         flush_to_ldisc()
>                                       flush_to_ldisc()
>                                        receive_buf()
>                                         tty_port_default_receive_buf()
>                                          tty_ldisc_receive_buf()
>                                           tty_ldisc_receive_buf()
>                                            n_tty_receive_buf2()
>                                             n_tty_receive_buf_common()
>                                              down_read(&tty->termios_rwsem);
>                                              __receive_buf()
>                                               copies data into n_tty buffer
>                                              up_read(&tty->termios_rwsem);
>  down_read(&tty->termios_rwsem);
>  copy buffer data to user

Your text got line-wrapped here for this explaination and it doesn't
make much sense :(

Can you resend this?

> This change checks to see if the tty is being hung up before copying
> anything in n_tty_receive_buf_common().  It has to be done after the
> tty->termios_rwsem semaphore is claimed, for reasons that should be
> apparent from the sequence above.

That kind of makes sense, but it's tricky, if you resend with the above
fixed it might be more obvious...

thanks,

greg k-h