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

[H. Peter Anvin]

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

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

	-hpa