| 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
| ||
|
Message-ID: <20170605230530.GU6365@ZenIV.linux.org.uk>
Date: Tue, 6 Jun 2017 00:05:30 +0100
From: Al Viro <viro@...IV.linux.org.uk>
To: Alan Cox <gnomes@...rguk.ukuu.org.uk>
Cc: Adam Borowski <kilobyte@...band.pl>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jslaby@...e.com>, linux-kernel@...r.kernel.org
Subject: 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;
}
/*
Powered by blists - more mailing lists