[GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Greg KH]

The following changes since commit 4495c08e84729385774601b5146d51d9e5849f81:

  Linux 4.11-rc2 (2017-03-12 14:47:08 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/ tags/tty-4.11-rc4

for you to fetch changes up to a4a3e061149f09c075f108b6f1cf04d9739a6bc2:

  tty: fix data race in tty_ldisc_ref_wait() (2017-03-17 14:07:10 +0900)

----------------------------------------------------------------
TTY/Serial driver fixes for 4.11-rc4

Here are some tty and serial driver fixes for 4.11-rc4.  One of these
fix a long-standing issue in the ldisc code that was found by Dmitry
Vyukov with his great fuzzing work.  The other fixes resolve other
reported issues, and there is one revert of a patch in 4.11-rc1 that
wasn't correct.

All of these have been in linux-next for a while with no reported
issues.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

----------------------------------------------------------------
Aleksey Makarov (1):
      Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"

Dmitry Vyukov (2):
      tty: don't panic on OOM in tty_set_ldisc()
      tty: fix data race in tty_ldisc_ref_wait()

Heiko Stuebner (1):
      serial: 8250_dw: Honor clk_round_rate errors in dw8250_set_termios

James Hogan (1):
      serial: 8250_dw: Fix breakage when HAVE_CLK=n

Timur Tabi (1):
      tty: acpi/spcr: QDF2400 E44 checks for wrong OEM revision

 drivers/acpi/spcr.c               |  2 +-
 drivers/tty/serial/8250/8250_dw.c |  9 +++-
 drivers/tty/serial/amba-pl011.c   |  2 +-
 drivers/tty/tty_ldisc.c           | 92 +++++++++------------------------------
 4 files changed, 30 insertions(+), 75 deletions(-)

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Vegard Nossum]

On 26 March 2017 at 13:04, Greg KH <gregkh@linuxfoundation.org> wrote:
> The following changes since commit 4495c08e84729385774601b5146d51d9e5849f81:
>
>   Linux 4.11-rc2 (2017-03-12 14:47:08 -0700)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/ tags/tty-4.11-rc4
>
> for you to fetch changes up to a4a3e061149f09c075f108b6f1cf04d9739a6bc2:
>
>   tty: fix data race in tty_ldisc_ref_wait() (2017-03-17 14:07:10 +0900)
>
> ----------------------------------------------------------------
> TTY/Serial driver fixes for 4.11-rc4
>
> Here are some tty and serial driver fixes for 4.11-rc4.  One of these
> fix a long-standing issue in the ldisc code that was found by Dmitry
> Vyukov with his great fuzzing work.  The other fixes resolve other
> reported issues, and there is one revert of a patch in 4.11-rc1 that
> wasn't correct.
>
> All of these have been in linux-next for a while with no reported
> issues.
>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ----------------------------------------------------------------
> Aleksey Makarov (1):
>       Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"
>
> Dmitry Vyukov (2):
>       tty: don't panic on OOM in tty_set_ldisc()

I've bisected a syzkaller crash down to this commit
(5362544bebe85071188dd9e479b5a5040841c895). The crash is:

[   25.137552] BUG: unable to handle kernel paging request at 0000000000002280
[   25.137579] IP: mutex_lock_interruptible+0xb/0x30
[   25.137589] PGD 3b0c067
[   25.137593] PUD 3911067
[   25.137597] PMD 0
[   25.137601]
[   25.137611] Oops: 0002 [#1] PREEMPT SMP KASAN
[   25.137624] CPU: 1 PID: 3690 Comm: a.out Not tainted 4.11.0-rc2+ #145
[   25.137631] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[   25.137639] task: ffff880003b96400 task.stack: ffff880004e98000
[   25.137651] RIP: 0010:mutex_lock_interruptible+0xb/0x30
[   25.137657] RSP: 0018:ffff880004e9fae0 EFLAGS: 00010246
[   25.137668] RAX: 0000000000000000 RBX: ffff880004e6c000 RCX: ffffffff817bb2a9
[   25.137675] RDX: ffff880003b96400 RSI: 0000000000000015 RDI: 0000000000002280
[   25.137696] RBP: ffff880004e9fca0 R08: 0000000000000003 R09: 0000000000000002
[   25.137703] R10: 0000000000000002 R11: ffffed0000c23fe9 R12: ffff880004e6c000
[   25.137710] R13: 0000000080045430 R14: ffff880004bac900 R15: ffff880004bacb60
[   25.137720] FS:  00007f7cac233700(0000) GS:ffff880006100000(0000)
knlGS:0000000000000000
[   25.137727] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   25.137733] CR2: 0000000000002280 CR3: 0000000003b67000 CR4: 00000000000006e0
[   25.137746] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   25.137752] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   25.137755] Call Trace:
[   25.137769]  ? n_tty_read+0x15f/0xc70
[   25.137783]  ? preempt_count_add+0xb2/0xe0
[   25.137793]  ? n_tty_flush_buffer+0x90/0x90
[   25.137806]  ? wait_woken+0x100/0x100
[   25.137817]  tty_read+0xd8/0x140
[   25.137830]  __vfs_read+0xd1/0x320
[   25.137842]  ? do_sendfile+0x6c0/0x6c0
[   25.137853]  ? __fsnotify_update_child_dentry_flags+0x30/0x30
[   25.137864]  ? selinux_file_permission+0x1c0/0x210
[   25.137873]  ? __fsnotify_parent+0x27/0x130
[   25.137882]  ? security_file_permission+0xce/0xf0
[   25.137893]  ? rw_verify_area+0x73/0x140
[   25.137904]  vfs_read+0xba/0x1b0
[   25.137915]  SyS_read+0xa0/0x120
[   25.137926]  ? vfs_write+0x260/0x260
[   25.137938]  ? preempt_count_sub+0x13/0xd0
[   25.137949]  entry_SYSCALL_64_fastpath+0x1a/0xa9
[   25.137957] RIP: 0033:0x7f7caf61351d
[   25.137963] RSP: 002b:00007f7cac232f20 EFLAGS: 00000293 ORIG_RAX:
0000000000000000
[   25.137974] RAX: ffffffffffffffda RBX: 00007f7cac233700 RCX: 00007f7caf61351d
[   25.137980] RDX: 000000000000003e RSI: 0000000080045430 RDI: 0000000000000004
[   25.137987] RBP: 00007fffb4f21250 R08: 00007f7cac233700 R09: 00007f7cac233700
[   25.137993] R10: 00007f7cac2339d0 R11: 0000000000000293 R12: 0000000000000000
[   25.137999] R13: 00007fffb4f2124f R14: 00007f7cac2339c0 R15: 0000000000000000
[   25.138002] Code: c7 43 20 00 00 00 00 48 89 df e8 91 ff ff ff 5b
41 5c 5d c3 83 e8 01 41 89 44 24 10 eb e1 66 90 65 48 8b 14 25 40 54
01 00 31 c0 <f0> 48 0f b1 17 48 85 c0 74 0a 55 48 89 e5 e8 e2 f4 ff ff
5d f3
[   25.138218] RIP: mutex_lock_interruptible+0xb/0x30 RSP: ffff880004e9fae0
[   25.138221] CR2: 0000000000002280
[   25.138301] ---[ end trace 242fd54c56b177b4 ]---

The syzkaller reproducer is:

