[BUG] tiocsti() NULL dereference if ld->ops->receive_buf==NULL

[BUG] tiocsti() NULL dereference if ld->ops->receive_buf==NULL

[Jann Horn]

Hi!

When a line discipline doesn't have a ->receive_buf handler, tiocsti()
attempts to call a NULL pointer. Both tty_n_tracesink and
spk_ttyio_ldisc_ops don't have such a handler.

To reproduce, build a kernel with CONFIG_SPEAKUP=y and
CONFIG_SPEAKUP_SYNTH_SOFT=y, set speakup.synth=soft in the kernel
command line, and run the following code as root:

=================
#include <fcntl.h>
#include <sys/ioctl.h>
#include <stdio.h>
#include <err.h>

void do_ioctl(int fd, unsigned long cmd, void *arg) {
  int res = ioctl(fd, cmd, arg);
  if (res < 0) {
    perror("ioctl");
  } else {
    printf("result %d\n", res);
  }
}

int main(void) {
  int fd = open("/dev/tty1", O_RDWR);
  if (fd == -1) err(1, "open");

  int disc = 26/*N_SPEAKUP*/;
  do_ioctl(fd, TIOCSETD, &disc);

  char c = 'A';
  do_ioctl(fd, TIOCSTI, &c);

  return 0;
}
=================

You should get this splat:
=================
[  124.609859] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000000
[  124.613510] #PF error: [INSTR]
[  124.614545] PGD 0 P4D 0
[  124.615249] Oops: 0010 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN
[...]
[  124.620994] RIP: 0010:          (null)
[  124.622016] Code: Bad RIP value.
[  124.622884] RSP: 0018:ffff8881d6c37ae0 EFLAGS: 00010246
[...]
[  124.643743] Call Trace:
[  124.644428]  ? tty_ioctl+0x5ec/0xbd0
[  124.645404]  ? tty_vhangup+0x20/0x20
[  124.646380]  ? preempt_count_add+0x8d/0xe0
[  124.647476]  ? _raw_spin_lock_irqsave+0x9a/0xf0
[  124.648702]  ? _raw_read_lock_irqsave+0x60/0x60
[  124.649966]  ? __wake_up_common+0x4b/0x1d0
[  124.651075]  ? preempt_count_sub+0x14/0xc0
[  124.652180]  ? _raw_spin_unlock_irqrestore+0x20/0x40
[  124.653508]  ? __wake_up_common_lock+0xd7/0x130
[  124.654740]  ? __wake_up_common+0x1d0/0x1d0
[  124.655870]  ? mutex_trylock+0x5f/0x90
[  124.656897]  ? ldsem_up_read+0x13/0x40
[  124.657940]  ? tty_write+0x2c7/0x450
[  124.658902]  ? n_tty_open+0xd0/0xd0
[  124.659868]  ? __vfs_write+0xc9/0x3b0
[  124.660876]  ? __anon_vma_prepare+0x159/0x240
[  124.662057]  ? preempt_count_add+0x8d/0xe0
[  124.663170]  ? do_vfs_ioctl+0x134/0x8f0
[  124.664220]  ? fsnotify+0x5ba/0x5e0
[  124.665169]  ? __lru_cache_add+0xc4/0x100
[  124.666255]  ? ioctl_preallocate+0x140/0x140
[  124.667383]  ? __fsnotify_inode_delete+0x20/0x20
[  124.668640]  ? vfs_write+0x120/0x230
[  124.669613]  ? ksys_ioctl+0x70/0x80
[  124.670570]  ? __x64_sys_ioctl+0x3d/0x50
[  124.671634]  ? do_syscall_64+0x73/0x160
[  124.672690]  ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  124.674099] Modules linked in: btrfs xor zstd_compress raid6_pq
[  124.676617] CR2: 0000000000000000
[  124.677523] ---[ end trace c5f555a92fe90edc ]---
[  124.678753] RIP: 0010:          (null)
[  124.679762] Code: Bad RIP value.
[...]
=================

I'm not familiar with the details of the TTY subsystem, so I'm not
sure how this should be fixed.

Re: [BUG] tiocsti() NULL dereference if ld->ops->receive_buf==NULL

[Greg Kroah-Hartman]

On Fri, Jan 18, 2019 at 08:09:07PM +0100, Jann Horn wrote:
> Hi!
> 
> When a line discipline doesn't have a ->receive_buf handler, tiocsti()
> attempts to call a NULL pointer. Both tty_n_tracesink and
> spk_ttyio_ldisc_ops don't have such a handler.
> 
> To reproduce, build a kernel with CONFIG_SPEAKUP=y and
> CONFIG_SPEAKUP_SYNTH_SOFT=y, set speakup.synth=soft in the kernel
> command line, and run the following code as root:

<snip>

Ugh, thanks for finding this.  I'll look at it later this afternoon...

greg k-h

Re: [BUG] tiocsti() NULL dereference if ld->ops->receive_buf==NULL

[Greg Kroah-Hartman]

On Sat, Jan 19, 2019 at 10:11:08AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Jan 18, 2019 at 08:09:07PM +0100, Jann Horn wrote:
> > Hi!
> > 
> > When a line discipline doesn't have a ->receive_buf handler, tiocsti()
> > attempts to call a NULL pointer. Both tty_n_tracesink and
> > spk_ttyio_ldisc_ops don't have such a handler.
> > 
> > To reproduce, build a kernel with CONFIG_SPEAKUP=y and
> > CONFIG_SPEAKUP_SYNTH_SOFT=y, set speakup.synth=soft in the kernel
> > command line, and run the following code as root:
> 
> <snip>
> 
> Ugh, thanks for finding this.  I'll look at it later this afternoon...

