linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64/efi: Move variable assignments after SECTIONS
@ 2019-08-13 23:04 Kees Cook
  2019-08-14 16:14 ` Ard Biesheuvel
  0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2019-08-13 23:04 UTC (permalink / raw)
  To: Will Deacon
  Cc: Fangrui Song, Ard Biesheuvel, Catalin Marinas, linux-kernel,
	clang-built-linux, Peter Smith, Nathan Chancellor,
	linux-arm-kernel

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>
---
 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] arm64/efi: Move variable assignments after SECTIONS
  2019-08-13 23:04 [PATCH] arm64/efi: Move variable assignments after SECTIONS Kees Cook
@ 2019-08-14 16:14 ` Ard Biesheuvel
  2019-08-14 16:19   ` Will Deacon
  0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2019-08-14 16:14 UTC (permalink / raw)
  To: Kees Cook
  Cc: Fangrui Song, Catalin Marinas, Linux Kernel Mailing List,
	clang-built-linux, Peter Smith, Nathan Chancellor, Will Deacon,
	linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] arm64/efi: Move variable assignments after SECTIONS
  2019-08-14 16:14 ` Ard Biesheuvel
@ 2019-08-14 16:19   ` Will Deacon
  2019-08-14 16:35     ` Kees Cook
  0 siblings, 1 reply; 4+ messages in thread
From: Will Deacon @ 2019-08-14 16:19 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Kees Cook, Fangrui Song, Catalin Marinas,
	Linux Kernel Mailing List, clang-built-linux, Peter Smith,
	Nathan Chancellor, linux-arm-kernel

On Wed, Aug 14, 2019 at 07:14:42PM +0300, Ard Biesheuvel wrote:
> 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>

Yup, it's gross, but I'll queue it with your ack.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] arm64/efi: Move variable assignments after SECTIONS
  2019-08-14 16:19   ` Will Deacon
@ 2019-08-14 16:35     ` Kees Cook
  0 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2019-08-14 16:35 UTC (permalink / raw)
  To: Will Deacon
  Cc: Fangrui Song, Ard Biesheuvel, Catalin Marinas,
	Linux Kernel Mailing List, clang-built-linux, Peter Smith,
	Nathan Chancellor, linux-arm-kernel

On Wed, Aug 14, 2019 at 05:19:04PM +0100, Will Deacon wrote:
> On Wed, Aug 14, 2019 at 07:14:42PM +0300, Ard Biesheuvel wrote:
> > 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>
> 
> Yup, it's gross, but I'll queue it with your ack.

Thanks, and agreed. :)

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-14 16:35 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-13 23:04 [PATCH] arm64/efi: Move variable assignments after SECTIONS Kees Cook
2019-08-14 16:14 ` Ard Biesheuvel
2019-08-14 16:19   ` Will Deacon
2019-08-14 16:35     ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).