# {Threaded:true Collide:true Repeat:true Procs:1 Sandbox:setuid Repro:false}
mmap(&(0x7f0000000000/0x9f000)=nil, (0x9f000), 0x3, 0x32,
0xffffffffffffffff, 0x0)
r0 = openat$ptmx(0xffffffffffffff9c,
&(0x7f0000001000-0xa)="2f6465762f70746d7800", 0x201, 0x0)
ioctl$TIOCSPTLCK(r0, 0x40045431, &(0x7f000009a000)=0x0)
r1 = syz_open_pts(r0, 0x0)
read(r1, &(0x7f0000028000-0x86)="0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
0x3e)
ioctl$TIOCSETD(r1, 0x5423, &(0x7f000009f000-0x4)=0x10080000001)

It takes 10-50 seconds to reproduce, but you can reduce it to ~2
seconds by opening /dev/ptmx only once in each process.

I've verified that reverting the commit from latest mainline makes the
crash go away.


Vegard

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Linus Torvalds]

On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>
> I've bisected a syzkaller crash down to this commit
> (5362544bebe85071188dd9e479b5a5040841c895). The crash is:
>
> [   25.137552] BUG: unable to handle kernel paging request at 0000000000002280
> [   25.137579] IP: mutex_lock_interruptible+0xb/0x30

It would seem to be the

                if (mutex_lock_interruptible(&ldata->atomic_read_lock))

call in n_tty_read(), the offset is about right for a NULL 'ldata'
pointer (it's a big structure, it has a couple of character buffers of
size N_TTY_BUF_SIZE).

I don't see the obvious fix, so I suspect at this point we should just
revert, as that commit seems to introduce worse problems that it is
supposed to fix. Greg?

              Linus

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Greg KH]

On Thu, Apr 13, 2017 at 09:07:40AM -0700, Linus Torvalds wrote:
> On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> >
> > I've bisected a syzkaller crash down to this commit
> > (5362544bebe85071188dd9e479b5a5040841c895). The crash is:
> >
> > [   25.137552] BUG: unable to handle kernel paging request at 0000000000002280
> > [   25.137579] IP: mutex_lock_interruptible+0xb/0x30
> 
> It would seem to be the
> 
>                 if (mutex_lock_interruptible(&ldata->atomic_read_lock))
> 
> call in n_tty_read(), the offset is about right for a NULL 'ldata'
> pointer (it's a big structure, it has a couple of character buffers of
> size N_TTY_BUF_SIZE).
> 
> I don't see the obvious fix, so I suspect at this point we should just
> revert, as that commit seems to introduce worse problems that it is
> supposed to fix. Greg?

Unless Dmitry has a better idea, I will just revert it and send you the
pull request in a day or so.

thanks,

greg k-h

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Vegard Nossum]

On 13 April 2017 at 20:34, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Apr 13, 2017 at 09:07:40AM -0700, Linus Torvalds wrote:
>> On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>> >
>> > I've bisected a syzkaller crash down to this commit
>> > (5362544bebe85071188dd9e479b5a5040841c895). The crash is:
>> >
>> > [   25.137552] BUG: unable to handle kernel paging request at 0000000000002280
>> > [   25.137579] IP: mutex_lock_interruptible+0xb/0x30
>>
>> It would seem to be the
>>
>>                 if (mutex_lock_interruptible(&ldata->atomic_read_lock))
>>
>> call in n_tty_read(), the offset is about right for a NULL 'ldata'
>> pointer (it's a big structure, it has a couple of character buffers of
>> size N_TTY_BUF_SIZE).
>>
>> I don't see the obvious fix, so I suspect at this point we should just
>> revert, as that commit seems to introduce worse problems that it is
>> supposed to fix. Greg?
>
> Unless Dmitry has a better idea, I will just revert it and send you the
> pull request in a day or so.

I don't think we need to rush a revert, I'd hope there's a way to fix
it properly.

So the original problem is that the vmalloc() in n_tty_open() can
fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
because of its unwillingness to proceed if the tty doesn't have an
ldisc.

Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
allocation failure as we can see from the comment in tty_set_ldisc().

Unfortunately, it would appear that some other bits of code do not
like tty->ldisc == NULL (other than the crash in this thread, I saw
2-3 similar crashes in other functions, e.g. poll()). I see two
possibilities:

1) make other code handle tty->ldisc == NULL.

2) don't close/free the old ldisc until the new one has been
successfully created/initialised/opened/attached to the tty, and
return an error to userspace if changing it failed.

I'm leaning towards #2 as the more obviously correct fix, it makes
tty_set_ldisc() transactional, the fix seems limited in scope to
tty_set_ldisc() itself, and we don't need to make every other bit of
code that uses tty->ldisc handle the NULL case.


Vegard

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Greg KH]

On Fri, Apr 14, 2017 at 11:41:26AM +0200, Vegard Nossum wrote:
> On 13 April 2017 at 20:34, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Apr 13, 2017 at 09:07:40AM -0700, Linus Torvalds wrote:
> >> On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> >> >
> >> > I've bisected a syzkaller crash down to this commit
> >> > (5362544bebe85071188dd9e479b5a5040841c895). The crash is:
> >> >
> >> > [   25.137552] BUG: unable to handle kernel paging request at 0000000000002280
> >> > [   25.137579] IP: mutex_lock_interruptible+0xb/0x30
> >>
> >> It would seem to be the
> >>
> >>                 if (mutex_lock_interruptible(&ldata->atomic_read_lock))
> >>
> >> call in n_tty_read(), the offset is about right for a NULL 'ldata'
> >> pointer (it's a big structure, it has a couple of character buffers of
> >> size N_TTY_BUF_SIZE).
> >>
> >> I don't see the obvious fix, so I suspect at this point we should just
> >> revert, as that commit seems to introduce worse problems that it is
> >> supposed to fix. Greg?
> >
> > Unless Dmitry has a better idea, I will just revert it and send you the
> > pull request in a day or so.
> 
> I don't think we need to rush a revert, I'd hope there's a way to fix
> it properly.

For this late in the release cycle, for something as complex as tty
ldisc handling, for an issue that has been present for over a decade,
the safest thing right now is to go back to the old well-known code by
applying a revert :)

> So the original problem is that the vmalloc() in n_tty_open() can
> fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
> because of its unwillingness to proceed if the tty doesn't have an
> ldisc.
> 
> Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
> allocation failure as we can see from the comment in tty_set_ldisc().
> 
> Unfortunately, it would appear that some other bits of code do not
> like tty->ldisc == NULL (other than the crash in this thread, I saw
> 2-3 similar crashes in other functions, e.g. poll()). I see two
> possibilities:
> 
> 1) make other code handle tty->ldisc == NULL.
> 
> 2) don't close/free the old ldisc until the new one has been
> successfully created/initialised/opened/attached to the tty, and
> return an error to userspace if changing it failed.
> 
> I'm leaning towards #2 as the more obviously correct fix, it makes
> tty_set_ldisc() transactional, the fix seems limited in scope to
> tty_set_ldisc() itself, and we don't need to make every other bit of
> code that uses tty->ldisc handle the NULL case.

That sounds reasonable to me, care to work on a patch for this?

thanks,

greg k-h

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Dmitry Vyukov]

