[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Kees Cook]

On Mon, Apr 4, 2016 at 6:56 PM, Baoquan He <bhe@redhat.com> wrote:
> On 03/22/16 at 03:31pm, Baoquan He wrote:
>> ***Background:
>> Previously a bug is reported that kdump didn't work when kaslr is enabled. During
>> discussing that bug fix, we found current kaslr has a limilation that it can
>> only randomize in 1GB region.
>>
>> This is because in curent kaslr implementaion only physical address of kernel
>> loading is randomized. Then calculate the delta of physical address where
>> vmlinux was linked to load and where it is finally loaded. If delta is not
>> equal to 0, namely there's a new physical address where kernel is actually
>> decompressed, relocation handling need be done. Then delta is added to offset
>> of kernel symbol relocation, this makes the address of kernel text mapping move
>> delta long. Though in principle kernel can be randomized to any physical address,
>> kernel text mapping address space is limited and only 1G, namely as follows on
>> x86_64:
>>       [0xffffffff80000000, 0xffffffffc0000000)
>>
>> In one word, kernel text physical address and virtual address randomization is
>> coupled. This causes the limitation.
>>
>> Then hpa and Vivek suggested we should change this. To decouple the physical
>> address and virtual address randomization of kernel text and let them work
>> separately. Then kernel text physical address can be randomized in region
>> [16M, 64T), and kernel text virtual address can be randomized in region
>> [0xffffffff80000000, 0xffffffffc0000000).
>>
>> ***Problems we need solved:
>>   - For kernel boot from startup_32 case, only 0~4G identity mapping is built.
>>     If kernel will be randomly put anywhere from 16M to 64T at most, the price
>>     to build all region of identity mapping is too high. We need build the
>>     identity mapping on demand, not covering all physical address space.
>>
>>   - Decouple the physical address and virtual address randomization of kernel
>>     text and let them work separately.
>>
>> ***Parts:
>>    - The 1st part is Yinghai's identity mapping building on demand patches.
>>      This is used to solve the first problem mentioned above.
>>      (Patch 09-10/19)
>>    - The 2nd part is decoupling the physical address and virtual address
>>      randomization of kernel text and letting them work separately patches
>>      based on Yinghai's ident mapping patches.
>>      (Patch 12-19/19)
>>    - The 3rd part is some clean up patches which Yinghai found when he reviewed
>>      my patches and the related code around.
>>      (Patch 01-08/19)
>>
>> ***Patch status:
>> This patchset went through several rounds of review.
>>
>>     v1:
>>     - The first round can be found here:
>>       https://lwn.net/Articles/637115/
>>
>>     v1->v2:
>>     - In 2nd round Yinghai made a big patchset including this kaslr fix and another
>>       setup_data related fix. The link is here:
>>        http://lists-archives.com/linux-kernel/28346903-x86-updated-patches-for-kaslr-and-setup_data-etc-for-v4-3.html
>>       You can get the code from Yinghai's git branch:
>>       git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-x86-v4.3-next
>>
>>     v2->v3:
>>     - It only takes care of the kaslr related patches.
>>       For reviewers it's better to discuss only one issue in one thread.
>>         * I take off one patch as follows from Yinghai's because I think it's unnecessay.
>>            - Patch 05/19 x86, kaslr: rename output_size to output_run_size
>>              output_size is enough to represen the value:
>>               output_len > run_size ? output_len : run_size
>>
>>         * I add Patch 04/19, it's a comment update patch. For other patches, I just
>>           adjust patch log and do several places of change comparing with 2nd round.
>>           Please check the change log under patch log of each patch for details.
>>
>>         * Adjust sequence of several patches to make review easier. It doesn't
>>           affect codes.
>>
>>     v3->v4:
>>     - Made changes according to Kees's comments.
>>       Add one patch 20/20 as Kees suggested to use KERNEL_IMAGE_SIZE as offset
>>       max of virtual random, meanwhile clean up useless CONFIG_RANDOM_OFFSET_MAX
>>
>>         x86, kaslr: Use KERNEL_IMAGE_SIZE as the offset max for kernel virtual randomization
>>
>> You can also get this patchset from my github:
>>    https://github.com/baoquan-he/linux.git kaslr-above-4G
>>
>> Any comments about code changes, code comments, patch logs are welcome and
>> appreciated.
>>
>> Baoquan He (9):
>>   x86, kaslr: Fix a bug that relocation can not be handled when kernel
>>     is loaded above 2G
>>   x86, kaskr: Update the description for decompressor worst case
>>   x86, kaslr: Introduce struct slot_area to manage randomization slot
>>     info
>>   x86, kaslr: Add two functions which will be used later
>>   x86, kaslr: Introduce fetch_random_virt_offset to randomize the kernel
>>     text mapping address
>>   x86, kaslr: Randomize physical and virtual address of kernel
>>     separately
>>   x86, kaslr: Add support of kernel physical address randomization above
>>     4G
>>   x86, kaslr: Remove useless codes
>>   x86, kaslr: Use KERNEL_IMAGE_SIZE as the offset max for kernel virtual
>>     randomization
>>
>> Yinghai Lu (11):
>>   x86, kaslr: Remove not needed parameter for choose_kernel_location
>>   x86, boot: Move compressed kernel to end of buffer before
>>     decompressing
>>   x86, boot: Move z_extract_offset calculation to header.S
>>   x86, boot: Fix run_size calculation
>>   x86, kaslr: Clean up useless code related to run_size.
>>   x86, kaslr: Get correct max_addr for relocs pointer
>>   x86, kaslr: Consolidate mem_avoid array filling
>>   x86, boot: Split kernel_ident_mapping_init to another file
>>   x86, 64bit: Set ident_mapping for kaslr
>>   x86, boot: Add checking for memcpy
>>   x86, kaslr: Allow random address to be below loaded address
>>
>>  arch/x86/Kconfig                       |  57 +++----
>>  arch/x86/boot/Makefile                 |  13 +-
>>  arch/x86/boot/compressed/Makefile      |  19 ++-
>>  arch/x86/boot/compressed/aslr.c        | 300 +++++++++++++++++++++++++--------
>>  arch/x86/boot/compressed/head_32.S     |  14 +-
>>  arch/x86/boot/compressed/head_64.S     |  15 +-
>>  arch/x86/boot/compressed/misc.c        |  89 +++++-----
>>  arch/x86/boot/compressed/misc.h        |  34 ++--
>>  arch/x86/boot/compressed/misc_pgt.c    |  93 ++++++++++
>>  arch/x86/boot/compressed/mkpiggy.c     |  28 +--
>>  arch/x86/boot/compressed/string.c      |  29 +++-
>>  arch/x86/boot/compressed/vmlinux.lds.S |   1 +
>>  arch/x86/boot/header.S                 |  22 ++-
>>  arch/x86/include/asm/boot.h            |  19 +++
>>  arch/x86/include/asm/page.h            |   5 +
>>  arch/x86/include/asm/page_64_types.h   |   5 +-
>>  arch/x86/kernel/asm-offsets.c          |   1 +
>>  arch/x86/kernel/vmlinux.lds.S          |   1 +
>>  arch/x86/mm/ident_map.c                |  74 ++++++++
>>  arch/x86/mm/init_32.c                  |   3 -
>>  arch/x86/mm/init_64.c                  |  74 +-------
>>  arch/x86/tools/calc_run_size.sh        |  42 -----
>>  22 files changed, 605 insertions(+), 333 deletions(-)
>>  create mode 100644 arch/x86/boot/compressed/misc_pgt.c
>>  create mode 100644 arch/x86/mm/ident_map.c
>>  delete mode 100644 arch/x86/tools/calc_run_size.sh
>>
> Hi Ingo,
>
> Ping.
>
> Do you have any suggestions or comments for this patchset? As you
> suggested for Yinghai's earlier post, I re-check the whole patchset and
> try to understand them all, and try to re-write patch logs with my own
> understanding. I hope this patch logs can help people who are interested
> in this issue can understand what the patch is doing. Meanwhile Kees
> is very kind to help review the code change and patch log, help adjust
> or re-write some of them.
>
> If still there's something you don't like, your suggestions, ideas and
> comments are welcome and appreciated. I will continue making changes
> until it's satisfactory.

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).

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Ingo Molnar]

