Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1656738 > unrolled thread

[PATCH 1/5] vt: use copy_from/to_user instead of __get/put_user for scrnmap ioctls

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/5] vt: use copy_from/to_user instead of __get/put_user for scrnmap ioctls Adam Borowski <kilobyte@angband.pl> - 2017-06-03 09:40 +0200
    [PATCH 5/5] vt: drop access_ok() calls in unimap ioctls Adam Borowski <kilobyte@angband.pl> - 2017-06-03 09:40 +0200
    [PATCH 2/5] vt: fix unchecked __put_user() in tioclinux ioctls Adam Borowski <kilobyte@angband.pl> - 2017-06-03 09:40 +0200
    [PATCH 3/5] vt: use copy_to_user instead of __put_user in GIO_UNIMAP ioctl Adam Borowski <kilobyte@angband.pl> - 2017-06-03 09:40 +0200
      Re: [PATCH 3/5] vt: use copy_to_user instead of __put_user in  GIO_UNIMAP ioctl Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-06-05 18:40 +0200
        Re: [PATCH 3/5] vt: use copy_to_user instead of __put_user in  GIO_UNIMAP ioctl Al Viro <viro@ZenIV.linux.org.uk> - 2017-06-06 01:10 +0200
    [PATCH 4/5] vt: use memdup_user in PIO_UNIMAP ioctl Adam Borowski <kilobyte@angband.pl> - 2017-06-03 09:40 +0200

#1656738 — [PATCH 1/5] vt: use copy_from/to_user instead of __get/put_user for scrnmap ioctls