On Fri, Apr 14, 2017 at 2:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Apr 14, 2017 at 11:41:26AM +0200, Vegard Nossum wrote:
>> On 13 April 2017 at 20:34, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Apr 13, 2017 at 09:07:40AM -0700, Linus Torvalds wrote:
>> >> On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>> >> >
>> >> > I've bisected a syzkaller crash down to this commit
>> >> > (5362544bebe85071188dd9e479b5a5040841c895). The crash is:
>> >> >
>> >> > [   25.137552] BUG: unable to handle kernel paging request at 0000000000002280
>> >> > [   25.137579] IP: mutex_lock_interruptible+0xb/0x30
>> >>
>> >> It would seem to be the
>> >>
>> >>                 if (mutex_lock_interruptible(&ldata->atomic_read_lock))
>> >>
>> >> call in n_tty_read(), the offset is about right for a NULL 'ldata'
>> >> pointer (it's a big structure, it has a couple of character buffers of
>> >> size N_TTY_BUF_SIZE).
>> >>
>> >> I don't see the obvious fix, so I suspect at this point we should just
>> >> revert, as that commit seems to introduce worse problems that it is
>> >> supposed to fix. Greg?
>> >
>> > Unless Dmitry has a better idea, I will just revert it and send you the
>> > pull request in a day or so.
>>
>> I don't think we need to rush a revert, I'd hope there's a way to fix
>> it properly.
>
> For this late in the release cycle, for something as complex as tty
> ldisc handling, for an issue that has been present for over a decade,
> the safest thing right now is to go back to the old well-known code by
> applying a revert :)
>
>> So the original problem is that the vmalloc() in n_tty_open() can
>> fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
>> because of its unwillingness to proceed if the tty doesn't have an
>> ldisc.
>>
>> Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
>> allocation failure as we can see from the comment in tty_set_ldisc().
>>
>> Unfortunately, it would appear that some other bits of code do not
>> like tty->ldisc == NULL (other than the crash in this thread, I saw
>> 2-3 similar crashes in other functions, e.g. poll()). I see two
>> possibilities:
>>
>> 1) make other code handle tty->ldisc == NULL.
>>
>> 2) don't close/free the old ldisc until the new one has been
>> successfully created/initialised/opened/attached to the tty, and
>> return an error to userspace if changing it failed.
>>
>> I'm leaning towards #2 as the more obviously correct fix, it makes
>> tty_set_ldisc() transactional, the fix seems limited in scope to
>> tty_set_ldisc() itself, and we don't need to make every other bit of
>> code that uses tty->ldisc handle the NULL case.
>
> That sounds reasonable to me, care to work on a patch for this?

Vegard, do you know how to do this?
That was first thing that I tried, but I did not manage to make it
work. disc is tied to tty, so it's not that one can create a fully
initialized disc on the side and then simply swap pointers. Looking at
the code now, there is at least TTY_LDISC_OPEN bit in tty. But as far
as I remember there were more fundamental problems. Or maybe I just
did not try too hard.

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Vegard Nossum]

On 2 May 2017 at 18:35, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Fri, Apr 14, 2017 at 2:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Fri, Apr 14, 2017 at 11:41:26AM +0200, Vegard Nossum wrote:
>>> On 13 April 2017 at 20:34, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> > On Thu, Apr 13, 2017 at 09:07:40AM -0700, Linus Torvalds wrote:
>>> >> On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>>> So the original problem is that the vmalloc() in n_tty_open() can
>>> fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
>>> because of its unwillingness to proceed if the tty doesn't have an
>>> ldisc.
>>>
>>> Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
>>> allocation failure as we can see from the comment in tty_set_ldisc().
>>>
>>> Unfortunately, it would appear that some other bits of code do not
>>> like tty->ldisc == NULL (other than the crash in this thread, I saw
>>> 2-3 similar crashes in other functions, e.g. poll()). I see two
>>> possibilities:
>>>
>>> 1) make other code handle tty->ldisc == NULL.
>>>
>>> 2) don't close/free the old ldisc until the new one has been
>>> successfully created/initialised/opened/attached to the tty, and
>>> return an error to userspace if changing it failed.
>>>
>>> I'm leaning towards #2 as the more obviously correct fix, it makes
>>> tty_set_ldisc() transactional, the fix seems limited in scope to
>>> tty_set_ldisc() itself, and we don't need to make every other bit of
>>> code that uses tty->ldisc handle the NULL case.
>>
>> That sounds reasonable to me, care to work on a patch for this?
>
> Vegard, do you know how to do this?
> That was first thing that I tried, but I did not manage to make it
> work. disc is tied to tty, so it's not that one can create a fully
> initialized disc on the side and then simply swap pointers. Looking at
> the code now, there is at least TTY_LDISC_OPEN bit in tty. But as far
> as I remember there were more fundamental problems. Or maybe I just
> did not try too hard.

I had a look at it but like you said, the tty/ldisc relationship is
complicated :-/

Maybe we can split up ldisc initialisation into two methods so that
the first one (e.g. ->alloc) does all the allocation and is allowed to
fail and the second one (e.g. ->open) is not allowed to fail. Then you
can allocate a new ldisc without freeing the old one and only swap
them over if the allocation succeeded.

That would require fixing up ->open for all the ldisc drivers though,
I'm not sure how easy/feasible it is.

I'll think about possible solutions, but I have no prior experience
with the tty code. In the meantime syzkaller also hit a couple of
other fun tty/pty bugs including a write/ioctl race that results in
buffer overflow :-/


Vegard

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Dmitry Vyukov]

On Tue, May 2, 2017 at 11:52 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>>>> So the original problem is that the vmalloc() in n_tty_open() can
>>>> fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
>>>> because of its unwillingness to proceed if the tty doesn't have an
>>>> ldisc.
>>>>
>>>> Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
>>>> allocation failure as we can see from the comment in tty_set_ldisc().
>>>>
>>>> Unfortunately, it would appear that some other bits of code do not
>>>> like tty->ldisc == NULL (other than the crash in this thread, I saw
>>>> 2-3 similar crashes in other functions, e.g. poll()). I see two
>>>> possibilities:
>>>>
>>>> 1) make other code handle tty->ldisc == NULL.
>>>>
>>>> 2) don't close/free the old ldisc until the new one has been
>>>> successfully created/initialised/opened/attached to the tty, and
>>>> return an error to userspace if changing it failed.
>>>>
>>>> I'm leaning towards #2 as the more obviously correct fix, it makes
>>>> tty_set_ldisc() transactional, the fix seems limited in scope to
>>>> tty_set_ldisc() itself, and we don't need to make every other bit of
>>>> code that uses tty->ldisc handle the NULL case.
>>>
>>> That sounds reasonable to me, care to work on a patch for this?
>>
>> Vegard, do you know how to do this?
>> That was first thing that I tried, but I did not manage to make it
>> work. disc is tied to tty, so it's not that one can create a fully
>> initialized disc on the side and then simply swap pointers. Looking at
>> the code now, there is at least TTY_LDISC_OPEN bit in tty. But as far
>> as I remember there were more fundamental problems. Or maybe I just
>> did not try too hard.
>
> I had a look at it but like you said, the tty/ldisc relationship is
> complicated :-/
>
> Maybe we can split up ldisc initialisation into two methods so that
> the first one (e.g. ->alloc) does all the allocation and is allowed to
> fail and the second one (e.g. ->open) is not allowed to fail. Then you
> can allocate a new ldisc without freeing the old one and only swap
> them over if the allocation succeeded.
>
> That would require fixing up ->open for all the ldisc drivers though,
> I'm not sure how easy/feasible it is.


What do you think about making all tty code deal with NULL disc?
It seems that most of code is already prepared for this.


> I'll think about possible solutions, but I have no prior experience
> with the tty code. In the meantime syzkaller also hit a couple of
> other fun tty/pty bugs including a write/ioctl race that results in
> buffer overflow :-/
>
>
> Vegard

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Greg KH]

