[PATCH] staging: speakup: fix wraparound in uaccess length check

[PATCH] staging: speakup: fix wraparound in uaccess length check

[Jann Horn]

From: Samuel Thibault <samuel.thibault@ens-lyon.org>

From: Samuel Thibault <samuel.thibault@ens-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.

Fixes: 425e586cf95b ("speakup: add unicode variant of /dev/softsynth")
Cc: stable@vger.kernel.org
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Jann Horn <jannh@google.com>
---
 drivers/staging/speakup/speakup_soft.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/speakup_soft.c b/drivers/staging/speakup/speakup_soft.c
index a61bc41b82d7..947c79532e10 100644
--- a/drivers/staging/speakup/speakup_soft.c
+++ b/drivers/staging/speakup/speakup_soft.c
@@ -198,11 +198,15 @@ static ssize_t softsynthx_read(struct file *fp, char __user *buf, size_t count,
 	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 file *fp, char __user *buf, size_t count,
 	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';
-- 
2.18.0.203.gfac676dfb9-goog

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

[Al Viro]

On Fri, Jul 13, 2018 at 12:29:36AM +0200, Jann Horn wrote:
> From: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> From: Samuel Thibault <samuel.thibault@ens-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):

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, pos, 0);
+	return softsynthx_read_iter(iocb, to, 0);
 }
 
-static ssize_t softsynthu_read(struct file *fp, char __user *buf, size_t count,
+static ssize_t softsynthu_read_iter(struct kiocb *iocb, struct iov_iter *to)
 			       loff_t *pos)
 {
-	return softsynthx_read(fp, buf, count, pos, 1);
+	return softsynthx_read_iter(iocb, to, 1);
 }
 
 static int last_index;
