linux-kernel - Re: [PATCH] staging: speakup: fix wraparound in uaccess length check

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez0ZLzh1L8HieUJ+UmbQewRMc_0hwJG7-MB=WR1h=VagZA@mail.gmail.com>
Date:   Thu, 12 Jul 2018 16:12:39 -0700
From:   Jann Horn <jannh@...gle.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        speakup@...ux-speakup.org,
        Samuel Thibault <samuel.thibault@...-lyon.org>,
        William Hubbs <w.d.hubbs@...il.com>,
        Christopher Brannon <chris@...-brannons.com>, kirk@...sers.ca,
        kernel list <linux-kernel@...r.kernel.org>,
        devel@...verdev.osuosl.org
Subject: Re: [PATCH] staging: speakup: fix wraparound in uaccess length check

On Thu, Jul 12, 2018 at 3:47 PM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Fri, Jul 13, 2018 at 12:29:36AM +0200, Jann Horn wrote:
> > From: Samuel Thibault <samuel.thibault@...-lyon.org>
> >
> > From: Samuel Thibault <samuel.thibault@...-lyon.org>
> >
> > If softsynthx_read() is called with `count < 3`, `count - 3` wraps, causing
> > the loop to copy as much data as available to the provided buffer. If
> > softsynthx_read() is invoked through sys_splice(), this causes an
> > unbounded kernel write; but even when userspace just reads from it
> > normally, a small size could cause userspace crashes.
>
> Or you could try this (completely untested, though):

I think this has the same problem as my original buggy patch: At the
point where you notice that you'd overflow the buffer, you've already
consumed a character from the synth buffer. You'd have to put it back,
and since the spinlock protecting it has been dropped, that's a bit
weird.

Also, I'm not sure whether Greg prefers fixes for stable kernels that
don't also contain cleanup?

> diff --git a/drivers/staging/speakup/speakup_soft.c b/drivers/staging/speakup/speakup_soft.c
> index a61bc41b82d7..198936ed0b54 100644
> --- a/drivers/staging/speakup/speakup_soft.c
> +++ b/drivers/staging/speakup/speakup_soft.c
[...]
> @@ -224,11 +221,14 @@ static ssize_t softsynthx_read(struct file *fp, char __user *buf, size_t count,
>         }
>         finish_wait(&speakup_event, &wait);
>
> -       cp = buf;
>         init = get_initstring();
>
>         /* Keep 3 bytes available for a 16bit UTF-8-encoded character */
> -       while (chars_sent <= count - 3) {
> +       while (iov_iter_count(to)) {
> +               u_char s[3];
> +               int l, n;
> +               u16 ch;
> +
>                 if (speakup_info.flushing) {
>                         speakup_info.flushing = 0;
>                         ch = '\x18';
> @@ -244,60 +244,47 @@ static ssize_t softsynthx_read(struct file *fp, char __user *buf, size_t count,
>                 spin_unlock_irqrestore(&speakup_info.spinlock, flags);
>
>                 if ((!unicode && ch < 0x100) || (unicode && ch < 0x80)) {
> -                       u_char c = ch;
> -
> -                       if (copy_to_user(cp, &c, 1))
> -                               return -EFAULT;
> -
> -                       chars_sent++;
> -                       cp++;
> +                       s[0] = ch;
> +                       l = 1;
>                 } else if (unicode && ch < 0x800) {
> -                       u_char s[2] = {
> -                               0xc0 | (ch >> 6),
> -                               0x80 | (ch & 0x3f)
> -                       };
> -
> -                       if (copy_to_user(cp, s, sizeof(s)))
> -                               return -EFAULT;
> -
> -                       chars_sent += sizeof(s);
> -                       cp += sizeof(s);
> +                       s[0] = 0xc0 | (ch >> 6);
> +                       s[1] = 0x80 | (ch & 0x3f);
> +                       l = 2;
>                 } else if (unicode) {
> -                       u_char s[3] = {
> -                               0xe0 | (ch >> 12),
> -                               0x80 | ((ch >> 6) & 0x3f),
> -                               0x80 | (ch & 0x3f)
> -                       };
> -
> -                       if (copy_to_user(cp, s, sizeof(s)))
> -                               return -EFAULT;
> -
> -                       chars_sent += sizeof(s);
> -                       cp += sizeof(s);
> +                       s[0] = 0xe0 | (ch >> 12);
> +                       s[1] = 0x80 | ((ch >> 6) & 0x3f);
> +                       s[2] = 0x80 | (ch & 0x3f);
> +                       l = 3;
>                 }
> -
> +               n = copy_to_iter(s, l, to);
> +               if (n != l) {
> +                       iov_iter_revert(to, n);
> +                       spin_lock_irqsave(&speakup_info.spinlock, flags);
> +                       break;
> +               }