* Kees Cook <keescook@chromium.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 started applying the patches, started fixing up changelogs and gave up on 
patch #3.

Changelogs of such non-trivial code need to be proper English and need to be 
understandable.

For example patch #3 starts with:

> Current z_extract_offset is calculated in boot/compressed/mkpiggy.c. The problem 
> is in mkpiggy.c we don't know the detail of decompressor. Then we just get a 
> rough z_extract_offset according to extra_bytes. As we know extra_bytes can only 
> promise a safety margin when decompressing. In fact this brings some risks:

Beyond the bad grammar of the _first word_ of the changelog, this is not a proper 
high level description of the change. A _real_ high level description would be 
something like:

  > Currently z_extract_offset is calculated during kernel build time. The problem 
  > with that method is that at this stage we don't yet know the decompression 
  > buffer sizes - we only know that during bootup.
  >
  > Effects of this are that when we calculate z_extract_offset during the build 
  > we don't know the precise decompression details, we'll only get a rough 
  > estimation of z_extract_offset.
  >
  > Instead of that we want to calculate it during bootup.
  
etc. etc. - the whole series is _full_ of such crappy changelogs that make it 
difficult for me and others to see whether the author actually _understands_ the 
existing code or is hacking away on it. It's also much harder to review and 
validate.

This is totally unacceptable.