On Tue, May 02, 2017 at 11:52:40PM +0200, Vegard Nossum wrote:
> On 2 May 2017 at 18:35, Dmitry Vyukov <dvyukov@google.com> wrote:
> > On Fri, Apr 14, 2017 at 2:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> On Fri, Apr 14, 2017 at 11:41:26AM +0200, Vegard Nossum wrote:
> >>> On 13 April 2017 at 20:34, Greg KH <gregkh@linuxfoundation.org> wrote:
> >>> > On Thu, Apr 13, 2017 at 09:07:40AM -0700, Linus Torvalds wrote:
> >>> >> On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> >>> So the original problem is that the vmalloc() in n_tty_open() can
> >>> fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
> >>> because of its unwillingness to proceed if the tty doesn't have an
> >>> ldisc.
> >>>
> >>> Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
> >>> allocation failure as we can see from the comment in tty_set_ldisc().
> >>>
> >>> Unfortunately, it would appear that some other bits of code do not
> >>> like tty->ldisc == NULL (other than the crash in this thread, I saw
> >>> 2-3 similar crashes in other functions, e.g. poll()). I see two
> >>> possibilities:
> >>>
> >>> 1) make other code handle tty->ldisc == NULL.
> >>>
> >>> 2) don't close/free the old ldisc until the new one has been
> >>> successfully created/initialised/opened/attached to the tty, and
> >>> return an error to userspace if changing it failed.
> >>>
> >>> I'm leaning towards #2 as the more obviously correct fix, it makes
> >>> tty_set_ldisc() transactional, the fix seems limited in scope to
> >>> tty_set_ldisc() itself, and we don't need to make every other bit of
> >>> code that uses tty->ldisc handle the NULL case.
> >>
> >> That sounds reasonable to me, care to work on a patch for this?
> >
> > Vegard, do you know how to do this?
> > That was first thing that I tried, but I did not manage to make it
> > work. disc is tied to tty, so it's not that one can create a fully
> > initialized disc on the side and then simply swap pointers. Looking at
> > the code now, there is at least TTY_LDISC_OPEN bit in tty. But as far
> > as I remember there were more fundamental problems. Or maybe I just
> > did not try too hard.
> 
> I had a look at it but like you said, the tty/ldisc relationship is
> complicated :-/
> 
> Maybe we can split up ldisc initialisation into two methods so that
> the first one (e.g. ->alloc) does all the allocation and is allowed to
> fail and the second one (e.g. ->open) is not allowed to fail. Then you
> can allocate a new ldisc without freeing the old one and only swap
> them over if the allocation succeeded.
> 
> That would require fixing up ->open for all the ldisc drivers though,
> I'm not sure how easy/feasible it is.

We don't have that many ldisc drivers, so it shouldn't be that hard to
change to use this.  It makes a lot more sense to fix this the correct
way like this.

> I'll think about possible solutions, but I have no prior experience
> with the tty code. In the meantime syzkaller also hit a couple of
> other fun tty/pty bugs including a write/ioctl race that results in
> buffer overflow :-/

Ugh, let me know what they are and we'll work to fix them.

Thanks for all of the work you all are doing in finding these issues, I
might grumble about having to fix this and what a pain it is, but it's
just me being grumpy about the tty code, not your effort.  Your effort
is much appreciated.

thanks,

greg k-h

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Dmitry Vyukov]

On Wed, May 3, 2017 at 2:01 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >>> So the original problem is that the vmalloc() in n_tty_open() can
>> >>> fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
>> >>> because of its unwillingness to proceed if the tty doesn't have an
>> >>> ldisc.
>> >>>
>> >>> Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
>> >>> allocation failure as we can see from the comment in tty_set_ldisc().
>> >>>
>> >>> Unfortunately, it would appear that some other bits of code do not
>> >>> like tty->ldisc == NULL (other than the crash in this thread, I saw
>> >>> 2-3 similar crashes in other functions, e.g. poll()). I see two
>> >>> possibilities:
>> >>>
>> >>> 1) make other code handle tty->ldisc == NULL.
>> >>>
>> >>> 2) don't close/free the old ldisc until the new one has been
>> >>> successfully created/initialised/opened/attached to the tty, and
>> >>> return an error to userspace if changing it failed.
>> >>>
>> >>> I'm leaning towards #2 as the more obviously correct fix, it makes
>> >>> tty_set_ldisc() transactional, the fix seems limited in scope to
>> >>> tty_set_ldisc() itself, and we don't need to make every other bit of
>> >>> code that uses tty->ldisc handle the NULL case.
>> >>
>> >> That sounds reasonable to me, care to work on a patch for this?
>> >
>> > Vegard, do you know how to do this?
>> > That was first thing that I tried, but I did not manage to make it
>> > work. disc is tied to tty, so it's not that one can create a fully
>> > initialized disc on the side and then simply swap pointers. Looking at
>> > the code now, there is at least TTY_LDISC_OPEN bit in tty. But as far
>> > as I remember there were more fundamental problems. Or maybe I just
>> > did not try too hard.
>>
>> I had a look at it but like you said, the tty/ldisc relationship is
>> complicated :-/
>>
>> Maybe we can split up ldisc initialisation into two methods so that
>> the first one (e.g. ->alloc) does all the allocation and is allowed to
>> fail and the second one (e.g. ->open) is not allowed to fail. Then you
>> can allocate a new ldisc without freeing the old one and only swap
>> them over if the allocation succeeded.
>>
>> That would require fixing up ->open for all the ldisc drivers though,
>> I'm not sure how easy/feasible it is.
>
> We don't have that many ldisc drivers, so it shouldn't be that hard to
> change to use this.  It makes a lot more sense to fix this the correct
> way like this.


There are 26 drivers. This will require some careful surgery of them.
There is no way to test all of them without special hardware, right?
Also will require some changes to tty_ldisc.c.
So far I've filed https://github.com/google/syzkaller/issues/203 to
not lose track of it and collect all relevant details in one place.



>> I'll think about possible solutions, but I have no prior experience
>> with the tty code. In the meantime syzkaller also hit a couple of
>> other fun tty/pty bugs including a write/ioctl race that results in
>> buffer overflow :-/
>
> Ugh, let me know what they are and we'll work to fix them.
>
> Thanks for all of the work you all are doing in finding these issues, I
> might grumble about having to fix this and what a pain it is, but it's
> just me being grumpy about the tty code, not your effort.  Your effort
> is much appreciated.

Thanks, Greg. Nice to hear.

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Alan Cox]

> >> I'll think about possible solutions, but I have no prior experience
> >> with the tty code. In the meantime syzkaller also hit a couple of
> >> other fun tty/pty bugs including a write/ioctl race that results in
> >> buffer overflow :-/  

There are several of those, including some of that have been documented
for years but nobody ever volunteered to fix - in particular all the
interfaces that push characters to the tty other than via the normal
interrupt receive path are dodgy (console selection in particular)

The original tty model btw was that setting the ldisc to n_tty cannot
fail, and the structure allocated was smaller than a page size so was
safe.

The simple way to fix it is to restore that behaviour by adding a 'null'
ldisc that we can fail to instead of N_TTY since the N_TTY failback path
is long broken.

Something like this (untested)

commit 797035eaf800889287b0b176a11c89c0f1fbba30
Author: Alan Cox <alan@llwyncelyn.cymru>
Date:   Tue May 30 12:59:45 2017 +0100

    tty: handle the case where we cannot restore a line discipline
    
    Historically the N_TTY driver could never fail but this has become broken over
    time. Rather than trying to rewrite half the ldisc layer to fix the breakage
    introduce a second level of fallback with an N_NULL ldisc which cannot fail,
    and thus restore the guarantees required by the ldisc layer.
    
    We still try and fail to N_TTY first. It's much more useful to find yourself
    back in your old ldisc (first attempt) or in N_TTY (second attempt), and while
    I'm not aware of any code out there that makes those assumptions it's good to
    drive(r) defensively.
    
    No signed off by: this is just a proposal!

diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c
new file mode 100644
index 0000000..c5812cd
--- /dev/null
+++ b/drivers/tty/n_null.c
@@ -0,0 +1,67 @@
+/*
+ *  n_null.c - Null line discipline used in the failure path
+ *
+ *  Copyright (C) Intel 2017
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+static int n_null_open(struct tty_struct *tty)
+{
+	return 0;
+}
+
+static void n_null_close(struct tty_struct *tty)
+{
+}
+
+static ssize_t n_null_read(struct tty_struct *tty, struct file *file,
+			   unsigned char __user * buf, size_t nr)
+{
+	return -EOPNOTSUPP;
+}
+
+static ssize_t n_null_write(struct tty_struct *tty, struct file *file,
+			    const unsigned char *buf, size_t nr)
+{
+	return -EOPNOTSUPP;
+}
+
+static ssize_t n_null_receivebuf(struct tty_struct *tty,
+				 const unsigned char *cp, char *fp,
+				 int cnt)
+{
+}
+
+static struct tty_ldisc_ops null_ldisc {
+	.owner		=	THIS_MODULE,
+	.magic		=	TTY_LDISC_MAGIC,
+	.name		=	"n_null",
+	.open		=	n_null_open,
+	.close		=	n_null_close,
+	.read		=	n_null_read,
+	.write		=	n_null_write,
+	.receive_buf	=	n_null_receivebuf
+};
+
+static int __init n_null_init(void)
+{
+	BUG_ON(tty_register_ldisc(N_NULL, &null_ldisc));
+	return 0;
+}
+
+static void __exit n_null_exit(void)
+{
+	tty_unregister_ldisc(N_NULL);
+}
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index f6ffe28..e80e05f 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -492,6 +492,29 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
 }
 
 /**
+ *	tty_ldisc_failto	-	helper for ldisc failback
+ *	@tty: tty to open the ldisc on
+ *	@ld: ldisc we are trying to fail back to
+ *
+ *	Helper to try and recover a tty when switching back to the old
+ *	ldisc fails and we need something attached.
+ */
+
+static int tty_ldisc_failto(struct tty_struct *tty, int ld)
+{
+	struct tty_ldisc *disc = tty_ldisc_get(tty, ld);
+	int r;
+
+	if (IS_ERR(disc))
+		return PTR_ERR(disc);
+	tty->ldisc = disc;
+	tty_set_termios_ldisc(tty, ld);
+	if ((r = tty_ldisc_open(tty, disc)) < 0)
+		tty_ldisc_put(ld);
+	return r;
+}
+
+/**
  *	tty_ldisc_restore	-	helper for tty ldisc change
  *	@tty: tty to recover
  *	@old: previous ldisc
@@ -512,15 +535,14 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
 	tty_set_termios_ldisc(tty, old->ops->num);
 	if (tty_ldisc_open(tty, old) < 0) {
 		tty_ldisc_put(old);
-		/* This driver is always present */
-		new_ldisc = tty_ldisc_get(tty, N_TTY);
-		if (IS_ERR(new_ldisc))
-			panic("n_tty: get");
-		tty->ldisc = new_ldisc;
-		tty_set_termios_ldisc(tty, N_TTY);
-		r = tty_ldisc_open(tty, new_ldisc);
+		/* The traditional behaviour is to fall back to N_TTY, we
+		   want to avoid falling back to N_NULL unless we have no
+		   choice to avoid the risk of breaking anything */
+		if (tty_ldisc_failto(tty, N_TTY) < 0 &&
+		    tty_ldisc_failto(tty, N_NULL) < 0)
+		/* Fall back to null ldisc */
 		if (r < 0)
-			panic("Couldn't open N_TTY ldisc for "
+			panic("Couldn't open N_NULL ldisc for "
 			      "%s --- error %d.",
 			      tty_name(tty), r);
 	}
diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
index e7855df..cf14553 100644
--- a/include/uapi/linux/tty.h
+++ b/include/uapi/linux/tty.h
@@ -36,5 +36,6 @@
 #define N_TRACEROUTER	24	/* Trace data routing for MIPI P1149.7 */
 #define N_NCI		25	/* NFC NCI UART */
 #define N_SPEAKUP	26	/* Speakup communication with synths */
+#define N_NULL		27	/* Null ldisc used for error handling */
 
 #endif /* _UAPI_LINUX_TTY_H */

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Dmitry Vyukov]

On Tue, May 30, 2017 at 2:09 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
>> >> I'll think about possible solutions, but I have no prior experience
>> >> with the tty code. In the meantime syzkaller also hit a couple of
>> >> other fun tty/pty bugs including a write/ioctl race that results in
>> >> buffer overflow :-/
>
> There are several of those, including some of that have been documented
> for years but nobody ever volunteered to fix - in particular all the
> interfaces that push characters to the tty other than via the normal
> interrupt receive path are dodgy (console selection in particular)
>
> The original tty model btw was that setting the ldisc to n_tty cannot
> fail, and the structure allocated was smaller than a page size so was
> safe.
>
> The simple way to fix it is to restore that behaviour by adding a 'null'
> ldisc that we can fail to instead of N_TTY since the N_TTY failback path
> is long broken.

Greg, what do you think about this patch? Are you ready to accept
something like this?
Definitely shorter than changing all drivers.

