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]
Message-ID: <20180712224753.GE30522@ZenIV.linux.org.uk>
Date:   Thu, 12 Jul 2018 23:47:53 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Jann Horn <jannh@...gle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        speakup@...ux-speakup.org,
        Samuel Thibault <samuel.thibault@...-lyon.org>,
        William Hubbs <w.d.hubbs@...il.com>,
        Chris Brannon <chris@...-brannons.com>,
        Kirk Reiser <kirk@...sers.ca>, linux-kernel@...r.kernel.org,
        devel@...verdev.osuosl.org
Subject: 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 <samuel.thibault@...-lyon.org>
> 
> From: Samuel Thibault <samuel.thibault@...-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,