Please make sure every changelog starts with a proper high level description that 
tells the story and convinces the reader about what the problem is and what the 
change should be.

And part of that are the patch titles. Things like:

Subject: [PATCH v3 03/19] x86, boot: Move z_extract_offset calculation to header.S

are absolutely mindless titles. A better title would be:

      x86/boot: Calculate precise decompressor parameters during bootup, not build time

... or something like that. Even having read the changelog 3 times I'm unsure what 
the change really is about.

Thanks,

	Ingo

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Kees Cook]

On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.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 started applying the patches, started fixing up changelogs and gave up on
> patch #3.
>
> Changelogs of such non-trivial code need to be proper English and need to be
> understandable.
>
> For example patch #3 starts with:
>
>> Current z_extract_offset is calculated in boot/compressed/mkpiggy.c. The problem
>> is in mkpiggy.c we don't know the detail of decompressor. Then we just get a
>> rough z_extract_offset according to extra_bytes. As we know extra_bytes can only
>> promise a safety margin when decompressing. In fact this brings some risks:
>
> Beyond the bad grammar of the _first word_ of the changelog, this is not a proper
> high level description of the change. A _real_ high level description would be
> something like:
>
>   > Currently z_extract_offset is calculated during kernel build time. The problem
>   > with that method is that at this stage we don't yet know the decompression
>   > buffer sizes - we only know that during bootup.
>   >
>   > Effects of this are that when we calculate z_extract_offset during the build
>   > we don't know the precise decompression details, we'll only get a rough
>   > estimation of z_extract_offset.
>   >
>   > Instead of that we want to calculate it during bootup.
>
> etc. etc. - the whole series is _full_ of such crappy changelogs that make it
> difficult for me and others to see whether the author actually _understands_ the
> existing code or is hacking away on it. It's also much harder to review and
> validate.
>
> This is totally unacceptable.
>
> Please make sure every changelog starts with a proper high level description that
> tells the story and convinces the reader about what the problem is and what the
> change should be.
>
> And part of that are the patch titles. Things like:
>
> Subject: [PATCH v3 03/19] x86, boot: Move z_extract_offset calculation to header.S
>
> are absolutely mindless titles. A better title would be:
>
>       x86/boot: Calculate precise decompressor parameters during bootup, not build time
>
> ... or something like that. Even having read the changelog 3 times I'm unsure what
> the change really is about.

I'll rewrite all the changelogs and resend the series. Thanks taking a look! :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Kees Cook]

On Wed, Apr 13, 2016 at 7:11 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Kees Cook <keescook@chromium.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:

[    0.000000] clearing pte for ram above max_low_pfn: pfn: 37dcc pmd:
f9144f7c pmd phys: 39144f7c pte: f9a1b730 pte phys: 39a1b730

Can you confirm? I suspect relocation problems, but ran out of time
today to debug it.

I have the entire series with cleaned up changelogs and various other
refactorings up here now:

http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr/highmem

Thanks!

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Baoquan He]

