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]
Date:	Wed, 3 Sep 2014 14:18:06 +0200
From:	Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:	Maarten Lankhorst <maarten.lankhorst@...onical.com>
Cc:	Matt Fleming <matt@...sole-pimps.org>,
	Ulf Winkelvos <ulf@...kelvos.de>,
	Matt Fleming <matt.fleming@...el.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	Seth Forshee <seth.forshee@...onical.com>,
	Matthew Garrett <mjg59@...f.ucam.org>
Subject: Re: [REGRESSION] "efi: efistub: Convert into static library" and
 preparation patches

On 3 September 2014 10:27, Maarten Lankhorst
<maarten.lankhorst@...onical.com> wrote:
> Op 03-09-14 om 08:06 schreef Ard Biesheuvel:
>> On 2 September 2014 21:29, Matt Fleming <matt@...sole-pimps.org> wrote:
>>> On Tue, 02 Sep, at 05:25:58PM, Maarten Lankhorst wrote:
>>>> Hey,
>>>>
>>>> My macbook pro 8.2 fails to do a efi stub boot with these patches.
>>>>
>>>> Commit f23cf8bd5c1f49 "efi/x86: efistub: Move shared dependencies to <asm/efi.h>"
>>>> causes the first break, but this can be averted by changing
>>>>
>>>> struct efi_config *efi_early;
>>>>
>>>> to
>>>>
>>>> struct efi_config *efi_early __attribute__((visibility("hidden")));
>>> Weird. That sounds like a bug in the Apple EFI PE loader. Does any other
>>> visibility result in a working kernel?
>>>
>>>> I also need to revert commit f4f75ad5741fe "efi: efistub: Convert into static library"
>>>> to get boot working.
>>> I'll take a look at the symbol changes between these commits and try and
>>> guess what's going on.
>>>
>>>> I'm not an early boot expert, so I have no idea what's going on here.
>>>> Only console output I see when the boot fails is "setup_efi_pci() failed!" after
>>>> the commit that adds this message.
>>> Yeah, that should be unrelated.
>>>
>>> Thanks for the report.
>>>
>> If x86 is anything like ARM in this respect, this is likely caused by
>> the way PIC references to globals are emitted.
>> When using default visibility, a reference to a global is emitted by a
>> reference to the GOT, and an offset into the GOT, and the two are
>> added and dereferenced at runtime. For this to work, the GOT needs to
>> contain the correct absolute address for the global in question. This,
>> in turn, relies on absolute relocations to be resolved at load time,
>> which we don't do with the EFI stub. Could it be that the link address
>> and load address are usually the same on x86 EFI but not on the Mac?
>>
>> With hidden visibility, the PIC reference is emitted using a relative
>> relocation, to which the value of the program counter is added at
>> runtime, and no GOT is involved, and all relocations can be resolved
>> at build time.
>>
>> I think it makes sense to add a #pragma GCC visibility push(hidden) to
>> efistub.h (if pragmas are in fashion these days), making sure that no
>> global references (not even externs) will generate GOT entries.
>>
> Something like that probably.
>
> Following patch FAILS:
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index f277184e2ac1..f9738d92c9ce 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -15,6 +15,8 @@
>
>  #undef memcpy                  /* Use memcpy from misc.c */
>
> +#pragma GCC visibility push(hidden)
> +
>  #include "eboot.h"
>
>  static efi_system_table_t *sys_table;
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 32d5cca30f49..7ef10f36cc1b 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -13,6 +13,8 @@
>  #include <linux/efi.h>
>  #include <asm/efi.h>
>
> +#pragma GCC visibility push(hidden)
> +
>  #include "efistub.h"
>
>  #define EFI_READ_CHUNK_SIZE    (1024 * 1024)
>
> -----
> But this works:
>
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 7a801a310e37..eebd13ee301b 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -30,11 +30,10 @@ VMLINUX_OBJS = $(obj)/vmlinux.lds $(obj)/head_$(BITS).o $(obj)/misc.o \
>         $(obj)/string.o $(obj)/cmdline.o $(obj)/early_serial_console.o \
>         $(obj)/piggy.o $(obj)/cpuflags.o $(obj)/aslr.o
>
> -$(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone
> +$(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone -fvisibility=hidden
>
>  ifeq ($(CONFIG_EFI_STUB), y)
> -       VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
> -                               $(objtree)/drivers/firmware/efi/libstub/lib.a
> +       VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o
>  endif
>
>  $(obj)/vmlinux: $(VMLINUX_OBJS) FORCE
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index f277184e2ac1..e18a265460dd 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -17,6 +17,8 @@
>
>  #include "eboot.h"
>
> +#include "../../../drivers/firmware/efi/libstub/efi-stub-helper.c"
> +
>  static efi_system_table_t *sys_table;
>
>  struct efi_config *efi_early;
>
>
> In case it was caused by lacking -fshort-wchar or order of building, I tried the following, also fails:
> ---
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 7a801a310e37..8f62c273badd 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -30,11 +30,10 @@ VMLINUX_OBJS = $(obj)/vmlinux.lds $(obj)/head_$(BITS).o $(obj)/misc.o \
>         $(obj)/string.o $(obj)/cmdline.o $(obj)/early_serial_console.o \
>         $(obj)/piggy.o $(obj)/cpuflags.o $(obj)/aslr.o
>
> -$(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone
> +$(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone -fvisibility=hidden
>
>  ifeq ($(CONFIG_EFI_STUB), y)
> -       VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
> -                               $(objtree)/drivers/firmware/efi/libstub/lib.a
> +       VMLINUX_OBJS += $(objtree)/drivers/firmware/efi/libstub/efi-stub-helper.o $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o
>  endif
>
>  $(obj)/vmlinux: $(VMLINUX_OBJS) FORCE
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index b14bc2b9fb4d..66c5536cff4d 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -8,7 +8,8 @@ cflags-$(CONFIG_X86_32)         := -march=i386
>  cflags-$(CONFIG_X86_64)                := -mcmodel=small
>  cflags-$(CONFIG_X86)           += -m$(BITS) -D__KERNEL__ $(LINUX_INCLUDE) -O2 \
>                                    -fPIC -fno-strict-aliasing -mno-red-zone \
> -                                  -mno-mmx -mno-sse -DDISABLE_BRANCH_PROFILING
> +                                  -mno-mmx -mno-sse -DDISABLE_BRANCH_PROFILING \
> +                                  -fvisibility=hidden -fshort-wchar
>
>  cflags-$(CONFIG_ARM64)         := $(subst -pg,,$(KBUILD_CFLAGS))
>  cflags-$(CONFIG_ARM)           := $(subst -pg,,$(KBUILD_CFLAGS)) \
>
>
>
> ---
> I have no idea why only directly referencing the file in eboot.c works.
>

Could you please try adding the visibility attribute lik this instead?

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 044a2fd3c5fe..8725d85f1903 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -178,7 +178,7 @@ struct efi_config {
        bool is64;
 } __packed;

-extern struct efi_config *efi_early;
+extern __attribute__((visibility("hidden"))) struct efi_config *efi_early;

 #define efi_call_early(f, ...)                                         \
        efi_early->call(efi_early->f, __VA_ARGS__);

Before this change, I get 18 R_X86_64_GOTPCREL relocations pointing to
efi_early, both in efi-stub-helper.c and eboot.c.
After the change, I get 0, using 'readelf -a
drivers/firmware/efi/libstub/efi-stub-helper.o
arch/x86/boot/compressed/eboot.o|grep GOTPCREL|wc -l'

-- 
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists