Message-ID: <CAGXu5jKqV051qcLJDfyi4rfbw1KzMKHU_cMiw7RUd3GCdk3PSg@mail.gmail.com>
Date:	Fri, 15 Apr 2016 11:55:00 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Yinghai Lu <yinghai@...nel.org>, Baoquan He <bhe@...hat.com>,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	Matt Redfearn <matt.redfearn@...tec.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>,
	Vivek Goyal <vgoyal@...hat.com>,
	Andy Lutomirski <luto@...nel.org>, lasse.collin@...aani.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dave Young <dyoung@...hat.com>,
	"kernel-hardening@...ts.openwall.com" 
	<kernel-hardening@...ts.openwall.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v5 01/21] x86, KASLR: Remove unneeded boot_params argument

On Fri, Apr 15, 2016 at 12:29 AM, Ingo Molnar <mingo@...nel.org> wrote:
>
> * Kees Cook <keescook@...omium.org> wrote:
>
>> From: Yinghai Lu <yinghai@...nel.org>
>>
>> Since the boot_params can be found using the real_mode global variable, there is
>> no need to pass around a pointer to it. This slightly simplifies the
>> choose_kernel_location function and its callers.
>
> Yeah, so I really wanted to apply this series because the changelogs are now very
> nice, but got held up by the very first patch again ...

Thanks for reading through everything! I'll be going through it all
and fixing up what you mentioned (along with anything else that jumps
out at me).

> Guys, 'real_mode' totally sucks as a variable name!

In our defense, that variable name predates git history. ;)
Regardless, I'll get it renamed.

> By removing a seemingly unnecessary parameter, you made the code actively worse to
> read...
>
> So please make a second patch on top of this one that renames 'real_mode' to
> something that both displays what it is about (it sure isn't mainly about real
> mode only!!), and also expresses that it's a global variable, not some local
> function parameter.
>
> In arch/x86/ we usually achieve that via prefixing it with x86_ or so. So
> something like 'x86_boot_params' would work for me. I realize that this is
> somewhat special, because this is pre-relocation code so we really have to be
> careful about pointers - so maybe name it x86_boot_params_rm or so.
>
> Also, I had a look at the whole file you are patching,
> arch/x86/boot/compressed/misc.c, and for heaven's sake, please first improve the
> comments on top of that file before further complicating that code with KASLR
> details! Right now it consists of various low level ramblings choke full of typos.
> No mention of where KASLR fits into the picture at all!
>
> Right now it says:
>
>  * misc.c
>  *
>  * This is a collection of several routines from gzip-1.0.3
>  * adapted for Linux.
>
> Which was perhaps true 15 years ago, but sure it's not true today! We do
> relocation and KASLR processing in this path as well, which is just as important
> as running the decompressor. Also, gzip is not the only decompressor we support.
>
> The high level purpose of the code in this file should be explained first, and the
> file should probably be renamed to something like extract.c - to signal that this
> file is about extracting a kernel image and preparing it for execution (i.e.
> relinking it, etc.). It's (much!) more than just pure decompression.
>
> Likewise, decompress_kernel() should be renamed to extract_kernel().

I'll get these all cleaned up. Yay comment-bit-rot. :)

> The low level explanations in extract.c should be moved to the function that is
> affected by those details. The top of the file should only contain general
> explanations, a high level description of in what state the kernel is when we call
> this code, and the sort of stuff we do in that code.
>
> The credits should be collected into a single, coherent block. Unnecessary fluff
> should be cut.
>
> Furthermore, I see similar problems with arch/x86/boot/compressed/aslr.c: for
> example it has no high level description at the top of the file _at all_. WTF is
> this with writing security-sensitive code that has no high level design
> description whatsoever?? Also, why is it named aslr.c, why not kaslr.c? We do have
> both KASLR and ASLR code in the kernel, confusing naming on the source code level
> sure does not help.
>
> Also, what's this thing about choose_kernel_location()? That name is actively
> hiding the fact that the main purpose of that function is to randomize things ...
>
> It should be named x86_randomize_kernel_address() or so.

When naming this I wanted to make it clear that the only thing it was
doing was picking a location. The move is done by the decompressor.
I'll try to clarify the name of the function.

> We need to stop making a mess of the x86 boot code! This is a highly critical
> piece of kernel code that every single x86 Linux user will execute, we should
> improve its visual presentation and general readability to the level of being
> proud of it ...

I totally agree. :) There is a bit of a culture of "don't change it
just to rename things" which I try to avoid colliding with. In this
case, though, they would all seriously improve readability.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Powered by blists - more mailing lists