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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5jLCfMbGCkPYbLPSHax5-GU65xPpjYTscfSkOmZJEv+jrA@mail.gmail.com>
Date:	Thu, 12 May 2016 15:54:24 -0700
From:	Kees Cook <keescook@...omium.org>
To:	"H. Peter Anvin" <hpa@...or.com>
Cc:	Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...e.de>,
	Baoquan He <bhe@...hat.com>, Yinghai Lu <yinghai@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"x86@...nel.org" <x86@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	"H.J. Lu" <hjl.tools@...il.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/boot: Refuse to build with data relocations

On Thu, May 12, 2016 at 3:29 PM, H. Peter Anvin <hpa@...or.com> wrote:
> On May 12, 2016 1:31:04 PM PDT, Kees Cook <keescook@...omium.org> 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@...nel.org>
>>Signed-off-by: Kees Cook <keescook@...omium.org>
>>---
>> 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