> Something like this (untested)
>
> commit 797035eaf800889287b0b176a11c89c0f1fbba30
> Author: Alan Cox <alan@llwyncelyn.cymru>
> Date:   Tue May 30 12:59:45 2017 +0100
>
>     tty: handle the case where we cannot restore a line discipline
>
>     Historically the N_TTY driver could never fail but this has become broken over
>     time. Rather than trying to rewrite half the ldisc layer to fix the breakage
>     introduce a second level of fallback with an N_NULL ldisc which cannot fail,
>     and thus restore the guarantees required by the ldisc layer.
>
>     We still try and fail to N_TTY first. It's much more useful to find yourself
>     back in your old ldisc (first attempt) or in N_TTY (second attempt), and while
>     I'm not aware of any code out there that makes those assumptions it's good to
>     drive(r) defensively.
>
>     No signed off by: this is just a proposal!
>
> diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c
> new file mode 100644
> index 0000000..c5812cd
> --- /dev/null
> +++ b/drivers/tty/n_null.c
> @@ -0,0 +1,67 @@
> +/*
> + *  n_null.c - Null line discipline used in the failure path
> + *
> + *  Copyright (C) Intel 2017
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2
> + *  as published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +static int n_null_open(struct tty_struct *tty)
> +{
> +       return 0;
> +}
> +
> +static void n_null_close(struct tty_struct *tty)
> +{
> +}
> +
> +static ssize_t n_null_read(struct tty_struct *tty, struct file *file,
> +                          unsigned char __user * buf, size_t nr)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static ssize_t n_null_write(struct tty_struct *tty, struct file *file,
> +                           const unsigned char *buf, size_t nr)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static ssize_t n_null_receivebuf(struct tty_struct *tty,
> +                                const unsigned char *cp, char *fp,
> +                                int cnt)
> +{
> +}
> +
> +static struct tty_ldisc_ops null_ldisc {
> +       .owner          =       THIS_MODULE,
> +       .magic          =       TTY_LDISC_MAGIC,
> +       .name           =       "n_null",
> +       .open           =       n_null_open,
> +       .close          =       n_null_close,
> +       .read           =       n_null_read,
> +       .write          =       n_null_write,
> +       .receive_buf    =       n_null_receivebuf
> +};
> +
> +static int __init n_null_init(void)
> +{
> +       BUG_ON(tty_register_ldisc(N_NULL, &null_ldisc));
> +       return 0;
> +}
> +
> +static void __exit n_null_exit(void)
> +{
> +       tty_unregister_ldisc(N_NULL);
> +}
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index f6ffe28..e80e05f 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -492,6 +492,29 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
>  }
>
>  /**
> + *     tty_ldisc_failto        -       helper for ldisc failback
> + *     @tty: tty to open the ldisc on
> + *     @ld: ldisc we are trying to fail back to
> + *
> + *     Helper to try and recover a tty when switching back to the old
> + *     ldisc fails and we need something attached.
> + */
> +
> +static int tty_ldisc_failto(struct tty_struct *tty, int ld)
> +{
> +       struct tty_ldisc *disc = tty_ldisc_get(tty, ld);
> +       int r;
> +
> +       if (IS_ERR(disc))
> +               return PTR_ERR(disc);
> +       tty->ldisc = disc;
> +       tty_set_termios_ldisc(tty, ld);
> +       if ((r = tty_ldisc_open(tty, disc)) < 0)
> +               tty_ldisc_put(ld);
> +       return r;
> +}
> +
> +/**
>   *     tty_ldisc_restore       -       helper for tty ldisc change
>   *     @tty: tty to recover
>   *     @old: previous ldisc
> @@ -512,15 +535,14 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
>         tty_set_termios_ldisc(tty, old->ops->num);
>         if (tty_ldisc_open(tty, old) < 0) {
>                 tty_ldisc_put(old);
> -               /* This driver is always present */
> -               new_ldisc = tty_ldisc_get(tty, N_TTY);
> -               if (IS_ERR(new_ldisc))
> -                       panic("n_tty: get");
> -               tty->ldisc = new_ldisc;
> -               tty_set_termios_ldisc(tty, N_TTY);
> -               r = tty_ldisc_open(tty, new_ldisc);
> +               /* The traditional behaviour is to fall back to N_TTY, we
> +                  want to avoid falling back to N_NULL unless we have no
> +                  choice to avoid the risk of breaking anything */
> +               if (tty_ldisc_failto(tty, N_TTY) < 0 &&
> +                   tty_ldisc_failto(tty, N_NULL) < 0)
> +               /* Fall back to null ldisc */
>                 if (r < 0)
> -                       panic("Couldn't open N_TTY ldisc for "
> +                       panic("Couldn't open N_NULL ldisc for "
>                               "%s --- error %d.",
>                               tty_name(tty), r);
>         }
> diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
> index e7855df..cf14553 100644
> --- a/include/uapi/linux/tty.h
> +++ b/include/uapi/linux/tty.h
> @@ -36,5 +36,6 @@
>  #define N_TRACEROUTER  24      /* Trace data routing for MIPI P1149.7 */
>  #define N_NCI          25      /* NFC NCI UART */
>  #define N_SPEAKUP      26      /* Speakup communication with synths */
> +#define N_NULL         27      /* Null ldisc used for error handling */
>
>  #endif /* _UAPI_LINUX_TTY_H */

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Greg KH]

On Wed, May 31, 2017 at 10:39:23AM +0200, Dmitry Vyukov wrote:
> On Tue, May 30, 2017 at 2:09 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> >> I'll think about possible solutions, but I have no prior experience
> >> >> with the tty code. In the meantime syzkaller also hit a couple of
> >> >> other fun tty/pty bugs including a write/ioctl race that results in
> >> >> buffer overflow :-/
> >
> > There are several of those, including some of that have been documented
> > for years but nobody ever volunteered to fix - in particular all the
> > interfaces that push characters to the tty other than via the normal
> > interrupt receive path are dodgy (console selection in particular)
> >
> > The original tty model btw was that setting the ldisc to n_tty cannot
> > fail, and the structure allocated was smaller than a page size so was
> > safe.
> >
> > The simple way to fix it is to restore that behaviour by adding a 'null'
> > ldisc that we can fail to instead of N_TTY since the N_TTY failback path
> > is long broken.
> 
> Greg, what do you think about this patch? Are you ready to accept
> something like this?
> Definitely shorter than changing all drivers.

Yes, it looks reasonable to me.

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Alan Cox]

On Wed, 31 May 2017 20:16:12 +0900
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, May 31, 2017 at 10:39:23AM +0200, Dmitry Vyukov wrote:
> > On Tue, May 30, 2017 at 2:09 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:  
> > >> >> I'll think about possible solutions, but I have no prior experience
> > >> >> with the tty code. In the meantime syzkaller also hit a couple of
> > >> >> other fun tty/pty bugs including a write/ioctl race that results in
> > >> >> buffer overflow :-/  
> > >
> > > There are several of those, including some of that have been documented
> > > for years but nobody ever volunteered to fix - in particular all the
> > > interfaces that push characters to the tty other than via the normal
> > > interrupt receive path are dodgy (console selection in particular)
> > >
> > > The original tty model btw was that setting the ldisc to n_tty cannot
> > > fail, and the structure allocated was smaller than a page size so was
> > > safe.
> > >
> > > The simple way to fix it is to restore that behaviour by adding a 'null'
> > > ldisc that we can fail to instead of N_TTY since the N_TTY failback path
> > > is long broken.  
> > 
> > Greg, what do you think about this patch? Are you ready to accept
> > something like this?
> > Definitely shorter than changing all drivers.  
> 
> Yes, it looks reasonable to me.



Ok try this

commit f6db8de7eca11cfeafa92f2ec866fa75425c5f38
Author: Alan Cox <alan@llwyncelyn.cymru>
Date:   Tue May 30 12:59:45 2017 +0100

    tty: handle the case where we cannot restore a line discipline
    
    Historically the N_TTY driver could never fail but this has become broken over
    time. Rather than trying to rewrite half the ldisc layer to fix the breakage
    introduce a second level of fallback with an N_NULL ldisc which cannot fail,
    and thus restore the guarantees required by the ldisc layer.
    
    We still try and fail to N_TTY first. It's much more useful to find yourself
    back in your old ldisc (first attempt) or in N_TTY (second attempt), and while
    I'm not aware of any code out there that makes those assumptions it's good to
    drive(r) defensively.

diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index f02becd..8689279 100644
--- a/drivers/tty/Makefile
+++ b/drivers/tty/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_TTY)		+= tty_io.o n_tty.o tty_ioctl.o tty_ldisc.o \
 				   tty_buffer.o tty_port.o tty_mutex.o \
-				   tty_ldsem.o tty_baudrate.o tty_jobctrl.o
+				   tty_ldsem.o tty_baudrate.o tty_jobctrl.o \
+				   n_null.o
 obj-$(CONFIG_LEGACY_PTYS)	+= pty.o
 obj-$(CONFIG_UNIX98_PTYS)	+= pty.o
 obj-$(CONFIG_AUDIT)		+= tty_audit.o
diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c
new file mode 100644
index 0000000..d63261c
--- /dev/null
+++ b/drivers/tty/n_null.c
@@ -0,0 +1,80 @@
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/tty.h>
+#include <linux/module.h>
+
+/*
+ *  n_null.c - Null line discipline used in the failure path
+ *
+ *  Copyright (C) Intel 2017
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+static int n_null_open(struct tty_struct *tty)
+{
+	return 0;
+}
+
+static void n_null_close(struct tty_struct *tty)
+{
+}
+
+static ssize_t n_null_read(struct tty_struct *tty, struct file *file,
+			   unsigned char __user * buf, size_t nr)
+{
+	return -EOPNOTSUPP;
+}
+
+static ssize_t n_null_write(struct tty_struct *tty, struct file *file,
+			    const unsigned char *buf, size_t nr)
+{
+	return -EOPNOTSUPP;
+}
+
+static void n_null_receivebuf(struct tty_struct *tty,
+				 const unsigned char *cp, char *fp,
+				 int cnt)
+{
+}
+
+static struct tty_ldisc_ops null_ldisc = {
+	.owner		=	THIS_MODULE,
+	.magic		=	TTY_LDISC_MAGIC,
+	.name		=	"n_null",
+	.open		=	n_null_open,
+	.close		=	n_null_close,
+	.read		=	n_null_read,
+	.write		=	n_null_write,
+	.receive_buf	=	n_null_receivebuf
+};
+
+static int __init n_null_init(void)
+{
+	BUG_ON(tty_register_ldisc(N_NULL, &null_ldisc));
+	return 0;
+}
+
+static void __exit n_null_exit(void)
+{
+	tty_unregister_ldisc(N_NULL);
+}
+
+module_init(n_null_init);
+module_exit(n_null_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alan Cox");
+MODULE_ALIAS_LDISC(N_NULL);
+MODULE_DESCRIPTION("Null ldisc driver");
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index f6ffe28..2fe216b 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -492,6 +492,29 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
 }
 
 /**
+ *	tty_ldisc_failto	-	helper for ldisc failback
+ *	@tty: tty to open the ldisc on
+ *	@ld: ldisc we are trying to fail back to
+ *
+ *	Helper to try and recover a tty when switching back to the old
+ *	ldisc fails and we need something attached.
+ */
+
+static int tty_ldisc_failto(struct tty_struct *tty, int ld)
+{
+	struct tty_ldisc *disc = tty_ldisc_get(tty, ld);
+	int r;
+
+	if (IS_ERR(disc))
+		return PTR_ERR(disc);
+	tty->ldisc = disc;
+	tty_set_termios_ldisc(tty, ld);
+	if ((r = tty_ldisc_open(tty, disc)) < 0)
+		tty_ldisc_put(disc);
+	return r;
+}
+
+/**
  *	tty_ldisc_restore	-	helper for tty ldisc change
  *	@tty: tty to recover
  *	@old: previous ldisc
@@ -502,9 +525,6 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
 
 static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
 {
-	struct tty_ldisc *new_ldisc;
-	int r;
-
 	/* There is an outstanding reference here so this is safe */
 	old = tty_ldisc_get(tty, old->ops->num);
 	WARN_ON(IS_ERR(old));
@@ -512,17 +532,13 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
 	tty_set_termios_ldisc(tty, old->ops->num);
 	if (tty_ldisc_open(tty, old) < 0) {
 		tty_ldisc_put(old);
-		/* This driver is always present */
-		new_ldisc = tty_ldisc_get(tty, N_TTY);
-		if (IS_ERR(new_ldisc))
-			panic("n_tty: get");
-		tty->ldisc = new_ldisc;
-		tty_set_termios_ldisc(tty, N_TTY);
-		r = tty_ldisc_open(tty, new_ldisc);
-		if (r < 0)
-			panic("Couldn't open N_TTY ldisc for "
-			      "%s --- error %d.",
-			      tty_name(tty), r);
+		/* The traditional behaviour is to fall back to N_TTY, we
+		   want to avoid falling back to N_NULL unless we have no
+		   choice to avoid the risk of breaking anything */
+		if (tty_ldisc_failto(tty, N_TTY) < 0 &&
+		    tty_ldisc_failto(tty, N_NULL) < 0)
+			panic("Couldn't open N_NULL ldisc for %s.",
+			      tty_name(tty));
 	}
 }
 
diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
index e7855df..cf14553 100644
--- a/include/uapi/linux/tty.h
+++ b/include/uapi/linux/tty.h
@@ -36,5 +36,6 @@
 #define N_TRACEROUTER	24	/* Trace data routing for MIPI P1149.7 */
 #define N_NCI		25	/* NFC NCI UART */
 #define N_SPEAKUP	26	/* Speakup communication with synths */
+#define N_NULL		27	/* Null ldisc used for error handling */
 
 #endif /* _UAPI_LINUX_TTY_H */

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Dmitry Vyukov]

On Wed, May 31, 2017 at 5:04 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> On Wed, 31 May 2017 20:16:12 +0900
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
>> On Wed, May 31, 2017 at 10:39:23AM +0200, Dmitry Vyukov wrote:
>> > On Tue, May 30, 2017 at 2:09 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
>> > >> >> I'll think about possible solutions, but I have no prior experience
>> > >> >> with the tty code. In the meantime syzkaller also hit a couple of
>> > >> >> other fun tty/pty bugs including a write/ioctl race that results in
>> > >> >> buffer overflow :-/
>> > >
>> > > There are several of those, including some of that have been documented
>> > > for years but nobody ever volunteered to fix - in particular all the
>> > > interfaces that push characters to the tty other than via the normal
>> > > interrupt receive path are dodgy (console selection in particular)
>> > >
>> > > The original tty model btw was that setting the ldisc to n_tty cannot
>> > > fail, and the structure allocated was smaller than a page size so was
>> > > safe.
>> > >
>> > > The simple way to fix it is to restore that behaviour by adding a 'null'
>> > > ldisc that we can fail to instead of N_TTY since the N_TTY failback path
>> > > is long broken.
>> >
>> > Greg, what do you think about this patch? Are you ready to accept
>> > something like this?
>> > Definitely shorter than changing all drivers.
>>
>> Yes, it looks reasonable to me.
>
>
>
> Ok try this


I've applied the patch and run syzkaller with it. I don't see kernel
panics in tty_ldisc_restore any more. Also don't see any new
tty-related crashes.

Greg, will you take it from here?


