linux-kernel - Re: [PATCH] tty: vt: remove multi-fetch, derive font.height from font.data

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 4 Oct 2017 10:22:31 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Meng Xu <mengxu.gatech@...il.com>
Cc:     jslaby@...e.com, kilobyte@...band.pl, linux-kernel@...r.kernel.org,
        meng.xu@...ech.edu, sanidhya@...ech.edu, taesoo@...ech.edu
Subject: Re: [PATCH] tty: vt: remove multi-fetch, derive font.height from
 font.data

On Sun, Sep 24, 2017 at 12:59:42PM -0400, Meng Xu wrote:
> In con_font_set(), when we need to guess font height (for
> compat reasons?), the current approach uses multiple userspace
> fetches, i.e., get_user(tmp, &charmap[32*i+h-1]), to derive
> the height. This has two drawbacks:
> 
> 1. performance: accessing userspace memory is less efficient than
> directly de-reference the byte
> 
> 2. security: a more critical problem is that the height derived
> might not match with the actual font.data. This is because a user
> thread might race condition to change the memory of op->data after
> the op->height guessing but before the second fetch: font.data =
> memdup_user(op->data, size). Leaving font.height = 32 while the
> actual height is 1 or vice-versa.
> 
> This patch tries to resolve both issues by re-locating the height
> guessing part after the font.data is fetched in. In this way, the
> userspace data is fetched in one shot and we directly dereference
> the font.data in kernel space to probe for the height.
> 
> Signed-off-by: Meng Xu <mengxu.gatech@...il.com>
> ---
>  drivers/tty/vt/vt.c | 48 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 20 deletions(-)

Please always run your patches through checkpatch.pl so that you don't
get a grumpy maintainer telling you to use checkpatch.pl :(

Can you fix the obvious issues up and resend?

thanks,

greg k-h