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