Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180707015344.146672-1-jannh@google.com> <20180707081347.czde44j6rjepjpkf@var.youpi.perso.aquilenet.fr>
In-Reply-To: <20180707081347.czde44j6rjepjpkf@var.youpi.perso.aquilenet.fr>
From:   Jann Horn <jannh@google.com>
Date:   Sat, 7 Jul 2018 10:22:52 +0200
Message-ID: <CAG48ez3pPcrA8B9bLwuht1Az8EXbDZa1fGmx73esvZBpmwifpg@mail.gmail.com>
Subject: Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
To:     samuel.thibault@ens-lyon.org, w.d.hubbs@gmail.com,
        chris@the-brannons.com, kirk@reisers.ca,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        kernel list <linux-kernel@vger.kernel.org>,
        speakup@linux-speakup.org, devel@driverdev.osuosl.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Jul 7, 2018 at 10:13 AM Samuel Thibault
<samuel.thibault@ens-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`?