Messages in this thread | | | From | Ard Biesheuvel <> | Date | Wed, 14 Aug 2019 19:14:42 +0300 | Subject | Re: [PATCH] arm64/efi: Move variable assignments after SECTIONS |
| |
On Wed, 14 Aug 2019 at 02:04, Kees Cook <keescook@chromium.org> wrote: > > It seems that LLVM's linker does not correctly handle variable assignments > involving section positions that are updated during the SECTIONS > parsing. Commit aa69fb62bea1 ("arm64/efi: Mark __efistub_stext_offset as > an absolute symbol explicitly") ran into this too, but found a different > workaround. > > However, this was not enough, as other variables were also miscalculated > which manifested as boot failures under UEFI where __efistub__end was > not taking the correct _end value (they should be the same): > > $ ld.lld -EL -maarch64elf --no-undefined -X -shared \ > -Bsymbolic -z notext -z norelro --no-apply-dynamic-relocs \ > -o vmlinux.lld -T poc.lds --whole-archive vmlinux.o && \ > readelf -Ws vmlinux.lld | egrep '\b(__efistub_|)_end\b' > 368272: ffff000002218000 0 NOTYPE LOCAL HIDDEN 38 __efistub__end > 368322: ffff000012318000 0 NOTYPE GLOBAL DEFAULT 38 _end > > $ aarch64-linux-gnu-ld.bfd -EL -maarch64elf --no-undefined -X -shared \ > -Bsymbolic -z notext -z norelro --no-apply-dynamic-relocs \ > -o vmlinux.bfd -T poc.lds --whole-archive vmlinux.o && \ > readelf -Ws vmlinux.bfd | egrep '\b(__efistub_|)_end\b' > 338124: ffff000012318000 0 NOTYPE LOCAL DEFAULT ABS __efistub__end > 383812: ffff000012318000 0 NOTYPE GLOBAL DEFAULT 15325 _end > > To work around this, all of the __efistub_-prefixed variable assignments > need to be moved after the linker script's SECTIONS entry. As it turns > out, this also solves the problem fixed in commit aa69fb62bea1, so those > changes are reverted here. > > Link: https://github.com/ClangBuiltLinux/linux/issues/634 > Link: https://bugs.llvm.org/show_bug.cgi?id=42990 > Signed-off-by: Kees Cook <keescook@chromium.org>
Although it is slightly disappointing that we need to work around this kind of bugs when adding support for a new toolchain, I don't see anything wrong with this patch, so
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> --- > arch/arm64/kernel/image-vars.h | 51 +++++++++++++++++++++++++++++++++ > arch/arm64/kernel/image.h | 42 --------------------------- > arch/arm64/kernel/vmlinux.lds.S | 2 ++ > 3 files changed, 53 insertions(+), 42 deletions(-) > create mode 100644 arch/arm64/kernel/image-vars.h > > diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h > new file mode 100644 > index 000000000000..25a2a9b479c2 > --- /dev/null > +++ b/arch/arm64/kernel/image-vars.h > @@ -0,0 +1,51 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Linker script variables to be set after section resolution, as > + * ld.lld does not like variables assigned before SECTIONS is processed. > + */ > +#ifndef __ARM64_KERNEL_IMAGE_VARS_H > +#define __ARM64_KERNEL_IMAGE_VARS_H > + > +#ifndef LINKER_SCRIPT > +#error This file should only be included in vmlinux.lds.S > +#endif > + > +#ifdef CONFIG_EFI > + > +__efistub_stext_offset = stext - _text; > + > +/* > + * The EFI stub has its own symbol namespace prefixed by __efistub_, to > + * isolate it from the kernel proper. The following symbols are legally > + * accessed by the stub, so provide some aliases to make them accessible. > + * Only include data symbols here, or text symbols of functions that are > + * guaranteed to be safe when executed at another offset than they were > + * linked at. The routines below are all implemented in assembler in a > + * position independent manner > + */ > +__efistub_memcmp = __pi_memcmp; > +__efistub_memchr = __pi_memchr; > +__efistub_memcpy = __pi_memcpy; > +__efistub_memmove = __pi_memmove; > +__efistub_memset = __pi_memset; > +__efistub_strlen = __pi_strlen; > +__efistub_strnlen = __pi_strnlen; > +__efistub_strcmp = __pi_strcmp; > +__efistub_strncmp = __pi_strncmp; > +__efistub_strrchr = __pi_strrchr; > +__efistub___flush_dcache_area = __pi___flush_dcache_area; > + > +#ifdef CONFIG_KASAN > +__efistub___memcpy = __pi_memcpy; > +__efistub___memmove = __pi_memmove; > +__efistub___memset = __pi_memset; > +#endif > + > +__efistub__text = _text; > +__efistub__end = _end; > +__efistub__edata = _edata; > +__efistub_screen_info = screen_info; > + > +#endif > + > +#endif /* __ARM64_KERNEL_IMAGE_VARS_H */ > diff --git a/arch/arm64/kernel/image.h b/arch/arm64/kernel/image.h > index 2b85c0d6fa3d..c7d38c660372 100644 > --- a/arch/arm64/kernel/image.h > +++ b/arch/arm64/kernel/image.h > @@ -65,46 +65,4 @@ > DEFINE_IMAGE_LE64(_kernel_offset_le, TEXT_OFFSET); \ > DEFINE_IMAGE_LE64(_kernel_flags_le, __HEAD_FLAGS); > > -#ifdef CONFIG_EFI > - > -/* > - * Use ABSOLUTE() to avoid ld.lld treating this as a relative symbol: > - * https://github.com/ClangBuiltLinux/linux/issues/561 > - */ > -__efistub_stext_offset = ABSOLUTE(stext - _text); > - > -/* > - * The EFI stub has its own symbol namespace prefixed by __efistub_, to > - * isolate it from the kernel proper. The following symbols are legally > - * accessed by the stub, so provide some aliases to make them accessible. > - * Only include data symbols here, or text symbols of functions that are > - * guaranteed to be safe when executed at another offset than they were > - * linked at. The routines below are all implemented in assembler in a > - * position independent manner > - */ > -__efistub_memcmp = __pi_memcmp; > -__efistub_memchr = __pi_memchr; > -__efistub_memcpy = __pi_memcpy; > -__efistub_memmove = __pi_memmove; > -__efistub_memset = __pi_memset; > -__efistub_strlen = __pi_strlen; > -__efistub_strnlen = __pi_strnlen; > -__efistub_strcmp = __pi_strcmp; > -__efistub_strncmp = __pi_strncmp; > -__efistub_strrchr = __pi_strrchr; > -__efistub___flush_dcache_area = __pi___flush_dcache_area; > - > -#ifdef CONFIG_KASAN > -__efistub___memcpy = __pi_memcpy; > -__efistub___memmove = __pi_memmove; > -__efistub___memset = __pi_memset; > -#endif > - > -__efistub__text = _text; > -__efistub__end = _end; > -__efistub__edata = _edata; > -__efistub_screen_info = screen_info; > - > -#endif > - > #endif /* __ARM64_KERNEL_IMAGE_H */ > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S > index 7fa008374907..803b24d2464a 100644 > --- a/arch/arm64/kernel/vmlinux.lds.S > +++ b/arch/arm64/kernel/vmlinux.lds.S > @@ -245,6 +245,8 @@ SECTIONS > HEAD_SYMBOLS > } > > +#include "image-vars.h" > + > /* > * The HYP init code and ID map text can't be longer than a page each, > * and should not cross a page boundary. > -- > 2.17.1 > > > -- > Kees Cook
| |