Date:	Tue, 17 May 2016 15:28:38 -0400
From:	Kees Cook <keescook@...omium.org>
To:	"H. Peter Anvin" <hpa@...or.com>
Cc:	Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...e.de>,
	Baoquan He <bhe@...hat.com>, Yinghai Lu <yinghai@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"x86@...nel.org" <x86@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	"H.J. Lu" <hjl.tools@...il.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/boot: Refuse to build with data relocations

On Tue, May 17, 2016 at 12:56 PM, H. Peter Anvin <hpa@...or.com> wrote:
> On 05/17/16 06:53, Kees Cook wrote:
>>>
>>> Either look at the inputs, or add the -q option to the link line
>>> (--emit-relocs); that preserves the relocations into the output file
>>> (the same we use to generate the relocation tables to be able to
>>> relocate the kernel proper.)
>>
>> (FWIW, this adds about 20K to the resulting vmlinux.)
>>
>
> Sure, but unless I'm completely out to sea this is only a matter during
> the build; it is not carried into any actual product files.

Ah, yes, found it. You are totally correct:

OBJCOPYFLAGS_vmlinux.bin :=  -R .comment -S
$(obj)/vmlinux.bin: vmlinux FORCE
        $(call if_changed,objcopy)

The -S on vmlinux.bin drops all of it what we retained with the
earlier -q on vmlinux

>>> I have to admit that peeking at the object code I have to wonder if we
>>> wouldn't be better off just doing what we do for the kernel and just
>>> relocate it explicitly.  We already have to relocate the GOT; the code
>>> to do general relocation is no more complex and we already have done it
>>> multiple times.
>>
>> I think if we can just avoid it, we can continue to not have to deal
>> with both the larger code and complexity. We haven't been using these
>> relocations, and there's no driving reason to have them now. This
>> whole thread was to just reliably detect them, since prior to this
>> attempt, there was just a warning in a comment.
>
> Well, actually we *do* deal with a fair bit of it.  I worry that the
> current situation is a lot more fragile than we give it credit for, and
> that makes me nervous (see below.)

Right, the GOT is already adjusted, though it's easy, since it's
already a table. :)

>
>>> I *do* see a disturbing number of absolute references in the current
>>> object; some of those are references to absolute symbols, however.  This
>>> is where leveraging our existing relocs program would be awesome,
>>> because it already has the necessary logic to deal with absolute symbols
>>> and so on.
>>
>> I spent some time last night initially adding ET_REL support to the
>> relocs tool, which is how I ended up deciding that the section was
>> more interesting than the type. Regardless, with the -q on vmlinux,
>> here's the output that changes between a regular build and one with an
>> intentionally bad relocation (from readelf -r):
>>
>> -Relocation section '.rela.data' at offset 0x8cbb60 contains 2 entries:
>> +Relocation section '.rela.data' at offset 0x8cbb78 contains 3 entries:
>>    Offset          Info           Type           Sym. Value  Sym. Name + Addend
>>  0000006c7422  00070000000a R_X86_64_32  00000000006c7420 .data + 0
>>  0000006c74b0  00c900000001 R_X86_64_64  00000000006c4100 efi_call + 0
>> +0000006c74e0  000300000001 R_X86_64_64  00000000006bc9c0 .text + 4de0
>>
>> And similarly, with my modified relocs tool, I see R_X86_64_64
>> relocations in the regular build too, so it didn't seem meaningful.
>> The relocations the tool flags are almost entirely in .head.text, and
>> the remaining two match the readelf report above:
>>
>> reloc section   reloc type      symbol  symbol section
>> ...
>> .data   R_X86_64_32     .data   .data
>> .data   R_X86_64_64     efi_call        .text
>>
>> And a 32-bit build shows a ton of R_386_32 in .text.
>>
>> So, I don't understand why some of these types are okay when others
>> aren't, and it seems like the .rel.data section on the .o files holds
>> the main clue.
>
> 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. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Powered by blists - more mailing lists