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

[Kees Cook]

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

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.

-Kees

>
>  fomalhaut:~/linux/linux> readelf -r arch/x86/boot/compressed/vmlinux | grep -q  'There are no relocations in this file' ; echo $?
>  0
>
> versus a regular object file with lots of relocations:
>
>  fomalhaut:~/linux/linux> readelf -r arch/x86/built-in.o | grep -q 'There are no relocations in this file' ; echo $?
>  1
>
> I.e. the relevant portion of Kees's patch would do something like:
>
> quiet_cmd_check_data_rel = DATAREL $@
> define cmd_check_data_rel
>        for obj in $(filter %.o,$^); do \
>                readelf -r $$obj | grep -qF 'There are no relocations in this file' && exit 0 || { \
>                        echo "error: $$obj has data relocations!" >&2; \
>                        exit 1; \
>                } \
>        done
> endef
>
> (totally untested)
>
> Agreed?
>
> Thanks,
>
>         Ingo



-- 
Kees Cook
Chrome OS & Brillo Security