Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
On Thu, Jul 12, 2018 at 04:12:39PM -0700, Jann Horn wrote: > On Thu, Jul 12, 2018 at 3:47 PM Al Viro wrote: > > > > On Fri, Jul 13, 2018 at 12:29:36AM +0200, Jann Horn wrote: > > > From: Samuel Thibault > > > > > > From: Samuel Thibault > > > > > > 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? For staging code, I really don't care, as long as it's fixing an issue :) thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
On Fri, Jul 13, 2018 at 12:29:36AM +0200, Jann Horn wrote:
> From: Samuel Thibault
>
> From: Samuel Thibault
>
> 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):
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
@@ -192,13 +192,10 @@ static int softsynth_close(struct inode *inode, struct
file *fp)
return 0;
}
-static ssize_t softsynthx_read(struct file *fp, char __user *buf, size_t count,
- loff_t *pos, int unicode)
+static ssize_t softsynthx_read_iter(struct kiocb *iocb, struct iov_iter *to,
int unicode)
{
int chars_sent = 0;
- char __user *cp;
char *init;
- u16 ch;
int empty;
unsigned long flags;
DEFINE_WAIT(wait);
@@ -211,7 +208,7 @@ static ssize_t softsynthx_read(struct file *fp, char __user
*buf, size_t count,
if (!synth_buffer_empty() || speakup_info.flushing)
break;
spin_unlock_irqrestore(&speakup_info.spinlock, flags);
- if (fp->f_flags & O_NONBLOCK) {
+ if (iocb->ki_flags & IOCB_NOWAIT) {
finish_wait(&speakup_event, &wait);
return -EAGAIN;
}
@@ -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;
+ }
+ chars_sent += l;
spin_lock_irqsave(&speakup_info.spinlock, flags);
}
- *pos += chars_sent;
+ iocb->ki_pos += chars_sent;
empty = synth_buffer_empty();
spin_unlock_irqrestore(&speakup_info.spinlock, flags);
if (empty) {
speakup_start_ttys();
- *pos = 0;
+ iocb->ki_pos = 0;
}
return chars_sent;
}
-static ssize_t softsynth_read(struct file *fp, char __user *buf, size_t count,
+static ssize_t softsynth_read_iter(struct kiocb *iocb, struct iov_iter *to)
loff_t *pos)
{
- return softsynthx_read(fp, buf, count
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
Hello, Jann Horn, le mar. 10 juil. 2018 13:34:33 -0700, a ecrit: > On Sat, Jul 7, 2018 at 1:29 AM Samuel Thibault > wrote: > > Could you review, test, and resubmit the patch below instead? > > Er... you mean, you want me to take your patch, add my Signed-off-by > below yours, and then send that? Yes, please. > Some random thing I noticed, but I don't think it has anything to do > with this issue: In some runs, when the console is repeatedly printing > "Debian GNU/Linux 9 debian tty1\n\ndebian login: " in response to me > pressing enter repeatedly, /dev/softsynthu (read in 1-byte steps) > seems to return things like "Debian GNU slash Linux 9 debian tty1 \n > debi login: ". I don't understand why it sometimes says "debi login" > instead of "debian login". It's odd indeed, but I agree it's unrelated. Samuel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
On Tue, Jul 10, 2018 at 01:34:59PM -0700, Jann Horn wrote:
> On Sat, Jul 7, 2018 at 7:03 AM Greg Kroah-Hartman
> wrote:
> >
> > On Sat, Jul 07, 2018 at 10:29:26AM +0200, Samuel Thibault wrote:
> > > Re,
> > >
> > > Could you review, test, and resubmit the patch below instead?
> > >
> > > Samuel
> > >
> > >
> > > 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.
> > >
> > > Fixes: 425e586cf95b ("speakup: add unicode variant of /dev/softsynth")
> > > Cc: sta...@vger.kernel.org
> > > Signed-off-by: Samuel Thibault
> >
> > You forgot a "reported-by:" line :(
> >
> > also, I already applied Jann's patch, so could you either just send the
> > fixup, or a revert/add of this patch once you all agree on the proper
> > solution here?
>
> I think my patch was garbage (as both Samuel and Dan Carpenter's
> smatch warning pointed out) and should be reverted. Should I be
> sending the revert?
I'll just go drop it, thanks for letting me know.
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
On Sat, Jul 07, 2018 at 10:29:26AM +0200, Samuel Thibault wrote:
> Re,
>
> Could you review, test, and resubmit the patch below instead?
>
> Samuel
>
>
> 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.
>
> Fixes: 425e586cf95b ("speakup: add unicode variant of /dev/softsynth")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Samuel Thibault
You forgot a "reported-by:" line :(
also, I already applied Jann's patch, so could you either just send the
fixup, or a revert/add of this patch once you all agree on the proper
solution here?
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
Jann Horn, le sam. 07 juil. 2018 10:22:52 +0200, a ecrit: > Or should I rewrite the > patch to be simple and just bail out on `count < 3`? Our mails have crossed :) I believe what I sent is correct: for softsynth it does not make sense to have room for less than 1 (non-unicode) or 3 (unicode) bytes. Samuel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
Re,
Could you review, test, and resubmit the patch below instead?
Samuel
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.
Fixes: 425e586cf95b ("speakup: add unicode variant of /dev/softsynth")
Cc: sta...@vger.kernel.org
Signed-off-by: Samuel Thibault
--- a/drivers/staging/speakup/speakup_soft.c
+++ b/drivers/staging/speakup/speakup_soft.c
@@ -198,11 +198,15 @@ static ssize_t softsynthx_read(struct fi
int chars_sent = 0;
char __user *cp;
char *init;
+ size_t bytes_per_ch = unicode ? 3 : 1;
u16 ch;
int empty;
unsigned long flags;
DEFINE_WAIT(wait);
+ if (count < bytes_per_ch)
+ return -EINVAL;
+
spin_lock_irqsave(&speakup_info.spinlock, flags);
while (1) {
prepare_to_wait(&speakup_event, &wait, TASK_INTERRUPTIBLE);
@@ -228,7 +232,7 @@ static ssize_t softsynthx_read(struct fi
init = get_initstring();
/* Keep 3 bytes available for a 16bit UTF-8-encoded character */
- while (chars_sent <= count - 3) {
+ while (chars_sent <= count - bytes_per_ch) {
if (speakup_info.flushing) {
speakup_info.flushing = 0;
ch = '\x18';
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
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. Nacked-by: Samuel Thibault Samuel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
On Sat, Jul 07, 2018 at 03:53:44AM +0200, Jann Horn wrote:
> 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.
>
> Fixes: 425e586cf95b ("speakup: add unicode variant of /dev/softsynth")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Jann Horn
> ---
>
> Reproducer (kernel overflows userspace stack, resulting in segfault):
Nice find, thanks for the patch!
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
