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: [email protected]
Signed-off-by: Jann Horn <[email protected]>
---
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
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: [email protected]
> Signed-off-by: Jann Horn <[email protected]>
> ---
>
> Reproducer (kernel overflows userspace stack, resulting in segfault):
Nice find, thanks for the patch!
greg k-h
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 <[email protected]>
Samuel
On Sat, Jul 7, 2018 at 10:13 AM Samuel Thibault
<[email protected]> 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,
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: [email protected]
Signed-off-by: Samuel Thibault <[email protected]>
--- 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';
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
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: [email protected]
> Signed-off-by: Samuel Thibault <[email protected]>
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
On Sat, Jul 7, 2018 at 1:29 AM Samuel Thibault
<[email protected]> 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: [email protected]
> Signed-off-by: Samuel Thibault <[email protected]>
>
> --- 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';
On Sat, Jul 7, 2018 at 7:03 AM Greg Kroah-Hartman
<[email protected]> 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: [email protected]
> > Signed-off-by: Samuel Thibault <[email protected]>
>
> 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?
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
> <[email protected]> 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: [email protected]
> > > Signed-off-by: Samuel Thibault <[email protected]>
> >
> > 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
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
> <[email protected]> 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