> commit f6db8de7eca11cfeafa92f2ec866fa75425c5f38
> Author: Alan Cox <alan@llwyncelyn.cymru>
> Date:   Tue May 30 12:59:45 2017 +0100
>
>     tty: handle the case where we cannot restore a line discipline
>
>     Historically the N_TTY driver could never fail but this has become broken over
>     time. Rather than trying to rewrite half the ldisc layer to fix the breakage
>     introduce a second level of fallback with an N_NULL ldisc which cannot fail,
>     and thus restore the guarantees required by the ldisc layer.
>
>     We still try and fail to N_TTY first. It's much more useful to find yourself
>     back in your old ldisc (first attempt) or in N_TTY (second attempt), and while
>     I'm not aware of any code out there that makes those assumptions it's good to
>     drive(r) defensively.
>
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index f02becd..8689279 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_TTY)              += tty_io.o n_tty.o tty_ioctl.o tty_ldisc.o \
>                                    tty_buffer.o tty_port.o tty_mutex.o \
> -                                  tty_ldsem.o tty_baudrate.o tty_jobctrl.o
> +                                  tty_ldsem.o tty_baudrate.o tty_jobctrl.o \
> +                                  n_null.o
>  obj-$(CONFIG_LEGACY_PTYS)      += pty.o
>  obj-$(CONFIG_UNIX98_PTYS)      += pty.o
>  obj-$(CONFIG_AUDIT)            += tty_audit.o
> diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c
> new file mode 100644
> index 0000000..d63261c
> --- /dev/null
> +++ b/drivers/tty/n_null.c
> @@ -0,0 +1,80 @@
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/tty.h>
> +#include <linux/module.h>
> +
> +/*
> + *  n_null.c - Null line discipline used in the failure path
> + *
> + *  Copyright (C) Intel 2017
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2
> + *  as published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +static int n_null_open(struct tty_struct *tty)
> +{
> +       return 0;
> +}
> +
> +static void n_null_close(struct tty_struct *tty)
> +{
> +}
> +
> +static ssize_t n_null_read(struct tty_struct *tty, struct file *file,
> +                          unsigned char __user * buf, size_t nr)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static ssize_t n_null_write(struct tty_struct *tty, struct file *file,
> +                           const unsigned char *buf, size_t nr)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static void n_null_receivebuf(struct tty_struct *tty,
> +                                const unsigned char *cp, char *fp,
> +                                int cnt)
> +{
> +}
> +
> +static struct tty_ldisc_ops null_ldisc = {
> +       .owner          =       THIS_MODULE,
> +       .magic          =       TTY_LDISC_MAGIC,
> +       .name           =       "n_null",
> +       .open           =       n_null_open,
> +       .close          =       n_null_close,
> +       .read           =       n_null_read,
> +       .write          =       n_null_write,
> +       .receive_buf    =       n_null_receivebuf
> +};
> +
> +static int __init n_null_init(void)
> +{
> +       BUG_ON(tty_register_ldisc(N_NULL, &null_ldisc));
> +       return 0;
> +}
> +
> +static void __exit n_null_exit(void)
> +{
> +       tty_unregister_ldisc(N_NULL);
> +}
> +
> +module_init(n_null_init);
> +module_exit(n_null_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Alan Cox");
> +MODULE_ALIAS_LDISC(N_NULL);
> +MODULE_DESCRIPTION("Null ldisc driver");
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index f6ffe28..2fe216b 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -492,6 +492,29 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
>  }
>
>  /**
> + *     tty_ldisc_failto        -       helper for ldisc failback
> + *     @tty: tty to open the ldisc on
> + *     @ld: ldisc we are trying to fail back to
> + *
> + *     Helper to try and recover a tty when switching back to the old
> + *     ldisc fails and we need something attached.
> + */
> +
> +static int tty_ldisc_failto(struct tty_struct *tty, int ld)
> +{
> +       struct tty_ldisc *disc = tty_ldisc_get(tty, ld);
> +       int r;
> +
> +       if (IS_ERR(disc))
> +               return PTR_ERR(disc);
> +       tty->ldisc = disc;
> +       tty_set_termios_ldisc(tty, ld);
> +       if ((r = tty_ldisc_open(tty, disc)) < 0)
> +               tty_ldisc_put(disc);
> +       return r;
> +}
> +
> +/**
>   *     tty_ldisc_restore       -       helper for tty ldisc change
>   *     @tty: tty to recover
>   *     @old: previous ldisc
> @@ -502,9 +525,6 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
>
>  static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
>  {
> -       struct tty_ldisc *new_ldisc;
> -       int r;
> -
>         /* There is an outstanding reference here so this is safe */
>         old = tty_ldisc_get(tty, old->ops->num);
>         WARN_ON(IS_ERR(old));
> @@ -512,17 +532,13 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
>         tty_set_termios_ldisc(tty, old->ops->num);
>         if (tty_ldisc_open(tty, old) < 0) {
>                 tty_ldisc_put(old);
> -               /* This driver is always present */
> -               new_ldisc = tty_ldisc_get(tty, N_TTY);
> -               if (IS_ERR(new_ldisc))
> -                       panic("n_tty: get");
> -               tty->ldisc = new_ldisc;
> -               tty_set_termios_ldisc(tty, N_TTY);
> -               r = tty_ldisc_open(tty, new_ldisc);
> -               if (r < 0)
> -                       panic("Couldn't open N_TTY ldisc for "
> -                             "%s --- error %d.",
> -                             tty_name(tty), r);
> +               /* The traditional behaviour is to fall back to N_TTY, we
> +                  want to avoid falling back to N_NULL unless we have no
> +                  choice to avoid the risk of breaking anything */
> +               if (tty_ldisc_failto(tty, N_TTY) < 0 &&
> +                   tty_ldisc_failto(tty, N_NULL) < 0)
> +                       panic("Couldn't open N_NULL ldisc for %s.",
> +                             tty_name(tty));
>         }
>  }
>
> diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
> index e7855df..cf14553 100644
> --- a/include/uapi/linux/tty.h
> +++ b/include/uapi/linux/tty.h
> @@ -36,5 +36,6 @@
>  #define N_TRACEROUTER  24      /* Trace data routing for MIPI P1149.7 */
>  #define N_NCI          25      /* NFC NCI UART */
>  #define N_SPEAKUP      26      /* Speakup communication with synths */
> +#define N_NULL         27      /* Null ldisc used for error handling */
>
>  #endif /* _UAPI_LINUX_TTY_H */

Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4

[Greg KH]

On Thu, Jun 01, 2017 at 02:06:08PM +0200, Dmitry Vyukov wrote:
> On Wed, May 31, 2017 at 5:04 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> > On Wed, 31 May 2017 20:16:12 +0900
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> >> On Wed, May 31, 2017 at 10:39:23AM +0200, Dmitry Vyukov wrote:
> >> > On Tue, May 30, 2017 at 2:09 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> > >> >> I'll think about possible solutions, but I have no prior experience
> >> > >> >> with the tty code. In the meantime syzkaller also hit a couple of
> >> > >> >> other fun tty/pty bugs including a write/ioctl race that results in
> >> > >> >> buffer overflow :-/
> >> > >
> >> > > There are several of those, including some of that have been documented
> >> > > for years but nobody ever volunteered to fix - in particular all the
> >> > > interfaces that push characters to the tty other than via the normal
> >> > > interrupt receive path are dodgy (console selection in particular)
> >> > >
> >> > > The original tty model btw was that setting the ldisc to n_tty cannot
> >> > > fail, and the structure allocated was smaller than a page size so was
> >> > > safe.
> >> > >
> >> > > The simple way to fix it is to restore that behaviour by adding a 'null'
> >> > > ldisc that we can fail to instead of N_TTY since the N_TTY failback path
> >> > > is long broken.
> >> >
> >> > Greg, what do you think about this patch? Are you ready to accept
> >> > something like this?
> >> > Definitely shorter than changing all drivers.
> >>
> >> Yes, it looks reasonable to me.
> >
> >
> >
> > Ok try this
> 
> 
> I've applied the patch and run syzkaller with it. I don't see kernel
> panics in tty_ldisc_restore any more. Also don't see any new
> tty-related crashes.
> 
> Greg, will you take it from here?

I can if Alan sends it to me in a form I can apply it in (i.e. it has a
siged-off-by line...)

thanks,

greg k-h