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

[Kees Cook]

On Tue, May 17, 2016 at 5:31 AM, H. Peter Anvin <hpa@xxxxxxxxx> wrote:
> On 05/17/16 01:13, Kees Cook wrote:
>> On Mon, May 16, 2016 at 3:30 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>>>
>>> * H. Peter Anvin <hpa@xxxxxxxxx> wrote:
>>>
>>>> On 05/12/16 15:54, Kees Cook wrote:
>>>>>>
>>>>>> It would be far better to warn on the *type* of relocations rather than in which section they feel.
>>>>>
>>>>> I'm open to specific changes. What's the best way to detect what you want here?
>>>>>
>>>>
>>>> Use readelf -r and look for inappropriate relocation types (which are
>>>> basically the same ones that we should have to muck with for the main
>>>> kernel in relocs.c.)
>>>
>>> I suspect initially we are good if we don't allow any relocations in
>>> arch/x86/boot/compressed/vmlinux:
>>
>> No, examining vmlinux is already too late, since it was built with a
>> linker script that explicitly drops all sections it doesn't know how
>> to handle (including these "bad" relocations).
>
> 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.)

>> The "good" relocations are those that are either PC-relative or
>> resolved during .o linking (For example, all the assembler .o files
>> have non-PC relocations that are fully resolved after getting linked
>> together into the vmlinux, since those are organized by section.)
>>
>> So, we do specifically need to look at the _section_, not the _type_,
>> to draw any meaningful conclusion. I think my original patch is
>> correct and sufficient.
>
> No.  This is completely wrong, because it *IS* the TYPE that matters.
> You can end up with absolute relocations in text because someone is
> careless writing assembly, for example.
>
> Adding -q to the linker command line will preserve the relocations,
> again, what we do to generate the relocation tables for the kernel
> proper as well as the realmode stub.
>
> 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.

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

Here's my change for relocs, FWIW:

https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=kaslr/relocs&id=d4c4ae0b86d0b895727609296207c4f87ecabf26

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security