Linus wants to get rid of these functions, and these uses are especially
egregious: they copy a big linear array element by element.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 drivers/tty/vt/consolemap.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 1f6e17fc3fb0..1361f2a8b832 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -322,15 +322,13 @@ int con_set_trans_old(unsigned char __user * arg)
 {
 	int i;
 	unsigned short inbuf[E_TABSZ];
+	unsigned char ubuf[E_TABSZ];
 
-	if (!access_ok(VERIFY_READ, arg, E_TABSZ))
+	if (copy_from_user(ubuf, arg, E_TABSZ))
 		return -EFAULT;
 
-	for (i = 0; i < E_TABSZ ; i++) {
-		unsigned char uc;
-		__get_user(uc, arg+i);
-		inbuf[i] = UNI_DIRECT_BASE | uc;
-	}
+	for (i = 0; i < E_TABSZ ; i++)
+		inbuf[i] = UNI_DIRECT_BASE | ubuf[i];
 
 	console_lock();
 	memcpy(translations[USER_MAP], inbuf, sizeof(inbuf));
@@ -345,9 +343,6 @@ int con_get_trans_old(unsigned char __user * arg)
 	unsigned short *p = translations[USER_MAP];
 	unsigned char outbuf[E_TABSZ];
 
-	if (!access_ok(VERIFY_WRITE, arg, E_TABSZ))
-		return -EFAULT;
-
 	console_lock();
 	for (i = 0; i < E_TABSZ ; i++)
 	{
@@ -356,22 +351,16 @@ int con_get_trans_old(unsigned char __user * arg)
 	}
 	console_unlock();
 
-	for (i = 0; i < E_TABSZ ; i++)
-		__put_user(outbuf[i], arg+i);
-	return 0;
+	return copy_to_user(arg, outbuf, sizeof(outbuf)) ? -EFAULT : 0;
 }
 
 int con_set_trans_new(ushort __user * arg)
 {
-	int i;
 	unsigned short inbuf[E_TABSZ];
 
-	if (!access_ok(VERIFY_READ, arg, E_TABSZ*sizeof(unsigned short)))
+	if (copy_from_user(inbuf, arg, sizeof(inbuf)))
 		return -EFAULT;
 
-	for (i = 0; i < E_TABSZ ; i++)
-		__get_user(inbuf[i], arg+i);
-
 	console_lock();
 	memcpy(translations[USER_MAP], inbuf, sizeof(inbuf));
 	update_user_maps();
@@ -381,19 +370,13 @@ int con_set_trans_new(ushort __user * arg)
 
 int con_get_trans_new(ushort __user * arg)
 {
-	int i;
 	unsigned short outbuf[E_TABSZ];
 
-	if (!access_ok(VERIFY_WRITE, arg, E_TABSZ*sizeof(unsigned short)))
-		return -EFAULT;
-
 	console_lock();
 	memcpy(outbuf, translations[USER_MAP], sizeof(outbuf));
 	console_unlock();
 
-	for (i = 0; i < E_TABSZ ; i++)
-		__put_user(outbuf[i], arg+i);
-	return 0;
+	return copy_to_user(arg, outbuf, sizeof(outbuf)) ? -EFAULT : 0;
 }
 
 /*
-- 
2.11.0

[toc] | [next] | [standalone]

#1656740 — [PATCH 5/5] vt: drop access_ok() calls in unimap ioctls

Done by copy_{from,to}_user().

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 drivers/tty/vt/vt_ioctl.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 0cbfe1ff6f6c..96d389cb506c 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -266,10 +266,6 @@ do_unimap_ioctl(int cmd, struct unimapdesc __user *user_ud, int perm, struct vc_
 
 	if (copy_from_user(&tmp, user_ud, sizeof tmp))
 		return -EFAULT;
-	if (tmp.entries)
-		if (!access_ok(VERIFY_WRITE, tmp.entries,
-				tmp.entry_ct*sizeof(struct unipair)))
-			return -EFAULT;
 	switch (cmd) {
 	case PIO_UNIMAP:
 		if (!perm)
@@ -1170,10 +1166,6 @@ compat_unimap_ioctl(unsigned int cmd, struct compat_unimapdesc __user *user_ud,
 	if (copy_from_user(&tmp, user_ud, sizeof tmp))
 		return -EFAULT;
 	tmp_entries = compat_ptr(tmp.entries);
-	if (tmp_entries)
-		if (!access_ok(VERIFY_WRITE, tmp_entries,
-				tmp.entry_ct*sizeof(struct unipair)))
-			return -EFAULT;
 	switch (cmd) {
 	case PIO_UNIMAP:
 		if (!perm)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1656741 — [PATCH 2/5] vt: fix unchecked __put_user() in tioclinux ioctls

Only read access is checked before this call.

Actually, at the moment this is not an issue, as every in-tree arch does
the same manual checks for VERIFY_READ vs VERIFY_WRITE, relying on the MMU
to tell them apart, but this wasn't the case in the past and may happen
again on some odd arch in the future.

If anyone cares about 3.7 and earlier, this is a security hole (untested)
on real 80386 CPUs.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
CC: stable@vger.kernel.org # v3.7-
---
 drivers/tty/vt/vt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9309c7da660a..2ebaba16f785 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2709,13 +2709,13 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
 	 * related to the kernel should not use this.
 	 */
 			data = vt_get_shift_state();
-			ret = __put_user(data, p);
+			ret = put_user(data, p);
 			break;
 		case TIOCL_GETMOUSEREPORTING:
 			console_lock();	/* May be overkill */
 			data = mouse_reporting();
 			console_unlock();
-			ret = __put_user(data, p);
+			ret = put_user(data, p);
 			break;
 		case TIOCL_SETVESABLANK:
 			console_lock();
@@ -2724,7 +2724,7 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
 			break;
 		case TIOCL_GETKMSGREDIRECT:
 			data = vt_get_kmsg_redirect();
-			ret = __put_user(data, p);
+			ret = put_user(data, p);
 			break;
 		case TIOCL_SETKMSGREDIRECT:
 			if (!capable(CAP_SYS_ADMIN)) {
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1656742 — [PATCH 3/5] vt: use copy_to_user instead of __put_user in GIO_UNIMAP ioctl

A nice big linear transfer, no need to flip stac/PAN/etc every half-entry.
Also, yay __put_user() after checking only read.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 drivers/tty/vt/consolemap.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 1361f2a8b832..c6a692f63a9b 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -740,11 +740,11 @@ EXPORT_SYMBOL(con_copy_unimap);
  */
 int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct unipair __user *list)
 {
-	int i, j, k;
+	int i, j, k, ret = 0;
 	ushort ect;
 	u16 **p1, *p2;
 	struct uni_pagedir *p;
-	struct unipair *unilist, *plist;
+	struct unipair *unilist;
 
 	unilist = kmalloc_array(ct, sizeof(struct unipair), GFP_KERNEL);
 	if (!unilist)
@@ -775,13 +775,11 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 		}
 	}
 	console_unlock();
-	for (i = min(ect, ct), plist = unilist; i; i--, list++, plist++) {
-		__put_user(plist->unicode, &list->unicode);
-		__put_user(plist->fontpos, &list->fontpos);
-	}
-	__put_user(ect, uct);
+	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(struct unipair)))
+		ret = -EFAULT;
+	put_user(ect, uct);
 	kfree(unilist);