It looks to be a simple change.  We can't really "fail" this ioctl if
there's nothing wrong with the structure of the call, so we can just
quietly "eat" the character, given that the line discipline doesn't care
about it.

So, any objections to the patch below?

thanks,

greg k-h

-----------------

Subject: [PATCH] tty: Handle problem if line discipline does not have receive_buf

Some tty line disciplines do not have a receive buf callback, so
properly check for that before calling it.  If they do not have this
callback, just eat the character quietly, as we can't fail this call.

Reported-by: Jann Horn <jannh@google.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/tty_io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 23c6fd238422..21ffcce16927 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2189,7 +2189,8 @@ static int tiocsti(struct tty_struct *tty, char __user *p)
 	ld = tty_ldisc_ref_wait(tty);
 	if (!ld)
 		return -EIO;
-	ld->ops->receive_buf(tty, &ch, &mbz, 1);
+	if (ld->ops->receive_buf)
+		ld->ops->receive_buf(tty, &ch, &mbz, 1);
 	tty_ldisc_deref(ld);
 	return 0;
 }
-- 
2.20.1

Re: [BUG] tiocsti() NULL dereference if ld->ops->receive_buf==NULL

[Jann Horn]

On Sun, Jan 20, 2019 at 10:52 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Jan 19, 2019 at 10:11:08AM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Jan 18, 2019 at 08:09:07PM +0100, Jann Horn wrote:
> > > Hi!
> > >
> > > When a line discipline doesn't have a ->receive_buf handler, tiocsti()
> > > attempts to call a NULL pointer. Both tty_n_tracesink and
> > > spk_ttyio_ldisc_ops don't have such a handler.
> > >
> > > To reproduce, build a kernel with CONFIG_SPEAKUP=y and
> > > CONFIG_SPEAKUP_SYNTH_SOFT=y, set speakup.synth=soft in the kernel
> > > command line, and run the following code as root:
> >
> > <snip>
> >
> > Ugh, thanks for finding this.  I'll look at it later this afternoon...
>
> It looks to be a simple change.  We can't really "fail" this ioctl if
> there's nothing wrong with the structure of the call, so we can just
> quietly "eat" the character, given that the line discipline doesn't care
> about it.
>
> So, any objections to the patch below?

No objection from me.

(spk_ttyio_ldisc_ops has a receive_buf2 handler, but I don't know
whether that should be invoked here or not.)

> -----------------
>
> Subject: [PATCH] tty: Handle problem if line discipline does not have receive_buf
>
> Some tty line disciplines do not have a receive buf callback, so
> properly check for that before calling it.  If they do not have this
> callback, just eat the character quietly, as we can't fail this call.
>
> Reported-by: Jann Horn <jannh@google.com>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/tty/tty_io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 23c6fd238422..21ffcce16927 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2189,7 +2189,8 @@ static int tiocsti(struct tty_struct *tty, char __user *p)
>         ld = tty_ldisc_ref_wait(tty);
>         if (!ld)
>                 return -EIO;
> -       ld->ops->receive_buf(tty, &ch, &mbz, 1);
> +       if (ld->ops->receive_buf)
> +               ld->ops->receive_buf(tty, &ch, &mbz, 1);
>         tty_ldisc_deref(ld);
>         return 0;
>  }
> --
> 2.20.1
>

Re: [BUG] tiocsti() NULL dereference if ld->ops->receive_buf==NULL

[Greg Kroah-Hartman]

On Mon, Jan 21, 2019 at 04:38:33PM +0100, Jann Horn wrote:
> On Sun, Jan 20, 2019 at 10:52 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sat, Jan 19, 2019 at 10:11:08AM +0100, Greg Kroah-Hartman wrote:
> > > On Fri, Jan 18, 2019 at 08:09:07PM +0100, Jann Horn wrote:
> > > > Hi!
> > > >
> > > > When a line discipline doesn't have a ->receive_buf handler, tiocsti()
> > > > attempts to call a NULL pointer. Both tty_n_tracesink and
> > > > spk_ttyio_ldisc_ops don't have such a handler.
> > > >
> > > > To reproduce, build a kernel with CONFIG_SPEAKUP=y and
> > > > CONFIG_SPEAKUP_SYNTH_SOFT=y, set speakup.synth=soft in the kernel
> > > > command line, and run the following code as root:
> > >
> > > <snip>
> > >
> > > Ugh, thanks for finding this.  I'll look at it later this afternoon...
> >
> > It looks to be a simple change.  We can't really "fail" this ioctl if
> > there's nothing wrong with the structure of the call, so we can just
> > quietly "eat" the character, given that the line discipline doesn't care
> > about it.
> >
> > So, any objections to the patch below?
> 
> No objection from me.
> 
> (spk_ttyio_ldisc_ops has a receive_buf2 handler, but I don't know
> whether that should be invoked here or not.)

No, receive_buf2 can fail, or do a short "receive", which receive_buf()
can't do, and tiocsti can't fail (it's only used to fake input data).

Yeah, the tty layer is strange :(

thanks,

greg k-h