Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752073AbcDOTNA (ORCPT ); Fri, 15 Apr 2016 15:13:00 -0400 Received: from mail-wm0-f51.google.com ([74.125.82.51]:37981 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751562AbcDOTM7 (ORCPT ); Fri, 15 Apr 2016 15:12:59 -0400 MIME-Version: 1.0 In-Reply-To: <20160415080715.GD30715@gmail.com> References: <1460672954-32567-1-git-send-email-keescook@chromium.org> <1460672954-32567-4-git-send-email-keescook@chromium.org> <20160415080715.GD30715@gmail.com> Date: Fri, 15 Apr 2016 12:12:56 -0700 X-Google-Sender-Auth: Q1L23Pl0zxPpyN2mhq8sLxpZT1A Message-ID: Subject: Re: [PATCH v5 03/21] x86, KASLR: Drop CONFIG_RANDOMIZE_BASE_MAX_OFFSET From: Kees Cook To: Ingo Molnar Cc: Baoquan He , Yinghai Lu , Ard Biesheuvel , Matt Redfearn , "x86@kernel.org" , "H. Peter Anvin" , Ingo Molnar , Borislav Petkov , Vivek Goyal , Andy Lutomirski , lasse.collin@tukaani.org, Andrew Morton , Dave Young , "kernel-hardening@lists.openwall.com" , LKML , Linus Torvalds , Thomas Gleixner Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7347 Lines: 163 On Fri, Apr 15, 2016 at 1:07 AM, Ingo Molnar wrote: > > * Kees Cook wrote: > >> From: Baoquan He >> >> Currently CONFIG_RANDOMIZE_BASE_MAX_OFFSET is used to limit the maximum >> offset for kernel randomization. This limit doesn't need to be a CONFIG >> since it is tied completely to KERNEL_IMAGE_SIZE, and will make no sense >> once physical and virtual offsets are randomized separately. This patch >> removes CONFIG_RANDOMIZE_BASE_MAX_OFFSET and consolidates the Kconfig >> help text. >> >> Signed-off-by: Baoquan He >> [kees: rewrote changelog, dropped KERNEL_IMAGE_SIZE_DEFAULT, moved earlier] >> Signed-off-by: Kees Cook >> --- >> arch/x86/Kconfig | 57 +++++++++++++----------------------- >> arch/x86/boot/compressed/aslr.c | 12 ++++---- >> arch/x86/include/asm/page_64_types.h | 8 ++--- >> arch/x86/mm/init_32.c | 3 -- >> 4 files changed, 29 insertions(+), 51 deletions(-) >> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> index 2dc18605831f..fd9ac711ada8 100644 >> --- a/arch/x86/Kconfig >> +++ b/arch/x86/Kconfig >> @@ -1936,51 +1936,36 @@ config RANDOMIZE_BASE >> depends on RELOCATABLE >> default n >> ---help--- >> - Randomizes the physical and virtual address at which the >> - kernel image is decompressed, as a security feature that >> - deters exploit attempts relying on knowledge of the location >> - of kernel internals. >> + Randomizes the physical address at which the kernel image >> + is decompressed and the virtual address where the kernel >> + image is mapped, as a secrurity feature that deters exploit > > Guys, please _read_ what you write: s/secrurity/security Gah, sorry. I was reading these, but things slip by. I'll fix it. (And add these to the common misspellings that checkpatch.pl looks for.) > >> + attempts relying on knowledge of the location of kernel >> + internals. >> + >> + The kernel physical address can be randomized from 16M to >> + 64T at most.) > > The 64TB value sure reads weird if you are configuring a 32-bit system ... > > A much better approach would be to split the help text into 32-bit and 64-bit > portions: > > On 64-bit systems the kernel physical address will be randomized from 16M to the > top of available physical memory. (With a maximum of 64TB.) > > On 32-bit systems the kernel physical address will be randomized from 16MB to > 1GB. Yup, good idea. > Also note the assertive tone: if this Kconfig feature is eanbled, we say that the > kernel address _will_ be randomized, and we should make sure it is. (If for some > weird reason randomization fails we should warn prominently during bootup.) This will need some thought... is it better to fail to boot or to boot without entropy? As-is, it's possible to randomly position the kernel base address at exactly the location it was going to boot without KASLR too, yet this is still considered random... > > >> The kernel virtual address will be offset >> + by up to KERNEL_IMAGE_SIZE. On 32-bit KERNEL_IMAGE_SIZE is >> + 512MiB. while on 64-bit this is limited by how the kernel >> + fixmap page table is positioned, so this cannot be larger >> + than 1GiB currently. Without RANDOMIZE_BASE there is a 512MiB >> + to 1.5GiB split between kernel and modules. When RANDOMIZE_BASE >> + is enabled, the modules area will shrink to compensate, up >> + to a 1GiB to 1GiB split, KERNEL_IMAGE_SIZE changes from 512MiB >> + to 1GiB. > > Beyond the broken capitalization, I'll show you what 99.999% of users who are not > kernel hackers will understand from this paragraph, approximately: > > To dream: ay, and them? To bear to sling afterprises > coil, and scover'd cowards of resolence dream: ay, the us for no mome > wish'd. Thus and sweary life, or nobles cast and makes, whips and that > is sicklied of resolence of so long afterprises us more; for whips > all; and name whething after bear to sleep; to beart-ache shocks the > undiscover'd consummative have, but that pith a sleep of somethe under > 'tis the spurns of troud makes off thance doth make whose bourns of > dispriz'd consient and arms more. > > So this is really deep kernel internals, I get a headache trying to interpret it, > and it's my job to interpret this! Please try to formulate key pieces of > information in Kconfig help texts in a more ... approachable fashion, and move the > jargon to .c source code files. Trying to capture the effect of reducing the kernel/module split without detailing the actual numbers may sound evasive, but I'll see what I can do. > >> Entropy is generated using the RDRAND instruction if it is >> supported. If RDTSC is supported, it is used as well. If >> neither RDRAND nor RDTSC are supported, then randomness is >> read from the i8254 timer. > > Also, instead of 'used as well' I'd say "is mixed into the entropy pool as well" > or so, to make sure it's clear that we don't exclusively rely on RDRAND or RDTSC. > > Also, could we always mix the i8254 timer into this as well, not just when RDTSC > is unavailable? IIRC, hpa explicitly did not want to do this when he was making suggestions on this area. I would need to dig out the thread -- I can't find it now. I'd prefer to leave this as-is, since changing it would add yet another variable to the behavioral changes of this series. >> - The kernel will be offset by up to RANDOMIZE_BASE_MAX_OFFSET, >> - and aligned according to PHYSICAL_ALIGN. Since the kernel is >> - built using 2GiB addressing, and PHYSICAL_ALGIN must be at a >> - minimum of 2MiB, only 10 bits of entropy is theoretically >> - possible. At best, due to page table layouts, 64-bit can use >> - 9 bits of entropy and 32-bit uses 8 bits. >> + Since the kernel is built using 2GiB addressing, and >> + PHYSICAL_ALGIN must be at a minimum of 2MiB, only 10 bits of >> + entropy is theoretically possible. At best, due to page table >> + layouts, 64-bit can use 9 bits of entropy and 32-bit uses 8 >> + bits. > > Please read what you write, there's a typo in this section. Gah, I need to teach my spell checker about #defines. ;) I read this multiple times after you called it out and still couldn't see it (http://www.mrc-cbu.cam.ac.uk/people/matt.davis/cmabridge/). Finally dropped the _ and the spell checker flagged it. ;) > Another request: please stop the MiB/GiB nonsense and call it MB/GB. This isn't > storage code that has to fight marketing lies. Only the KASLR section in > arch/x86/Kconfig* is using MiB/GiB, everything else uses MB/GB naming, we should > stick with that. Totally fine by me. I prefer MB/GB. I wonder if it is worth documenting this preference somewhere in the style guide? It's certainly rare in the kernel, but it's present (and there are even #defines for it *sob*). $ git grep '[KMGTP]iB' | wc -l 1239 $ git grep '[KMGTP]B' | wc -l 192251 -Kees -- Kees Cook Chrome OS & Brillo Security