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

[Kees Cook]

On Thu, May 12, 2016 at 3:29 PM, H. Peter Anvin <hpa@xxxxxxxxx> wrote:
> On May 12, 2016 1:31:04 PM PDT, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>>The compressed kernel is built with -fPIC/-fPIE so that it can run in
>>any
>>location a bootloader happens to put it. However, since ELF relocation
>>processing is not happening (and all the relocation information has
>>already been stripped at link time), none of the code can use data
>>relocations (e.g. static assignments of pointers). This is already
>>noted
>>in a warning comment at the top of misc.c, but this adds an explicit
>>check for the condition during the linking stage to block any such bugs
>>from appearing.
>>
>>If this was in place with the earlier bug in pagetable.c, the build
>>would fail like this:
>>
>>  ...
>>    CC      arch/x86/boot/compressed/pagetable.o
>>    DATAREL arch/x86/boot/compressed/vmlinux
>>  error: arch/x86/boot/compressed/pagetable.o has data relocations!
>>  make[2]: *** [arch/x86/boot/compressed/vmlinux] Error 1
>>  ...
>>
>>A clean build shows the new check:
>>
>>  ...
>>    CC      arch/x86/boot/compressed/pagetable.o
>>    DATAREL arch/x86/boot/compressed/vmlinux
>>    LD      arch/x86/boot/compressed/vmlinux
>>  ...
>>
>>Suggested-by: Ingo Molnar <mingo@xxxxxxxxxx>
>>Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>---
>> arch/x86/boot/compressed/Makefile | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>>diff --git a/arch/x86/boot/compressed/Makefile
>>b/arch/x86/boot/compressed/Makefile
>>index cfdd8c3f8af2..25d477fcd5b4 100644
>>--- a/arch/x86/boot/compressed/Makefile
>>+++ b/arch/x86/boot/compressed/Makefile
>>@@ -85,7 +85,25 @@ vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o
>>$(obj)/efi_stub_$(BITS).o \
>>       $(objtree)/drivers/firmware/efi/libstub/lib.a
>> vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
>>
>>+# The compressed kernel is built with -fPIC/-fPIE so that a boot
>>loader
>>+# can place it anywhere in memory and it will still run. However,
>>since
>>+# it is executed as-is without any ELF relocation processing performed
>>+# (and has already had all relocation sections stripped from the
>>binary),
>>+# none of the code can use data relocations (e.g. static assignments
>>of
>>+# pointer values), since they will be meaningless at runtime. This
>>check
>>+# will refuse to link the vmlinux if any of these relocations are
>>found.
>>+quiet_cmd_check_data_rel = DATAREL $@
>>+define cmd_check_data_rel
>>+      for obj in $(filter %.o,$^); do \
>>+              readelf -S $$obj | grep -qF .data.rel && { \
>>+                      echo "error: $$obj has data relocations!" >&2; \
>>+                      exit 1; \
>>+              } || true; \
>>+      done
>>+endef
>>+
>> $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
>>+      $(call if_changed,check_data_rel)
>>       $(call if_changed,ld)
>>       @:
>>
>
> 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?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security