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

[Al Viro]

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, >unicode);
> > -   __put_user(plist->fontpos, >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;
 }
 
 /*

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

[Al Viro]

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, >unicode);
> > -   __put_user(plist->fontpos, >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;
 }
 
 /*

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

[Alan Cox]

> @@ -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, >unicode);
> - __put_user(plist->fontpos, >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

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

[Alan Cox]

> @@ -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, >unicode);
> - __put_user(plist->fontpos, >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

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

[Adam Borowski]

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 
---
 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, >unicode);
-   __put_user(plist->fontpos, >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

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

[Adam Borowski]

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 
---
 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, >unicode);
-   __put_user(plist->fontpos, >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