@@ -343,7 +330,7 @@ static unsigned char get_index(struct spk_synth *synth)
 static const struct file_operations softsynth_fops = {
 	.owner = THIS_MODULE,
 	.poll = softsynth_poll,
-	.read = softsynth_read,
+	.read_iter = softsynth_read_iter,
 	.write = softsynth_write,
 	.open = softsynth_open,
 	.release = softsynth_close,
@@ -352,7 +339,7 @@ static const struct file_operations softsynth_fops = {
 static const struct file_operations softsynthu_fops = {
 	.owner = THIS_MODULE,
 	.poll = softsynth_poll,
-	.read = softsynthu_read,
+	.read_iter = softsynthu_read_iter,
 	.write = softsynth_write,
 	.open = softsynth_open,
 	.release = softsynth_close,

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

[Jann Horn]

On Thu, Jul 12, 2018 at 3:47 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Fri, Jul 13, 2018 at 12:29:36AM +0200, Jann Horn wrote:
> > From: Samuel Thibault <samuel.thibault@ens-lyon.org>
> >
> > From: Samuel Thibault <samuel.thibault@ens-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;
> +               }

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

[Greg Kroah-Hartman]

On Thu, Jul 12, 2018 at 04:12:39PM -0700, Jann Horn wrote:
> On Thu, Jul 12, 2018 at 3:47 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Fri, Jul 13, 2018 at 12:29:36AM +0200, Jann Horn wrote:
> > > From: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > >
> > > From: Samuel Thibault <samuel.thibault@ens-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?

For staging code, I really don't care, as long as it's fixing an issue :)

thanks,

greg k-h

[PATCH] staging: speakup: fix wraparound in uaccess length check

[Jann Horn]

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: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---

Reproducer (kernel overflows userspace stack, resulting in segfault):

root@debian:/home/user# cat test.c
#include <unistd.h>
int main(void) {
  char buf[1];
  read(0, buf, 1);
}
root@debian:/home/user# gcc -o test test.c
root@debian:/home/user# ./test < /dev/softsynth
[do some stuff on the console so that it prints text]
Segmentation fault
root@debian:/home/user# strace ./test < /dev/softsynth
execve("./test", ["./test"], [/* 21 vars */]) = 0
brk(NULL)                               = 0x55d5977da000
access("/etc/ld.so.nohwcap", F_OK)      = -1 ENOENT (No such file or directory)
mmap(NULL, 12288, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f2ca2cac000
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=103509, ...}) = 0
mmap(NULL, 103509, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f2ca2c92000
close(3)                                = 0
access("/etc/ld.so.nohwcap", F_OK)      = -1 ENOENT (No such file or directory)
open("/lib/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
read(3, "\177ELF\2\1\1\3\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\320\3\2\0\0\0\0\0"..., 832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=1689360, ...}) = 0
mmap(NULL, 3795360, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7f2ca26ed000
mprotect(0x7f2ca2882000, 2097152, PROT_NONE) = 0
mmap(0x7f2ca2a82000, 24576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x195000) = 0x7f2ca2a82000
mmap(0x7f2ca2a88000, 14752, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f2ca2a88000
close(3)                                = 0
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f2ca2c90000
arch_prctl(ARCH_SET_FS, 0x7f2ca2c90700) = 0
mprotect(0x7f2ca2a82000, 16384, PROT_READ) = 0
mprotect(0x55d596384000, 4096, PROT_READ) = 0
mprotect(0x7f2ca2caf000, 4096, PROT_READ) = 0
munmap(0x7f2ca2c92000, 103509)          = 0
read(0, "\30\0012s\0015p\0015v\0011x\0010b\0010o\0015f\n", 1) = 23
--- SIGSEGV {si_signo=SIGSEGV, si_code=SI_KERNEL, si_addr=NULL} ---
+++ killed by SIGSEGV +++
Segmentation fault


 drivers/staging/speakup/speakup_soft.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/speakup_soft.c b/drivers/staging/speakup/speakup_soft.c
index a61bc41b82d7..f9b405bd052d 100644
--- a/drivers/staging/speakup/speakup_soft.c
+++ b/drivers/staging/speakup/speakup_soft.c
@@ -228,7 +228,7 @@ static ssize_t softsynthx_read(struct file *fp, char __user *buf, size_t count,
 	init = get_initstring();
 
 	/* Keep 3 bytes available for a 16bit UTF-8-encoded character */
-	while (chars_sent <= count - 3) {
+	while (chars_sent < count) {
 		if (speakup_info.flushing) {
 			speakup_info.flushing = 0;
 			ch = '\x18';
@@ -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;
 
@@ -269,6 +271,8 @@ static ssize_t softsynthx_read(struct file *fp, char __user *buf, size_t count,
 				0x80 | (ch & 0x3f)
 			};
 
+			if (chars_sent + 3 > count)
+				break;
 			if (copy_to_user(cp, s, sizeof(s)))
 				return -EFAULT;
 
-- 
2.18.0.203.gfac676dfb9-goog

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

[Greg Kroah-Hartman]

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: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> 
> Reproducer (kernel overflows userspace stack, resulting in segfault):

Nice find, thanks for the patch!

greg k-h

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

[Samuel Thibault]

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.thibault@ens-lyon.org>

Samuel

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

[Jann Horn]

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`?

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

[Samuel Thibault]

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

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

[Samuel Thibault]

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: stable@vger.kernel.org
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

--- 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';

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

[Greg Kroah-Hartman]

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: stable@vger.kernel.org
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

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

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

[Jann Horn]

On Sat, Jul 7, 2018 at 7:03 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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: stable@vger.kernel.org
> > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>
> 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?

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

[Greg Kroah-Hartman]

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
> <gregkh@linuxfoundation.org> 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: stable@vger.kernel.org
> > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> >
> > 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

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

[Jann Horn]

On Sat, Jul 7, 2018 at 1:29 AM Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
>
> Re,
>
> 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?

> 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.

This looks sane to me. I've also tested it, and it seems to work.

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".
> Fixes: 425e586cf95b ("speakup: add unicode variant of /dev/softsynth")
> Cc: stable@vger.kernel.org
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>
> --- 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';

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

[Samuel Thibault]

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
> <samuel.thibault@ens-lyon.org> 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