Date: Fri, 15 Apr 2016 12:08:30 +0800 From: Baoquan He <bhe@...hat.com> To: Kees Cook <keescook@...omium.org> Cc: Ingo Molnar <mingo@...nel.org>, Ingo Molnar <mingo@...hat.com>, LKML <linux-kernel@...r.kernel.org>, Yinghai Lu <yinghai@...nel.org>, "H. Peter Anvin" <hpa@...or.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> Subject: Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support On 04/14/16 at 10:56am, Kees Cook wrote: > On Thu, Apr 14, 2016 at 8:06 AM, Baoquan He <bhe@...hat.com> wrote: > > On 04/13/16 at 11:02pm, Kees Cook wrote: > >> On Wed, Apr 13, 2016 at 7:11 AM, Kees Cook <keescook@...omium.org> wrote: > >> > On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar <mingo@...nel.org> wrote: > >> >> > >> >> * Kees Cook <keescook@...omium.org> wrote: > >> >> > >> >>> FWIW, I've also had this tree up in my git branches, and the 0day > >> >>> tester hasn't complained at all about it in the last two weeks. I'd > >> >>> really like to see this in -next to fix the >4G (mainly kexec) issues > >> >>> and get us to feature parity with the arm64 kASLR work (randomized > >> >>> virtual address). > >> > >> So, I've done this and suddenly realized I hadn't boot-tested i386. It > >> doesn't work, unfortunately. (Which I find strange: I'd expect 0day to > >> have noticed...) > >> > >> Baoquan, have you tested this on 32-bit systems? I get a variety of > >> failures. Either it boots okay, it reboots, or I get tons of pte > >> errors like this: > > > > Hi Kees, > > > > I am sorry I didn't notice the change impacts i386. I got a i386 machine > > and had tests. Found i386 can't take separate randomzation since there's > > difference between i386 and x86_64. > > > > x86_64 has phys_base and can translate virt addr and phys addr according > > to below formula: > > > > paddr = vaddr - __START_KERNEL_map + phys_base; > > > > However i386 can only do like this: > > > > paddr = vaddr - PAGE_OFFSET; > > > > Besides i386 has to reserve 128M for VMALLOC at the end of kernel > > virtual address. So for i386 area 768M is the upper limit for > > randomization. But I am fine with the KERNEL_IMAGE_SIZE, the old default > > value. What do you say about this? > > > > So the plan should be keeping the old style of randomization for i386 > > system: > > > > 1) Disable virtual address randomization in i386 case because it's > > useless. This should be done in patch: > > x86, KASLR: Randomize virtual address separately > > > > 2) Add an upper limit for physical randomization if it's i386 system. > > x86, KASLR: Add physical address randomization >4G > > > > I just got a test machine in office, and haven't had time to change > > code. You can change it directly, or I will do it tomorrow. > > I was thinking about the physical vs virtual on i386 as I woke up > today. :) Thanks for confirming! These changes appear to make the > series boot reliably on i386 (pardon any gmail-induced whitespace > damage...): > > diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c > index 5bae54b50d4c..3a58fe8acb8e 100644 > --- a/arch/x86/boot/compressed/aslr.c > +++ b/arch/x86/boot/compressed/aslr.c > @@ -347,6 +347,10 @@ static void process_e820_entry(struct e820entry *entry, > if (entry->type != E820_RAM) > return; > > + /* On 32-bit, ignore entries entirely above our maximum. */ > + if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE) > + return; > + > /* Ignore entries entirely below our minimum. */ > if (entry->addr + entry->size < minimum) > return; > @@ -372,6 +376,11 @@ static void process_e820_entry(struct e820entry *entry, > /* Reduce size by any delta from the original address. */ > region.size -= region.start - start_orig; > > + /* On 32-bit, reduce region size to fit within max size. */ > + if (IS_ENABLED(CONFIG_X86_32) && > + region.start + region.size > KERNEL_IMAGE_SIZE) > + region.size = KERNEL_IMAGE_SIZE - region.start; > + > /* Return if region can't contain decompressed kernel */ > if (region.size < image_size) > return; > @@ -488,6 +497,8 @@ void choose_kernel_location(unsigned char *input, > } > > /* Pick random virtual address starting from LOAD_PHYSICAL_ADDR. */ > - random = find_random_virt_offset(LOAD_PHYSICAL_ADDR, output_size); > + if (IS_ENABLED(CONFIG_X86_64)) > + random = find_random_virt_offset(LOAD_PHYSICAL_ADDR, > + output_size); > *virt_offset = (unsigned char *)random; > } > > > I will split these chunks up into the correct patches and resend the > series. If you get a chance, can you double-check this? Yes, these changes sounds great. I checked the series you posted, and have to say you make them look much better. The change logs are perfect and great code refactoring. Just one little bit thing, here: [kees: rewrote changelog, refactored goto into while, limit 32-bit to 1G] in patch [PATCH v5 19/21] x86, KASLR: Add physical address randomization >4G In i386 KERNEL_IMAGE_SIZE is kept to be 0x20000000, namely 512M, w/o kaslr enabled. So here I guess it's a typo, should be "limit 32-bit to 1G". And what I said is wrong about upper limit yesterday, in fact i386 can put kernel in [16M, 896M), not 768M. But KERNEL_IMAGE_SIZE is good enough for i386 for now.
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.