| 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
| ||
|
Message-ID: <CAKv+Gu9fEAG3CqmORyO2X_Uqse09nnXEQiB1kTL-xBqLWsy8Xg@mail.gmail.com>
Date: Wed, 14 Aug 2019 19:14:42 +0300
From: Ard Biesheuvel <ard.biesheuvel@...aro.org>
To: Kees Cook <keescook@...omium.org>
Cc: Will Deacon <will@...nel.org>,
Nathan Chancellor <natechancellor@...il.com>,
Catalin Marinas <catalin.marinas@....com>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
clang-built-linux <clang-built-linux@...glegroups.com>,
Fangrui Song <maskray@...gle.com>,
Peter Smith <peter.smith@...aro.org>
Subject: Re: [PATCH] arm64/efi: Move variable assignments after SECTIONS
On Wed, 14 Aug 2019 at 02:04, Kees Cook <keescook@...omium.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@...omium.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@...aro.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
Powered by blists - more mailing lists