-	return ((ect <= ct) ? 0 : -ENOMEM);
+	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
 }
 
 /*
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1657816 — Re: [PATCH 3/5] vt: use copy_to_user instead of __put_user in GIO_UNIMAP ioctl

> @@ -775,13 +775,11 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
>  		}
>  	}
>  	console_unlock();
> -	for (i = min(ect, ct), plist = unilist; i; i--, list++, plist++) {
> -		__put_user(plist->unicode, &list->unicode);
> -		__put_user(plist->fontpos, &list->fontpos);
> -	}
> -	__put_user(ect, uct);
> +	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(struct unipair)))
> +		ret = -EFAULT;
> +	put_user(ect, uct);
>  	kfree(unilist);
> -	return ((ect <= ct) ? 0 : -ENOMEM);
> +	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
>  }


The rest looks good but that line needs taking out and shooting.

Alan

[toc] | [prev] | [next] | [standalone]

#1658212 — Re: [PATCH 3/5] vt: use copy_to_user instead of __put_user in GIO_UNIMAP ioctl

On Mon, Jun 05, 2017 at 05:34:16PM +0100, Alan Cox wrote:
> > @@ -775,13 +775,11 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
> >  		}
> >  	}
> >  	console_unlock();
> > -	for (i = min(ect, ct), plist = unilist; i; i--, list++, plist++) {
> > -		__put_user(plist->unicode, &list->unicode);
> > -		__put_user(plist->fontpos, &list->fontpos);
> > -	}
> > -	__put_user(ect, uct);
> > +	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(struct unipair)))
> > +		ret = -EFAULT;
> > +	put_user(ect, uct);
> >  	kfree(unilist);
> > -	return ((ect <= ct) ? 0 : -ENOMEM);
> > +	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
> >  }
> 
> 
> The rest looks good but that line needs taking out and shooting.

The rest of that function is also Not Nice(tm).  Creative indentation, unchecked
put_user() result...  How about this on top of Adam's commit?  One thing I'm
not sure about is this -ENOMEM return on overflow; there's no way for caller
to tell it from allocation failure, so what's the point copying the list out
in that case?  I've kept the behaviour as-is, but...

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index c6a692f63a9b..73ef478c7e68 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -731,6 +731,36 @@ int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc)
 }
 EXPORT_SYMBOL(con_copy_unimap);
 
+static int fill_unilist(struct unipair *list, struct uni_pagedir *dir, u16 ct)
+{
+	int i, j, k, n;
+
+	if (!dir)
+		return 0;
+
+	for (n = i = 0; i < 32; i++) {
+		u16 **p1 = dir->uni_pgdir[i];
+		if (!p1)
+			continue;
+		for (j = 0; j < 32; j++) {
+			u16 *p2 = p1[j];
+			if (!p2)
+				continue;
+			for (k = 0; k < 64; k++) {
+				u16 pos = p2[k];
+				if (pos >= MAX_GLYPH)
+					continue;
+				if (unlikely(n == ct))
+					return -1;
+				list[n].unicode = (i<<11) + (j<<6) + k;
+				list[n].fontpos = pos;
+				n++;
+			}
+		}
+	}
+	return n;
+}
+
 /**
  *	con_get_unimap		-	get the unicode map
  *	@vc: the console to read from
@@ -740,46 +770,29 @@ EXPORT_SYMBOL(con_copy_unimap);
  */
 int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct unipair __user *list)
 {
-	int i, j, k, ret = 0;
-	ushort ect;
-	u16 **p1, *p2;
-	struct uni_pagedir *p;
 	struct unipair *unilist;
+	ushort ect;
+	int n, ret = 0;
 
 	unilist = kmalloc_array(ct, sizeof(struct unipair), GFP_KERNEL);
 	if (!unilist)
 		return -ENOMEM;
 
 	console_lock();
-
-	ect = 0;
-	if (*vc->vc_uni_pagedir_loc) {
-		p = *vc->vc_uni_pagedir_loc;
-		for (i = 0; i < 32; i++) {
-		p1 = p->uni_pgdir[i];
-		if (p1)
-			for (j = 0; j < 32; j++) {
-			p2 = *(p1++);
-			if (p2)
-				for (k = 0; k < 64; k++, p2++) {
-					if (*p2 >= MAX_GLYPH)
-						continue;
-					if (ect < ct) {
-						unilist[ect].unicode =
-							(i<<11)+(j<<6)+k;
-						unilist[ect].fontpos = *p2;
-					}
-					ect++;
-				}
-			}
-		}
-	}
+	n = fill_unilist(unilist, *vc->vc_uni_pagedir_loc, ct);
 	console_unlock();
-	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(struct unipair)))
+	if (n < 0) {
+		ect = ct;
+		ret = -ENOMEM;
+	} else {
+		ect = n;
+		ret = 0;
+	}
+	if (copy_to_user(list, unilist, ect * sizeof(struct unipair)) ||
+	    put_user(ect, uct))
 		ret = -EFAULT;
-	put_user(ect, uct);
 	kfree(unilist);
-	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
+	return ret;
 }
 
 /*

[toc] | [prev] | [next] | [standalone]

#1656746 — [PATCH 4/5] vt: use memdup_user in PIO_UNIMAP ioctl

Again, a nice linear transfer that simplifies the code.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 drivers/tty/vt/consolemap.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index c6a692f63a9b..a5f88cf0f61d 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -540,14 +540,9 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 	if (!ct)
 		return 0;
 
-	unilist = kmalloc_array(ct, sizeof(struct unipair), GFP_KERNEL);
-	if (!unilist)
-		return -ENOMEM;
-
-	for (i = ct, plist = unilist; i; i--, plist++, list++) {
-		__get_user(plist->unicode, &list->unicode);
-		__get_user(plist->fontpos, &list->fontpos);
-	}
+	unilist = memdup_user(list, ct * sizeof(struct unipair));
+	if (IS_ERR(unilist))
+		return PTR_ERR(unilist);
 
 	console_lock();
 
-- 
2.11.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web