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 for Android: free password hash cracker in your pocket

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

Date:   Sat, 7 Jul 2018 10:22:52 +0200
From:   Jann Horn <jannh@...gle.com>
To:     samuel.thibault@...-lyon.org, w.d.hubbs@...il.com,
        chris@...-brannons.com, kirk@...sers.ca,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        speakup@...ux-speakup.org, devel@...verdev.osuosl.org
Subject: Re: [PATCH] staging: speakup: fix wraparound in uaccess length check

On Sat, Jul 7, 2018 at 10:13 AM Samuel Thibault
<samuel.thibault@...-lyon.org> wrote:
>
> Jann Horn, le sam. 07 juil. 2018 03:53:44 +0200, a ecrit:
> > @@ -257,6 +257,8 @@ static ssize_t softsynthx_read(struct file *fp, char __user *buf, size_t count,
> >                               0x80 | (ch & 0x3f)
> >                       };
> >
> > +                     if (chars_sent + 2 > count)
> > +                             break;
> >                       if (copy_to_user(cp, s, sizeof(s)))
> >                               return -EFAULT;
>
> Err, but then we have lost 'ch' that was consumed by the
> synth_buffer_getc() call, so the fix seems wrong to me.

Oh. Whoops.

So that means I'd need to first synth_buffer_peek(), then
synth_buffer_get() afterwards (and discard the result that time)? But
there are also no locks held at the moment the code is in there, which
could cause that approach to lead to inconsistent results... do you
want me to resend with synth_buffer_peek() and an additional mutex
that is held throughout softsynthx_read()? Or should I rewrite the
patch to be simple and just bail out on `count < 3`?