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

[H. Peter Anvin]

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.

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

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

	-hpa