Messages in this thread Patch in this message | | | Date | Tue, 6 Jun 2017 00:05:30 +0100 | From | Al Viro <> | 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; } /*
| |