Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751348AbdFEXFk (ORCPT ); Mon, 5 Jun 2017 19:05:40 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:40436 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751274AbdFEXFj (ORCPT ); Mon, 5 Jun 2017 19:05:39 -0400 Date: Tue, 6 Jun 2017 00:05:30 +0100 From: Al Viro To: Alan Cox Cc: Adam Borowski , Greg Kroah-Hartman , Jiri Slaby , linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/5] vt: use copy_to_user instead of __put_user in GIO_UNIMAP ioctl Message-ID: <20170605230530.GU6365@ZenIV.linux.org.uk> References: <20170603073255.zgpfmdp2pqusz4qw@angband.pl> <20170603073509.4718-1-kilobyte@angband.pl> <20170603073509.4718-3-kilobyte@angband.pl> <20170605173416.73da4975@lxorguk.ukuu.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170605173416.73da4975@lxorguk.ukuu.org.uk> User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3378 Lines: 128 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; } /*