Re: [PATCH] x86/boot: Refuse to build with data relocations

[Kees Cook]

On Thu, May 19, 2016 at 11:41 PM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
>> On Wed, May 18, 2016 at 4:29 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>> >
>> > * Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> >
>> >> > I think there is something way more subtle going on here, and it bothers me
>> >> > exactly because it is subtle.  It may be that it is OK right now, but there
>> >> > are alarm bells going on all over my brain on this.  I'm going to stare at
>> >> > this for a bit and see if I can make sense of it; but if it turns out that
>> >> > what we have is something really problematic it might be better to apply a big
>> >> > hammer and avoid future breakage once and for all.
>> >>
>> >> Sounds good. I would just like to decouple this from the KASLR improvements.
>> >> This fragility hasn't changed as a result of that work, but I'd really like to
>> >> have that series put to bed -- I've spent a lot of time already cleaning up it
>> >> and other areas of the compressed kernel code. :)
>> >
>> > So I disagree on that: while technically kASLR is independent of relocations, your
>> > series already introduced such a relocation bug and I don't want to further
>> > increase complexity via kASLR without first increasing robustness.
>>
>> Well, in my defense, the bug was never actually reachable.
>
> Hm, the changelog says a crash/reboot might happen:
>
> commit 434a6c9f90f7ab5ade619455df01ef5ebea533ee
> Author: Kees Cook <keescook@xxxxxxxxxxxx>
> Date:   Mon May 9 13:22:04 2016 -0700
>
>     x86/KASLR: Initialize mapping_info every time
>
>     As it turns out, mapping_info DOES need to be initialized every
>     time, because pgt_data address could be changed during kernel
>     relocation. So it can not be build time assigned.
>
>     Without this, page tables were not being corrected updated, which
>     could cause reboots when a physical address beyond 2G was chosen.
>
> is the changelog wrong?

It's not wrong in some much as that it is talking about the future.
i.e. without this fix, the future changes would have failed to work. I
talked about it a bit in the v7 "0" email:

- found and fixed a typo in my refactorings that was obscuring a bug in
  my changes to the page table ident mapping code (physical address was
  never being added to identity maps).

So, since the later patches were never in the tree, this bug was never
exposed, and I found and fixed it before sending the v7 batch.

>> > So could we try something to either detect or avoid such subtle and hard to
>> > debug relocation bugs in very early boot code?
>>
>> I've sent this (the readelf patch which detects the bug from the KASLR series),
>> but hpa wants to do a more comprehensive version. Could we temporarily use my
>> version of this, since it appears to accomplish at least a subset of the new
>> goal?
>
> Yeah, that's fine with me.

Do you want me to resend the series? (It'd be unchanged from the v8
and accompanying reloc-detector patch):

https://lkml.org/lkml/2016/5/10/676 <- v8
https://lkml.org/lkml/2016/5/12/721 <- check for reloc sections

>> And on a related topic, how would you like me to send Thomas Garnier's memory
>> base randomization series? Pull request, or as a series like I've done with the
>> other KASLR improvements?
>
> A series (size limited if necessary) would be nice!

Okay, I'll send that out.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security