On 04/13/16 at 11:02pm, Kees Cook wrote:
> On Wed, Apr 13, 2016 at 7:11 AM, Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >>
> >> * Kees Cook <keescook@chromium.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:
> 
> [    0.000000] clearing pte for ram above max_low_pfn: pfn: 37dcc pmd:
> f9144f7c pmd phys: 39144f7c pte: f9a1b730 pte phys: 39a1b730
> 
> Can you confirm? I suspect relocation problems, but ran out of time
> today to debug it.

Sorry, I didn't test i386 either before. I will get a i386 machine and
check it now. I know it's very late in your side, you need rest. Will
report update if there's any progress.

> 
> I have the entire series with cleaned up changelogs and various other
> refactorings up here now:
> 
> http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr/highmem
> 
> Thanks!
> 
> -Kees
> 
> -- 
> Kees Cook
> Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Baoquan He]

On 04/13/16 at 11:02pm, Kees Cook wrote:
> On Wed, Apr 13, 2016 at 7:11 AM, Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >>
> >> * Kees Cook <keescook@chromium.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.

Thanks

> 
> [    0.000000] clearing pte for ram above max_low_pfn: pfn: 37dcc pmd:
> f9144f7c pmd phys: 39144f7c pte: f9a1b730 pte phys: 39a1b730
> 
> Can you confirm? I suspect relocation problems, but ran out of time
> today to debug it.
> 
> I have the entire series with cleaned up changelogs and various other
> refactorings up here now:
> 
> http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr/highmem
> 
> Thanks!
> 
> -Kees
> 
> -- 
> Kees Cook
> Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Kees Cook]

On Thu, Apr 14, 2016 at 8:06 AM, Baoquan He <bhe@redhat.com> wrote:
> On 04/13/16 at 11:02pm, Kees Cook wrote:
>> On Wed, Apr 13, 2016 at 7:11 AM, Kees Cook <keescook@chromium.org> wrote:
>> > On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >>
>> >> * Kees Cook <keescook@chromium.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?

Thanks!

-Kees


>
> Thanks
>
>>
>> [    0.000000] clearing pte for ram above max_low_pfn: pfn: 37dcc pmd:
>> f9144f7c pmd phys: 39144f7c pte: f9a1b730 pte phys: 39a1b730
>>
>> Can you confirm? I suspect relocation problems, but ran out of time
>> today to debug it.
>>
>> I have the entire series with cleaned up changelogs and various other
>> refactorings up here now:
>>
>> http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr/highmem
>>
>> Thanks!
>>
>> -Kees
>>
>> --
>> Kees Cook
>> Chrome OS & Brillo Security



-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Baoquan He]

On 04/14/16 at 10:56am, Kees Cook wrote:
> On Thu, Apr 14, 2016 at 8:06 AM, Baoquan He <bhe@redhat.com> wrote:
> > On 04/13/16 at 11:02pm, Kees Cook wrote:
> >> On Wed, Apr 13, 2016 at 7:11 AM, Kees Cook <keescook@chromium.org> wrote:
> >> > On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >>
> >> >> * Kees Cook <keescook@chromium.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.

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Kees Cook]

On Thu, Apr 14, 2016 at 9:08 PM, Baoquan He <bhe@redhat.com> wrote:
> On 04/14/16 at 10:56am, Kees Cook wrote:
>> On Thu, Apr 14, 2016 at 8:06 AM, Baoquan He <bhe@redhat.com> wrote:
>> > On 04/13/16 at 11:02pm, Kees Cook wrote:
>> >> On Wed, Apr 13, 2016 at 7:11 AM, Kees Cook <keescook@chromium.org> wrote:
>> >> > On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >> >>
>> >> >> * Kees Cook <keescook@chromium.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.

Ah yeah, thanks. If we do a v6, I'll update the typo. I was going to
say "limit 32-bit to KERNEL_IMAGE_SIZE" but it was going to line-wrap.
:P

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> On Thu, Apr 14, 2016 at 9:08 PM, Baoquan He <bhe@redhat.com> wrote:

> >> 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.
> 
> Ah yeah, thanks. If we do a v6, I'll update the typo. I was going to say "limit 
> 32-bit to KERNEL_IMAGE_SIZE" but it was going to line-wrap.
> :P

No need to resend, I've fixed the changelog.

Thanks,

	Ingo