arm64: EFI stub isolation

With the stub to kernel interface being promoted to a proper interface so that other agents than the stub can boot the kernel proper in EFI mode, we can remove the linux,uefi-stub-kern-ver field, considering that its original purpose was to prevent this from happening in the first place.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 Documentation/arm/uefi.txt         | 2 --
 drivers/firmware/efi/libstub/fdt.c | 9 ---------
 2 files changed, 11 deletions(-)

arch/arm64/kernel/image.h +10 -0

@@ -59,4 +59,31 @@
        _kernel_offset_le       = DATA_LE64(TEXT_OFFSET);       \
        _kernel_flags_le        = DATA_LE64(__HEAD_FLAGS);
 
+#ifdef CONFIG_EFI
+
+/*
+ * 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

arch/arm64/kernel/image.h +6 -0

+
+__efistub__text                        = _text;
+__efistub__end                 = _end;
+__efistub__edata               = _edata;
+
+#endif

2015-10-08 22:02 GMT+03:00 Ard Biesheuvel <[email protected]>:

Quoted 21 lines

arch/arm64/kernel/image.h +15 -0

@@ -59,4 +59,31 @@
        _kernel_offset_le       = DATA_LE64(TEXT_OFFSET);       \
        _kernel_flags_le        = DATA_LE64(__HEAD_FLAGS);
 
+#ifdef CONFIG_EFI
+
+/*
+ * 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;

arch/arm64/kernel/image.h line 81

Quoted 5 lines

arch/arm64/kernel/image.h +5 -0

+__efistub_memset               = __pi_memset;
+__efistub_strlen               = __pi_strlen;
+__efistub_strcmp               = __pi_strcmp;
+__efistub_strncmp              = __pi_strncmp;
+__efistub___flush_dcache_area  = __pi___flush_dcache_area;

So why we need these __pi_* aliases?
We could just do __efistub_memcmp = memcmp; Right?

Quoted 22 lines

2015-10-08 22:02 GMT+03:00 Ard Biesheuvel <[email protected]>:

Quoted 21 lines

arch/arm64/kernel/image.h +15 -0

@@ -59,4 +59,31 @@
        _kernel_offset_le       = DATA_LE64(TEXT_OFFSET);       \
        _kernel_flags_le        = DATA_LE64(__HEAD_FLAGS);
 
+#ifdef CONFIG_EFI
+
+/*
+ * 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;

Quoted 8 lines

arch/arm64/kernel/image.h line 81

Quoted 5 lines

arch/arm64/kernel/image.h +5 -0

+__efistub_memset               = __pi_memset;
+__efistub_strlen               = __pi_strlen;
+__efistub_strcmp               = __pi_strcmp;
+__efistub_strncmp              = __pi_strncmp;
+__efistub___flush_dcache_area  = __pi___flush_dcache_area;

So why we need these __pi_* aliases?
We could just do __efistub_memcmp = memcmp; Right?

We *could*, but that defeats the whole purpose of tagging position-independent functions explicitly in the kernel text.

2015-10-09 12:10 GMT+03:00 Will Deacon <[email protected]>:

Quoted 32 lines · Andrey Ryabinin

Quoted 22 lines

2015-10-08 22:02 GMT+03:00 Ard Biesheuvel <[email protected]>:

Quoted 21 lines

arch/arm64/kernel/image.h +15 -0

@@ -59,4 +59,31 @@
        _kernel_offset_le       = DATA_LE64(TEXT_OFFSET);       \
        _kernel_flags_le        = DATA_LE64(__HEAD_FLAGS);
 
+#ifdef CONFIG_EFI
+
+/*
+ * 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;

Quoted 8 lines

arch/arm64/kernel/image.h line 81

Quoted 5 lines

arch/arm64/kernel/image.h +5 -0

+__efistub_memset               = __pi_memset;
+__efistub_strlen               = __pi_strlen;
+__efistub_strcmp               = __pi_strcmp;
+__efistub_strncmp              = __pi_strncmp;
+__efistub___flush_dcache_area  = __pi___flush_dcache_area;

So why we need these __pi_* aliases?
We could just do __efistub_memcmp = memcmp; Right?

Quoted 3 lines

We *could*, but that defeats the whole purpose of tagging position-independent functions explicitly in the kernel text.

I just don't get that "the whole purpose of tagging".

Yes, the EFI stub is allowed to call only PI kernel functions. But the EFI stub already protected by __efistub_ namespace. If we want to use some new PI function in the stub, we would need add it into this list at first.
So that __pi_ namespace doesn't bring any protection or isolation.

Quoted 37 lines

2015-10-09 12:10 GMT+03:00 Will Deacon <[email protected]>:

Quoted 32 lines · Andrey Ryabinin

Quoted 22 lines

2015-10-08 22:02 GMT+03:00 Ard Biesheuvel <[email protected]>:

Quoted 21 lines

arch/arm64/kernel/image.h +15 -0

@@ -59,4 +59,31 @@
        _kernel_offset_le       = DATA_LE64(TEXT_OFFSET);       \
        _kernel_flags_le        = DATA_LE64(__HEAD_FLAGS);
 
+#ifdef CONFIG_EFI
+
+/*
+ * 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;

Quoted 8 lines

arch/arm64/kernel/image.h line 81

Quoted 5 lines

arch/arm64/kernel/image.h +5 -0

+__efistub_memset               = __pi_memset;
+__efistub_strlen               = __pi_strlen;
+__efistub_strcmp               = __pi_strcmp;
+__efistub_strncmp              = __pi_strncmp;
+__efistub___flush_dcache_area  = __pi___flush_dcache_area;

So why we need these __pi_* aliases?
We could just do __efistub_memcmp = memcmp; Right?

Quoted 3 lines

We *could*, but that defeats the whole purpose of tagging position-independent functions explicitly in the kernel text.

Quoted 7 lines

I just don't get that "the whole purpose of tagging".

Yes, the EFI stub is allowed to call only PI kernel functions. But the EFI stub already protected by __efistub_ namespace. If we want to use some new PI function in the stub, we would need add it into this list at first.
So that __pi_ namespace doesn't bring any protection or isolation.

What it does it force people making future changes to the __pi_* functions to think about the stub, otherwise if the function happens to be position-independent when efistub starts using it, it could easily break due to some future patch where the author didn't realise that it was being used in that manner.

Will

Since arm64 does not use a builtin decompressor, the EFI stub is built into the kernel proper. So far, this has been working fine, but actually, since the stub is in fact a PE/COFF relocatable binary that is executed at an unknown offset in the 1:1 mapping provided by the UEFI firmware, we should not be seamlessly sharing code with the kernel proper, which is a position dependent executable linked at a high virtual offset.

So instead, separate the contents of libstub and its dependencies, by putting them into their own namespace by prefixing all of its symbols with __efistub. This way, we have tight control over what parts of the kernel proper are referenced by the stub.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/Makefile            | 12 ++++-
 arch/arm64/kernel/efi-entry.S         | 10 ++--
 arch/arm64/kernel/head.S              | 14 ++---
 arch/arm64/kernel/image.h             | 27 ++++++++++
 drivers/firmware/efi/libstub/Makefile | 39 +++++++++++---
 drivers/firmware/efi/libstub/string.c | 57 ++++++++++++++++++++
 6 files changed, 139 insertions(+), 20 deletions(-)

Since arm64 does not use a builtin decompressor, the EFI stub is built into the kernel proper. So far, this has been working fine, but actually, since the stub is in fact a PE/COFF relocatable binary that is executed at an unknown offset in the 1:1 mapping provided by the UEFI firmware, we should not be seamlessly sharing code with the kernel proper, which is a position dependent executable linked at a high virtual offset.

Quoted 6 lines · Ard Biesheuvel

Since arm64 does not use a builtin decompressor, the EFI stub is built into the kernel proper. So far, this has been working fine, but actually, since the stub is in fact a PE/COFF relocatable binary that is executed at an unknown offset in the 1:1 mapping provided by the UEFI firmware, we should not be seamlessly sharing code with the kernel proper, which is a position dependent executable linked at a high virtual offset.

This patch appears to be causing a build break

STUBCPY drivers/firmware/efi/libstub/lib-sort.stub.o
0000000000000000 R_AARCH64_ABS64 __efistub___crc_sort
drivers/firmware/efi/libstub/lib-sort.stub.o: absolute symbol references not allowed in the EFI stub

__kcrctab_sort:
.xword __crc_sort
.weak __crc_sort
.text

Which is the EXPORT_SYMBOL() in sort.c combined with !CONFIG_MODVERSIONS.

Quoted 24 lines · Jeremy Linton

Quoted 7 lines · Ard Biesheuvel

Since arm64 does not use a builtin decompressor, the EFI stub is built into the kernel proper. So far, this has been working fine, but actually, since the stub is in fact a PE/COFF relocatable binary that is executed at an unknown offset in the 1:1 mapping provided by the UEFI firmware, we should not be seamlessly sharing code with the kernel proper, which is a position dependent executable linked at a high virtual offset.

This patch appears to be causing a build break

STUBCPY drivers/firmware/efi/libstub/lib-sort.stub.o
0000000000000000 R_AARCH64_ABS64 __efistub___crc_sort
drivers/firmware/efi/libstub/lib-sort.stub.o: absolute symbol references not allowed in the EFI stub

__kcrctab_sort:
.xword __crc_sort
.weak __crc_sort
.text

Quoted 1 line

Which is the EXPORT_SYMBOL() in sort.c combined with !CONFIG_MODVERSIONS.

Sorry, I that should be CONFIG_MODVERSIONS..

From 1179099f89db54294f419493d152083fb8e5af3d Mon Sep 17 00:00:00 2001

Thanks for the report. The following patch should fix it

Quoted 2 lines · Ard Biesheuvel

Thanks for the report. The following patch should fix it

Ard,

Thanks, it does fix the build problem...

Now to get the darn thing to boot (not related to this AFAIK).

Quoted 7 lines · Jeremy Linton

Quoted 2 lines · Ard Biesheuvel

Thanks for the report. The following patch should fix it

Ard,

Quoted 4 lines

Thanks, it does fix the build problem...

Now to get the darn thing to boot (not related to this AFAIK).

Catalin,

Would you like me to resend this as a separate patch?

Quoted 5 lines · Jeremy Linton

Quoted 2 lines · Ard Biesheuvel

Thanks for the report. The following patch should fix it

Quoted 5 lines

Ard,

Thanks, it does fix the build problem...

Now to get the darn thing to boot (not related to this AFAIK).

Would you like me to resend this as a separate patch?

From 1179099f89db54294f419493d152083fb8e5af3d Mon Sep 17 00:00:00 2001
From: Ard Biesheuvel <[email protected]>
Date: Tue, 27 Oct 2015 11:12:51 +0900
Subject: [PATCH] arm64/efi: fix libstub build under CONFIG_MODVERSIONS

Now that we strictly forbid absolute relocations in libstub code, make sure that we don't emit any when CONFIG_MODVERSIONS is enabled, by stripping the kcrctab sections from the object file. This fixes a build problem under CONFIG_MODVERSIONS=y.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/libstub/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/firmware/efi/libstub/Makefile +0 -0

index bca9a76cbd33..acc25d7c5da2 100644

Ard,

drivers/firmware/efi/libstub/Makefile lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o fdt.o · line 66

Quoted 5 lines · Ard Biesheuvel

drivers/firmware/efi/libstub/Makefile +5 -0

+quiet_cmd_stubcopy = STUBCPY $@
+      cmd_stubcopy = if $(OBJCOPY) $(STUBCOPY_FLAGS-y) $< $@; then   \
+                  $(OBJDUMP) -r $@ | grep $(STUBCOPY_RELOC-y)        \
+                  && (echo >&2 "$@: absolute symbol references not allowed in the EFI stub"; \
+                      rm -f $@; /bin/false); else /bin/false; fi

This one is causing some more build errors for my setup. Any hint how to fix that?

STUBCPY drivers/firmware/efi/libstub/arm-stub.stub.o

0000000000000006 R_AARCH64_ABS32   .debug_abbrev
000000000000000c R_AARCH64_ABS32   .debug_str+0x0000000000000890
0000000000000011 R_AARCH64_ABS32   .debug_str+0x0000000000004ecd
0000000000000015 R_AARCH64_ABS32   .debug_str+0x0000000000000bb1
0000000000000019 R_AARCH64_ABS64   .init.text
0000000000000021 R_AARCH64_ABS64   .init.text+0x00000000000008cc
0000000000000029 R_AARCH64_ABS32   .debug_line
0000000000000030 R_AARCH64_ABS32   .debug_str+0x00000000000000eb
0000000000000047 R_AARCH64_ABS32   .debug_str+0x0000000000003ca8
0000000000000059 R_AARCH64_ABS32   .debug_str+0x0000000000004798
0000000000000060 R_AARCH64_ABS32   .debug_str+0x0000000000005008
...
000000000000a4bb R_AARCH64_ABS32   .debug_str+0x0000000000004a75
000000000000a4da R_AARCH64_ABS32   .debug_str+0x00000000000029ee
000000000000a4f7 R_AARCH64_ABS32   .debug_str+0x0000000000001d95
000000000000a519 R_AARCH64_ABS32   .debug_str+0x0000000000000773
0000000000000006 R_AARCH64_ABS32   .debug_info
0000000000000010 R_AARCH64_ABS64   .init.text
0000000000000623 R_AARCH64_ABS64   .init.text
0000000000000014 R_AARCH64_ABS32   .debug_frame
0000000000000018 R_AARCH64_ABS64   .init.text
000000000000002c R_AARCH64_ABS32   .debug_frame
0000000000000030 R_AARCH64_ABS64   .init.text+0x0000000000000018
0000000000000074 R_AARCH64_ABS32   .debug_frame
0000000000000078 R_AARCH64_ABS64   .init.text+0x0000000000000140
000000000000009c R_AARCH64_ABS32   .debug_frame
00000000000000a0 R_AARCH64_ABS64   .init.text+0x0000000000000158
00000000000000c4 R_AARCH64_ABS32   .debug_frame
00000000000000c8 R_AARCH64_ABS64   .init.text+0x0000000000000170
000000000000012c R_AARCH64_ABS32   .debug_frame
0000000000000130 R_AARCH64_ABS64   .init.text+0x0000000000000390
0000000000000154 R_AARCH64_ABS32   .debug_frame
0000000000000158 R_AARCH64_ABS64   .init.text+0x00000000000003b0
00000000000001a4 R_AARCH64_ABS32   .debug_frame
00000000000001a8 R_AARCH64_ABS64   .init.text+0x0000000000000780

drivers/firmware/efi/libstub/arm-stub.stub.o: absolute symbol references not allowed in the EFI stub

We need to ensure that the EFI stub only uses parts of the kernel proper that are safe to use when the kernel virtual mapping is not active yet.

So move all C code dependencies to the libstub, which is built with all instrumentation (gcov, kasan) disabled, and do a verification pass to ensure that no absolute relocations are used.

On the arm64 side, annotate all the stub's dependencies as safe for PI (position independent

@Matt: if you are OK with these, may we please have your ack on patches #1 and #2 so that Catalin can pick up the series? Thanks.

Quoted 7 lines · Ard Biesheuvel

We need to ensure that the EFI stub only uses parts of the kernel proper that are safe to use when the kernel virtual mapping is not active yet.

So move all C code dependencies to the libstub, which is built with all instrumentation (gcov, kasan) disabled, and do a verification pass to ensure that no absolute relocations are used.

Quoted 5 lines

On the arm64 side, annotate all the stub's dependencies as safe for PI (position independent

@Matt: if you are OK with these, may we please have your ack on patches #1 and #2 so that Catalin can pick up the series? Thanks.

I assumed you meant PATCH 1 and PATCH 3. If so, yeah, these look fine to be taken through Catalin's tree.