From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753535AbdEJTFh (ORCPT ); Wed, 10 May 2017 15:05:37 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:36548 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752977AbdEJTFd (ORCPT ); Wed, 10 May 2017 15:05:33 -0400 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (1.0) Subject: Re: [PATCH] efi/libstub: Indicate clang the relocation mode for arm64 From: Ard Biesheuvel X-Mailer: iPhone Mail (14D27) In-Reply-To: <20170510183848.GI128305@google.com> Date: Wed, 10 May 2017 21:05:28 +0200 Cc: Greg Hackmann , Matt Fleming , "linux-efi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Grant Grundler , Michael Davidson , =?utf-8?Q?Bernhard_Rosenkr=C3=A4nzer?= Message-Id: <69A18345-6933-4C6A-8FAC-DBD4D7EF30DE@linaro.org> References: <20170509193612.64105-1-mka@chromium.org> <25d94b46-765e-f12f-a287-853c76782143@google.com> <20170509214905.GH128305@google.com> <20170510183848.GI128305@google.com> To: Matthias Kaehlcke Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v4AJ5fEs014369 > On 10 May 2017, at 20:38, Matthias Kaehlcke wrote: > > Hoi Ard, > > El Wed, May 10, 2017 at 08:51:44AM +0100 Ard Biesheuvel ha dit: > >> On 9 May 2017 at 22:49, Matthias Kaehlcke wrote: >>> El Tue, May 09, 2017 at 01:50:36PM -0700 Greg Hackmann ha dit: >>> >>>> On 05/09/2017 12:36 PM, Matthias Kaehlcke wrote: >>>>> From: Greg Hackmann >>>>> >>>>> Without any extra guidance, clang will generate libstub with either >>>>> absolute or relative ELF relocations. Use the right combination of >>>>> -fpic and -fno-pic on different files to avoid this. >>>>> >>>>> Signed-off-by: Greg Hackmann >>>>> Signed-off-by: Bernhard Rosenkränzer >>>>> Signed-off-by: Matthias Kaehlcke >>>>> --- >>>>> drivers/firmware/efi/libstub/Makefile | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile >>>>> index f7425960f6a5..ccbaaf4d8650 100644 >>>>> --- a/drivers/firmware/efi/libstub/Makefile >>>>> +++ b/drivers/firmware/efi/libstub/Makefile >>>>> @@ -11,6 +11,9 @@ cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 \ >>>>> -mno-mmx -mno-sse >>>>> >>>>> cflags-$(CONFIG_ARM64) := $(subst -pg,,$(KBUILD_CFLAGS)) >>>>> +ifeq ($(cc-name),clang) >>>>> +cflags-$(CONFIG_ARM64) += -fpic >>>>> +endif >>>>> cflags-$(CONFIG_ARM) := $(subst -pg,,$(KBUILD_CFLAGS)) \ >>>>> -fno-builtin -fpic -mno-single-pic-base >>>>> >>>>> @@ -38,6 +41,9 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE >>>>> >>>>> lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o fdt.o string.o random.o \ >>>>> $(patsubst %.c,lib-%.o,$(arm-deps)) >>>>> +ifeq ($(cc-name),clang) >>>>> +CFLAGS_arm64-stub.o += -fno-pic >>>>> +endif >>>>> >>>>> lib-$(CONFIG_ARM) += arm32-stub.o >>>>> lib-$(CONFIG_ARM64) += arm64-stub.o >>>>> >>>> >>>> NAK. >>>> >>>> This patch was labeled "HACK:" in our experimental tree. There's no >>>> rhyme or reason to why this combination of -f[no-]pic flags >>>> generates code without problematic relocations. It's inherently >>>> fragile, and was only intended as a temporary workaround until I (or >>>> someone more familiar with EFI) got a chance to revisit the problem. >>>> >>>> Unless the gcc CFLAGS are also an artifact of "mess with -f[no-]pic >>>> until the compiler generates what you want", this doesn't belong >>>> upstream. >>> >>> Sorry, I didn't realize it is that bad of a hack. Unfortunately I'm >>> not very familiar with EFI either. >>> >>> I saw Ard did some work in this code related with relocation, maybe he >>> can provide a pointer towards a better solution. >>> >> >> This is a known issue. The problem is that generic AArch64 small model >> code is mostly position independent already, due to its use of >> adrp/add pairs to generate symbol references with a +/- 4 GB range. >> Building the same code with -fpic will result in GOT entries to be >> generated, which carry absolute addresses, so this achieves the exact >> opposite of what we want. >> >> The reason for the GOT entries is that GCC (and Clang, apparently) >> infer from the -fpic flag that you are building objects that will be >> linked into a shared library, to which ELF symbol preemption rules >> apply that stipulate that a symbol in the main executable supersedes a >> symbol under the same name in the shared library, and that the shared >> library should update all its internal references to the main >> executable's version of the symbol. The easiest way (but certainly not >> the only way) to achieve that is to indirect all internal symbol >> references via GOT entries, which can be made to refer to another >> symbol by updating a single value. >> >> The workaround I used is to use hidden visibility, using a #pragma. >> (There is a -fvisibility=hidden command line option as well, but this >> is a weaker form that does not apply to extern declarations, only to >> definitions). So if you add >> >> #pragma GCC visibility push(hidden) >> >> at the beginning of arm64-stub.c (and perhaps to one or two other >> files that contain externally visible symbol declarations these days), >> you should be able to compile the entire EFI stub with -fpic. Note >> that making those externally visible symbols 'static' where possible >> would solve the problem as well, but this triggers another issue in >> the 32-bit ARM stub. >> >> In my opinion, the correct fix would be to make -fpie (as opposed to >> -fpic) imply hidden visibility, given that PIE executables don't >> export symbols in the first place, and so the preemption rules do not >> apply. It is worth a try whether -fpie works as expected in this case >> on Clang, but the last time I tried it on GCC, it behaved exactly like >> -fpic. > > Thanks a lot for the detailed description and your suggestions! > > A clang build with -fpie for the EFI stub succeeds without complaints > about GOT entries. I will send out an updated patch (with -fpie only > for clang) later. > Good! I never liked the visibility hack, which is why I never upstreamed it. Could you please check how recent GCC behaves? From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ard Biesheuvel Subject: Re: [PATCH] efi/libstub: Indicate clang the relocation mode for arm64 Date: Wed, 10 May 2017 21:05:28 +0200 Message-ID: <69A18345-6933-4C6A-8FAC-DBD4D7EF30DE@linaro.org> References: <20170509193612.64105-1-mka@chromium.org> <25d94b46-765e-f12f-a287-853c76782143@google.com> <20170509214905.GH128305@google.com> <20170510183848.GI128305@google.com> Mime-Version: 1.0 (1.0) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20170510183848.GI128305-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Matthias Kaehlcke Cc: Greg Hackmann , Matt Fleming , "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Grant Grundler , Michael Davidson , =?utf-8?Q?Bernhard_Rosenkr=C3=A4nzer?= List-Id: linux-efi@vger.kernel.org > On 10 May 2017, at 20:38, Matthias Kaehlcke wrote: >=20 > Hoi Ard, >=20 > El Wed, May 10, 2017 at 08:51:44AM +0100 Ard Biesheuvel ha dit: >=20 >> On 9 May 2017 at 22:49, Matthias Kaehlcke wrote: >>> El Tue, May 09, 2017 at 01:50:36PM -0700 Greg Hackmann ha dit: >>>=20 >>>> On 05/09/2017 12:36 PM, Matthias Kaehlcke wrote: >>>>> From: Greg Hackmann >>>>>=20 >>>>> Without any extra guidance, clang will generate libstub with either >>>>> absolute or relative ELF relocations. Use the right combination of >>>>> -fpic and -fno-pic on different files to avoid this. >>>>>=20 >>>>> Signed-off-by: Greg Hackmann >>>>> Signed-off-by: Bernhard Rosenkr=C3=A4nzer >>>>> Signed-off-by: Matthias Kaehlcke >>>>> --- >>>>> drivers/firmware/efi/libstub/Makefile | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>>=20 >>>>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/= efi/libstub/Makefile >>>>> index f7425960f6a5..ccbaaf4d8650 100644 >>>>> --- a/drivers/firmware/efi/libstub/Makefile >>>>> +++ b/drivers/firmware/efi/libstub/Makefile >>>>> @@ -11,6 +11,9 @@ cflags-$(CONFIG_X86) +=3D -m$(BITS) -D= __KERNEL__ -O2 \ >>>>> -mno-mmx -mno-sse >>>>>=20 >>>>> cflags-$(CONFIG_ARM64) :=3D $(subst -pg,,$(KBUILD_CFLAGS)= ) >>>>> +ifeq ($(cc-name),clang) >>>>> +cflags-$(CONFIG_ARM64) +=3D -fpic >>>>> +endif >>>>> cflags-$(CONFIG_ARM) :=3D $(subst -pg,,$(KBUILD_CFLAGS)= ) \ >>>>> -fno-builtin -fpic -mno-single-pic-base >>>>>=20 >>>>> @@ -38,6 +41,9 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE >>>>>=20 >>>>> lib-$(CONFIG_EFI_ARMSTUB) +=3D arm-stub.o fdt.o string.o random.o \ >>>>> $(patsubst %.c,lib-%.o,$(arm-deps)) >>>>> +ifeq ($(cc-name),clang) >>>>> +CFLAGS_arm64-stub.o +=3D -fno-pic >>>>> +endif >>>>>=20 >>>>> lib-$(CONFIG_ARM) +=3D arm32-stub.o >>>>> lib-$(CONFIG_ARM64) +=3D arm64-stub.o >>>>>=20 >>>>=20 >>>> NAK. >>>>=20 >>>> This patch was labeled "HACK:" in our experimental tree. There's no >>>> rhyme or reason to why this combination of -f[no-]pic flags >>>> generates code without problematic relocations. It's inherently >>>> fragile, and was only intended as a temporary workaround until I (or >>>> someone more familiar with EFI) got a chance to revisit the problem. >>>>=20 >>>> Unless the gcc CFLAGS are also an artifact of "mess with -f[no-]pic >>>> until the compiler generates what you want", this doesn't belong >>>> upstream. >>>=20 >>> Sorry, I didn't realize it is that bad of a hack. Unfortunately I'm >>> not very familiar with EFI either. >>>=20 >>> I saw Ard did some work in this code related with relocation, maybe he >>> can provide a pointer towards a better solution. >>>=20 >>=20 >> This is a known issue. The problem is that generic AArch64 small model >> code is mostly position independent already, due to its use of >> adrp/add pairs to generate symbol references with a +/- 4 GB range. >> Building the same code with -fpic will result in GOT entries to be >> generated, which carry absolute addresses, so this achieves the exact >> opposite of what we want. >>=20 >> The reason for the GOT entries is that GCC (and Clang, apparently) >> infer from the -fpic flag that you are building objects that will be >> linked into a shared library, to which ELF symbol preemption rules >> apply that stipulate that a symbol in the main executable supersedes a >> symbol under the same name in the shared library, and that the shared >> library should update all its internal references to the main >> executable's version of the symbol. The easiest way (but certainly not >> the only way) to achieve that is to indirect all internal symbol >> references via GOT entries, which can be made to refer to another >> symbol by updating a single value. >>=20 >> The workaround I used is to use hidden visibility, using a #pragma. >> (There is a -fvisibility=3Dhidden command line option as well, but this >> is a weaker form that does not apply to extern declarations, only to >> definitions). So if you add >>=20 >> #pragma GCC visibility push(hidden) >>=20 >> at the beginning of arm64-stub.c (and perhaps to one or two other >> files that contain externally visible symbol declarations these days), >> you should be able to compile the entire EFI stub with -fpic. Note >> that making those externally visible symbols 'static' where possible >> would solve the problem as well, but this triggers another issue in >> the 32-bit ARM stub. >>=20 >> In my opinion, the correct fix would be to make -fpie (as opposed to >> -fpic) imply hidden visibility, given that PIE executables don't >> export symbols in the first place, and so the preemption rules do not >> apply. It is worth a try whether -fpie works as expected in this case >> on Clang, but the last time I tried it on GCC, it behaved exactly like >> -fpic. >=20 > Thanks a lot for the detailed description and your suggestions! >=20 > A clang build with -fpie for the EFI stub succeeds without complaints > about GOT entries. I will send out an updated patch (with -fpie only > for clang) later. >=20 Good! I never liked the visibility hack, which is why I never upstreamed it.= Could you please check how recent GCC behaves?=