All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it
@ 2020-10-27  7:32 Ard Biesheuvel
  2020-10-27  7:32 ` [PATCH 1/4] arm64: efi: increase EFI PE/COFF header padding to 64 KB Ard Biesheuvel
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2020-10-27  7:32 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, catalin.marinas, will, james.morse, Ard Biesheuvel

The EFI header definition was updated recently to increase the section
alignment to 64 KB, which causes the EFI loader to put the kernel Image
at an offset that is guaranteed to be compatible with the kernel's image
placement policy when CONFIG_RELOCATABLE=y, removing the need to move it
around in DRAM before boot.

This change failed to take into account that the first section in the
PE/COFF description should start at a 64 KB aligned boundary as well,
and so even though EFI loaders don't seem to care, the current PE/COFF
layout is not 100% compliant.

So let's fix this by padding the size of the .head region to 64 KB,
and while at it, removing it from the kernel's executable mapping, which
is now guaranteed to be possible regardless of the page size the kernel
is running with. And if we don't map it, we don't need to reserve it
either.

Ard Biesheuvel (4):
  arm64: efi: increase EFI PE/COFF header padding to 64 KB
  arm64: omit [_text, _stext) from permanent kernel mapping
  arm64/head: avoid symbol names pointing into first 64 KB of kernel
    image
  arm64: head: tidy up the Image header definition

 arch/arm64/kernel/efi-header.S  | 86 ++++++++++++--------
 arch/arm64/kernel/head.S        | 19 +----
 arch/arm64/kernel/setup.c       |  4 +-
 arch/arm64/kernel/vmlinux.lds.S |  2 +-
 arch/arm64/mm/init.c            |  2 +-
 arch/arm64/mm/mmu.c             | 10 +--
 6 files changed, 61 insertions(+), 62 deletions(-)

-- 
2.17.1

_______________________________________________
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] 14+ messages in thread

* [PATCH 1/4] arm64: efi: increase EFI PE/COFF header padding to 64 KB
  2020-10-27  7:32 [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it Ard Biesheuvel
@ 2020-10-27  7:32 ` Ard Biesheuvel
  2020-10-27  7:32 ` [PATCH 2/4] arm64: omit [_text, _stext) from permanent kernel mapping Ard Biesheuvel
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2020-10-27  7:32 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, catalin.marinas, will, james.morse, Ard Biesheuvel

Commit 76085aff29f5 ("efi/libstub/arm64: align PE/COFF sections to segment
alignment") increased the PE/COFF section alignment to match the minimum
segment alignment of the kernel image, which ensures that the kernel does
not need to be moved around in memory by the EFI stub if it was built as
relocatable.

However, the first PE/COFF section starts at _stext, which is only 4 KB
aligned, and so the section layout is inconsistent. Existing EFI loaders
seem to care little about this, but it is better to clean this up.

So let's pad the header to 64 KB to match the PE/COFF section alignment.

Fixes: 76085aff29f5 ("efi/libstub/arm64: align PE/COFF sections to ...")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/efi-header.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
index df67c0f2a077..a71844fb923e 100644
--- a/arch/arm64/kernel/efi-header.S
+++ b/arch/arm64/kernel/efi-header.S
@@ -147,6 +147,6 @@ efi_debug_entry:
 	 * correctly at this alignment, we must ensure that .text is
 	 * placed at a 4k boundary in the Image to begin with.
 	 */
-	.align 12
+	.balign	SEGMENT_ALIGN
 efi_header_end:
 	.endm
-- 
2.17.1


_______________________________________________
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] 14+ messages in thread

* [PATCH 2/4] arm64: omit [_text, _stext) from permanent kernel mapping
  2020-10-27  7:32 [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it Ard Biesheuvel
  2020-10-27  7:32 ` [PATCH 1/4] arm64: efi: increase EFI PE/COFF header padding to 64 KB Ard Biesheuvel
@ 2020-10-27  7:32 ` Ard Biesheuvel
  2020-10-28 14:10   ` Will Deacon
  2020-10-27  7:32 ` [PATCH 3/4] arm64/head: avoid symbol names pointing into first 64 KB of kernel image Ard Biesheuvel
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-10-27  7:32 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, catalin.marinas, will, james.morse, Ard Biesheuvel

In a previous patch, we increased the size of the EFI PE/COFF header
to 64 KB, which resulted in the _stext symbol to appear at a fixed
offset of 64 KB into the image.

Since 64 KB is also the largest page size we support, this completely
removes the need to map the first 64 KB of the kernel image, given that
it only contains the arm64 Image header and the EFI header, none of which
we ever access again after booting the kernel. More importantly, we should
avoid an executable mapping of non-executable and not entirely predictable
data, in the unlikely event that we emitted something that looks like an
opcode that could be used as a gadget for speculative execution.

So let's limit the kernel mapping of .text to the [_stext, _etext) region,
which matches the view of generic code (such as kallsyms) when it reasons
about the boundaries of the kernel's .text section.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/efi-header.S  |  7 -------
 arch/arm64/kernel/setup.c       |  4 ++--
 arch/arm64/kernel/vmlinux.lds.S |  2 +-
 arch/arm64/mm/init.c            |  2 +-
 arch/arm64/mm/mmu.c             | 10 +++++-----
 5 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
index a71844fb923e..3ad4aecff033 100644
--- a/arch/arm64/kernel/efi-header.S
+++ b/arch/arm64/kernel/efi-header.S
@@ -140,13 +140,6 @@ efi_debug_entry:
 	.set	efi_debug_entry_size, . - efi_debug_entry
 #endif
 
-	/*
-	 * EFI will load .text onwards at the 4k section alignment
-	 * described in the PE/COFF header. To ensure that instruction
-	 * sequences using an adrp and a :lo12: immediate will function
-	 * correctly at this alignment, we must ensure that .text is
-	 * placed at a 4k boundary in the Image to begin with.
-	 */
 	.balign	SEGMENT_ALIGN
 efi_header_end:
 	.endm
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 133257ffd859..fe1cf52f5f80 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -206,7 +206,7 @@ static void __init request_standard_resources(void)
 	unsigned long i = 0;
 	size_t res_size;
 
-	kernel_code.start   = __pa_symbol(_text);
+	kernel_code.start   = __pa_symbol(_stext);
 	kernel_code.end     = __pa_symbol(__init_begin - 1);
 	kernel_data.start   = __pa_symbol(_sdata);
 	kernel_data.end     = __pa_symbol(_end - 1);
@@ -283,7 +283,7 @@ u64 cpu_logical_map(int cpu)
 
 void __init __no_sanitize_address setup_arch(char **cmdline_p)
 {
-	init_mm.start_code = (unsigned long) _text;
+	init_mm.start_code = (unsigned long) _stext;
 	init_mm.end_code   = (unsigned long) _etext;
 	init_mm.end_data   = (unsigned long) _edata;
 	init_mm.brk	   = (unsigned long) _end;
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 6d78c041fdf6..6567d80dd15f 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -121,7 +121,7 @@ SECTIONS
 		_text = .;
 		HEAD_TEXT
 	}
-	.text : {			/* Real text segment		*/
+	.text : ALIGN(SEGMENT_ALIGN) {	/* Real text segment		*/
 		_stext = .;		/* Text and read-only data	*/
 			IRQENTRY_TEXT
 			SOFTIRQENTRY_TEXT
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 095540667f0f..aa438b9d7f40 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -367,7 +367,7 @@ void __init arm64_memblock_init(void)
 	 * Register the kernel text, kernel data, initrd, and initial
 	 * pagetables with memblock.
 	 */
-	memblock_reserve(__pa_symbol(_text), _end - _text);
+	memblock_reserve(__pa_symbol(_stext), _end - _stext);
 	if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
 		/* the generic initrd code expects virtual addresses */
 		initrd_start = __phys_to_virt(phys_initrd_start);
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 1c0f3e02f731..e6f2accaeade 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -464,14 +464,14 @@ void __init mark_linear_text_alias_ro(void)
 	/*
 	 * Remove the write permissions from the linear alias of .text/.rodata
 	 */
-	update_mapping_prot(__pa_symbol(_text), (unsigned long)lm_alias(_text),
-			    (unsigned long)__init_begin - (unsigned long)_text,
+	update_mapping_prot(__pa_symbol(_stext), (unsigned long)lm_alias(_stext),
+			    (unsigned long)__init_begin - (unsigned long)_stext,
 			    PAGE_KERNEL_RO);
 }
 
 static void __init map_mem(pgd_t *pgdp)
 {
-	phys_addr_t kernel_start = __pa_symbol(_text);
+	phys_addr_t kernel_start = __pa_symbol(_stext);
 	phys_addr_t kernel_end = __pa_symbol(__init_begin);
 	phys_addr_t start, end;
 	int flags = 0;
@@ -506,7 +506,7 @@ static void __init map_mem(pgd_t *pgdp)
 	}
 
 	/*
-	 * Map the linear alias of the [_text, __init_begin) interval
+	 * Map the linear alias of the [_stext, __init_begin) interval
 	 * as non-executable now, and remove the write permission in
 	 * mark_linear_text_alias_ro() below (which will be called after
 	 * alternative patching has completed). This makes the contents
@@ -665,7 +665,7 @@ static void __init map_kernel(pgd_t *pgdp)
 	 * Only rodata will be remapped with different permissions later on,
 	 * all other segments are allowed to use contiguous mappings.
 	 */
-	map_kernel_segment(pgdp, _text, _etext, text_prot, &vmlinux_text, 0,
+	map_kernel_segment(pgdp, _stext, _etext, text_prot, &vmlinux_text, 0,
 			   VM_NO_GUARD);
 	map_kernel_segment(pgdp, __start_rodata, __inittext_begin, PAGE_KERNEL,
 			   &vmlinux_rodata, NO_CONT_MAPPINGS, VM_NO_GUARD);
-- 
2.17.1


_______________________________________________
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] 14+ messages in thread

* [PATCH 3/4] arm64/head: avoid symbol names pointing into first 64 KB of kernel image
  2020-10-27  7:32 [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it Ard Biesheuvel
  2020-10-27  7:32 ` [PATCH 1/4] arm64: efi: increase EFI PE/COFF header padding to 64 KB Ard Biesheuvel
  2020-10-27  7:32 ` [PATCH 2/4] arm64: omit [_text, _stext) from permanent kernel mapping Ard Biesheuvel
@ 2020-10-27  7:32 ` Ard Biesheuvel
  2020-10-28 14:12   ` Will Deacon
  2020-10-27  7:32 ` [PATCH 4/4] arm64: head: tidy up the Image header definition Ard Biesheuvel
  2020-10-28 15:12 ` [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it Will Deacon
  4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-10-27  7:32 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, catalin.marinas, will, james.morse, Ard Biesheuvel

We no longer map the first 64 KB of the kernel image, as there is nothing
there that we ever need to refer back to once the kernel has booted. Even
though facilities like kallsyms are very careful to only refer to the
region that starts at _stext when mapping virtual addresses to symbol
names, let's avoid any confusion by switching to local .L prefixed symbol
names for the EFI header, as none of them have any significance to the
rest of the kernel.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/efi-header.S | 46 ++++++++++----------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
index 3ad4aecff033..ddaf57d825b5 100644
--- a/arch/arm64/kernel/efi-header.S
+++ b/arch/arm64/kernel/efi-header.S
@@ -9,28 +9,26 @@
 
 	.macro	__EFI_PE_HEADER
 	.long	PE_MAGIC
-coff_header:
 	.short	IMAGE_FILE_MACHINE_ARM64		// Machine
-	.short	section_count				// NumberOfSections
+	.short	.Lsection_count				// NumberOfSections
 	.long	0 					// TimeDateStamp
 	.long	0					// PointerToSymbolTable
 	.long	0					// NumberOfSymbols
-	.short	section_table - optional_header		// SizeOfOptionalHeader
+	.short	.Lsection_table - .Loptional_header	// SizeOfOptionalHeader
 	.short	IMAGE_FILE_DEBUG_STRIPPED | \
 		IMAGE_FILE_EXECUTABLE_IMAGE | \
 		IMAGE_FILE_LINE_NUMS_STRIPPED		// Characteristics
 
-optional_header:
+.Loptional_header:
 	.short	PE_OPT_MAGIC_PE32PLUS			// PE32+ format
 	.byte	0x02					// MajorLinkerVersion
 	.byte	0x14					// MinorLinkerVersion
-	.long	__initdata_begin - efi_header_end	// SizeOfCode
+	.long	__initdata_begin - .Lefi_header_end	// SizeOfCode
 	.long	__pecoff_data_size			// SizeOfInitializedData
 	.long	0					// SizeOfUninitializedData
 	.long	__efistub_efi_pe_entry - _head		// AddressOfEntryPoint
-	.long	efi_header_end - _head			// BaseOfCode
+	.long	.Lefi_header_end - _head		// BaseOfCode
 
-extra_header_fields:
 	.quad	0					// ImageBase
 	.long	SEGMENT_ALIGN				// SectionAlignment
 	.long	PECOFF_FILE_ALIGNMENT			// FileAlignment
@@ -45,7 +43,7 @@ extra_header_fields:
 	.long	_end - _head				// SizeOfImage
 
 	// Everything before the kernel image is considered part of the header
-	.long	efi_header_end - _head			// SizeOfHeaders
+	.long	.Lefi_header_end - _head		// SizeOfHeaders
 	.long	0					// CheckSum
 	.short	IMAGE_SUBSYSTEM_EFI_APPLICATION		// Subsystem
 	.short	0					// DllCharacteristics
@@ -54,7 +52,7 @@ extra_header_fields:
 	.quad	0					// SizeOfHeapReserve
 	.quad	0					// SizeOfHeapCommit
 	.long	0					// LoaderFlags
-	.long	(section_table - .) / 8			// NumberOfRvaAndSizes
+	.long	(.Lsection_table - .) / 8		// NumberOfRvaAndSizes
 
 	.quad	0					// ExportTable
 	.quad	0					// ImportTable
@@ -64,17 +62,17 @@ extra_header_fields:
 	.quad	0					// BaseRelocationTable
 
 #ifdef CONFIG_DEBUG_EFI
-	.long	efi_debug_table - _head			// DebugTable
-	.long	efi_debug_table_size
+	.long	.Lefi_debug_table - _head		// DebugTable
+	.long	.Lefi_debug_table_size
 #endif
 
 	// Section table
-section_table:
+.Lsection_table:
 	.ascii	".text\0\0\0"
-	.long	__initdata_begin - efi_header_end	// VirtualSize
-	.long	efi_header_end - _head			// VirtualAddress
-	.long	__initdata_begin - efi_header_end	// SizeOfRawData
-	.long	efi_header_end - _head			// PointerToRawData
+	.long	__initdata_begin - .Lefi_header_end	// VirtualSize
+	.long	.Lefi_header_end - _head		// VirtualAddress
+	.long	__initdata_begin - .Lefi_header_end	// SizeOfRawData
+	.long	.Lefi_header_end - _head		// PointerToRawData
 
 	.long	0					// PointerToRelocations
 	.long	0					// PointerToLineNumbers
@@ -98,7 +96,7 @@ section_table:
 		IMAGE_SCN_MEM_READ | \
 		IMAGE_SCN_MEM_WRITE			// Characteristics
 
-	.set	section_count, (. - section_table) / 40
+	.set	.Lsection_count, (. - .Lsection_table) / 40
 
 #ifdef CONFIG_DEBUG_EFI
 	/*
@@ -114,21 +112,21 @@ section_table:
 	__INITRODATA
 
 	.align	2
-efi_debug_table:
+.Lefi_debug_table:
 	// EFI_IMAGE_DEBUG_DIRECTORY_ENTRY
 	.long	0					// Characteristics
 	.long	0					// TimeDateStamp
 	.short	0					// MajorVersion
 	.short	0					// MinorVersion
 	.long	IMAGE_DEBUG_TYPE_CODEVIEW		// Type
-	.long	efi_debug_entry_size			// SizeOfData
+	.long	.Lefi_debug_entry_size			// SizeOfData
 	.long	0					// RVA
-	.long	efi_debug_entry - _head			// FileOffset
+	.long	.Lefi_debug_entry - _head		// FileOffset
 
-	.set	efi_debug_table_size, . - efi_debug_table
+	.set	.Lefi_debug_table_size, . - .Lefi_debug_table
 	.previous
 
-efi_debug_entry:
+.Lefi_debug_entry:
 	// EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY
 	.ascii	"NB10"					// Signature
 	.long	0					// Unknown
@@ -137,9 +135,9 @@ efi_debug_entry:
 
 	.asciz	VMLINUX_PATH
 
-	.set	efi_debug_entry_size, . - efi_debug_entry
+	.set	.Lefi_debug_entry_size, . - .Lefi_debug_entry
 #endif
 
 	.balign	SEGMENT_ALIGN
-efi_header_end:
+.Lefi_header_end:
 	.endm
-- 
2.17.1


_______________________________________________
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] 14+ messages in thread

* [PATCH 4/4] arm64: head: tidy up the Image header definition
  2020-10-27  7:32 [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it Ard Biesheuvel
                   ` (2 preceding siblings ...)
  2020-10-27  7:32 ` [PATCH 3/4] arm64/head: avoid symbol names pointing into first 64 KB of kernel image Ard Biesheuvel
@ 2020-10-27  7:32 ` Ard Biesheuvel
  2020-10-28 14:17   ` Will Deacon
  2020-10-28 15:12 ` [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it Will Deacon
  4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-10-27  7:32 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, catalin.marinas, will, james.morse, Ard Biesheuvel

Even though support for EFI boot remains entirely optional for arm64,
it is unlikely that we will ever be able to repurpose the image header
fields that the EFI loader relies on, i.e., the magic NOP at offset
0x0 and the PE header address at offset 0x3c.

So let's factor out the differences into a 'magic_nop' macro and a local
symbol representing the PE header address, and move the conditional
definitions into efi-header.S, taking into account whether CONFIG_EFI is
enabled or not.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/efi-header.S | 43 +++++++++++++++-----
 arch/arm64/kernel/head.S       | 19 +--------
 2 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
index ddaf57d825b5..7b7ac4316d95 100644
--- a/arch/arm64/kernel/efi-header.S
+++ b/arch/arm64/kernel/efi-header.S
@@ -7,7 +7,27 @@
 #include <linux/pe.h>
 #include <linux/sizes.h>
 
+	.macro	magic_nop
+#ifdef CONFIG_EFI
+.L_head:
+	/*
+	 * This add instruction has no meaningful effect except that
+	 * its opcode forms the magic "MZ" signature required by UEFI.
+	 */
+	add	x13, x18, #0x16
+#else
+	/*
+	 * Bootloaders may inspect the opcode at the start of the kernel
+	 * image to decide if the kernel is capable of booting via UEFI.
+	 * So put an ordinary NOP here, not the "MZ.." pseudo-nop above.
+	 */
+	nop
+#endif
+	.endm
+
 	.macro	__EFI_PE_HEADER
+#ifdef CONFIG_EFI
+	.set	.Lpe_header_offset, . - .L_head
 	.long	PE_MAGIC
 	.short	IMAGE_FILE_MACHINE_ARM64		// Machine
 	.short	.Lsection_count				// NumberOfSections
@@ -26,8 +46,8 @@
 	.long	__initdata_begin - .Lefi_header_end	// SizeOfCode
 	.long	__pecoff_data_size			// SizeOfInitializedData
 	.long	0					// SizeOfUninitializedData
-	.long	__efistub_efi_pe_entry - _head		// AddressOfEntryPoint
-	.long	.Lefi_header_end - _head		// BaseOfCode
+	.long	__efistub_efi_pe_entry - .L_head	// AddressOfEntryPoint
+	.long	.Lefi_header_end - .L_head		// BaseOfCode
 
 	.quad	0					// ImageBase
 	.long	SEGMENT_ALIGN				// SectionAlignment
@@ -40,10 +60,10 @@
 	.short	0					// MinorSubsystemVersion
 	.long	0					// Win32VersionValue
 
-	.long	_end - _head				// SizeOfImage
+	.long	_end - .L_head				// SizeOfImage
 
 	// Everything before the kernel image is considered part of the header
-	.long	.Lefi_header_end - _head		// SizeOfHeaders
+	.long	.Lefi_header_end - .L_head		// SizeOfHeaders
 	.long	0					// CheckSum
 	.short	IMAGE_SUBSYSTEM_EFI_APPLICATION		// Subsystem
 	.short	0					// DllCharacteristics
@@ -62,7 +82,7 @@
 	.quad	0					// BaseRelocationTable
 
 #ifdef CONFIG_DEBUG_EFI
-	.long	.Lefi_debug_table - _head		// DebugTable
+	.long	.Lefi_debug_table - .L_head		// DebugTable
 	.long	.Lefi_debug_table_size
 #endif
 
@@ -70,9 +90,9 @@
 .Lsection_table:
 	.ascii	".text\0\0\0"
 	.long	__initdata_begin - .Lefi_header_end	// VirtualSize
-	.long	.Lefi_header_end - _head		// VirtualAddress
+	.long	.Lefi_header_end - .L_head		// VirtualAddress
 	.long	__initdata_begin - .Lefi_header_end	// SizeOfRawData
-	.long	.Lefi_header_end - _head		// PointerToRawData
+	.long	.Lefi_header_end - .L_head		// PointerToRawData
 
 	.long	0					// PointerToRelocations
 	.long	0					// PointerToLineNumbers
@@ -84,9 +104,9 @@
 
 	.ascii	".data\0\0\0"
 	.long	__pecoff_data_size			// VirtualSize
-	.long	__initdata_begin - _head		// VirtualAddress
+	.long	__initdata_begin - .L_head		// VirtualAddress
 	.long	__pecoff_data_rawsize			// SizeOfRawData
-	.long	__initdata_begin - _head		// PointerToRawData
+	.long	__initdata_begin - .L_head		// PointerToRawData
 
 	.long	0					// PointerToRelocations
 	.long	0					// PointerToLineNumbers
@@ -121,7 +141,7 @@
 	.long	IMAGE_DEBUG_TYPE_CODEVIEW		// Type
 	.long	.Lefi_debug_entry_size			// SizeOfData
 	.long	0					// RVA
-	.long	.Lefi_debug_entry - _head		// FileOffset
+	.long	.Lefi_debug_entry - .L_head		// FileOffset
 
 	.set	.Lefi_debug_table_size, . - .Lefi_debug_table
 	.previous
@@ -140,4 +160,7 @@
 
 	.balign	SEGMENT_ALIGN
 .Lefi_header_end:
+#else
+	.set	.Lpe_header_offset, 0x0
+#endif
 	.endm
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index d8d9caf02834..086033f9c684 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -58,21 +58,11 @@
  * in the entry routines.
  */
 	__HEAD
-_head:
 	/*
 	 * DO NOT MODIFY. Image header expected by Linux boot-loaders.
 	 */
-#ifdef CONFIG_EFI
-	/*
-	 * This add instruction has no meaningful effect except that
-	 * its opcode forms the magic "MZ" signature required by UEFI.
-	 */
-	add	x13, x18, #0x16
-	b	primary_entry
-#else
+	magic_nop				// magic signature NOP
 	b	primary_entry			// branch to kernel start, magic
-	.long	0				// reserved
-#endif
 	.quad	0				// Image load offset from start of RAM, little-endian
 	le64sym	_kernel_size_le			// Effective size of kernel image, little-endian
 	le64sym	_kernel_flags_le		// Informative flags, little-endian
@@ -80,14 +70,9 @@ _head:
 	.quad	0				// reserved
 	.quad	0				// reserved
 	.ascii	ARM64_IMAGE_MAGIC		// Magic number
-#ifdef CONFIG_EFI
-	.long	pe_header - _head		// Offset to the PE header.
+	.long	.Lpe_header_offset		// Offset to the PE header.
 
-pe_header:
 	__EFI_PE_HEADER
-#else
-	.long	0				// reserved
-#endif
 
 	__INIT
 
-- 
2.17.1


_______________________________________________
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] 14+ messages in thread

* Re: [PATCH 2/4] arm64: omit [_text, _stext) from permanent kernel mapping
  2020-10-27  7:32 ` [PATCH 2/4] arm64: omit [_text, _stext) from permanent kernel mapping Ard Biesheuvel
@ 2020-10-28 14:10   ` Will Deacon
  0 siblings, 0 replies; 14+ messages in thread
From: Will Deacon @ 2020-10-28 14:10 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: mark.rutland, catalin.marinas, james.morse, linux-arm-kernel

On Tue, Oct 27, 2020 at 08:32:07AM +0100, Ard Biesheuvel wrote:
> In a previous patch, we increased the size of the EFI PE/COFF header
> to 64 KB, which resulted in the _stext symbol to appear at a fixed
> offset of 64 KB into the image.
> 
> Since 64 KB is also the largest page size we support, this completely
> removes the need to map the first 64 KB of the kernel image, given that
> it only contains the arm64 Image header and the EFI header, none of which
> we ever access again after booting the kernel. More importantly, we should
> avoid an executable mapping of non-executable and not entirely predictable
> data, in the unlikely event that we emitted something that looks like an
> opcode that could be used as a gadget for speculative execution.
> 
> So let's limit the kernel mapping of .text to the [_stext, _etext) region,
> which matches the view of generic code (such as kallsyms) when it reasons
> about the boundaries of the kernel's .text section.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm64/kernel/efi-header.S  |  7 -------
>  arch/arm64/kernel/setup.c       |  4 ++--
>  arch/arm64/kernel/vmlinux.lds.S |  2 +-
>  arch/arm64/mm/init.c            |  2 +-
>  arch/arm64/mm/mmu.c             | 10 +++++-----
>  5 files changed, 9 insertions(+), 16 deletions(-)

Acked-by: Will Deacon <will@kernel.org>

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] 14+ messages in thread

* Re: [PATCH 3/4] arm64/head: avoid symbol names pointing into first 64 KB of kernel image
  2020-10-27  7:32 ` [PATCH 3/4] arm64/head: avoid symbol names pointing into first 64 KB of kernel image Ard Biesheuvel
@ 2020-10-28 14:12   ` Will Deacon
  0 siblings, 0 replies; 14+ messages in thread
From: Will Deacon @ 2020-10-28 14:12 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: mark.rutland, catalin.marinas, james.morse, linux-arm-kernel

On Tue, Oct 27, 2020 at 08:32:08AM +0100, Ard Biesheuvel wrote:
> We no longer map the first 64 KB of the kernel image, as there is nothing
> there that we ever need to refer back to once the kernel has booted. Even
> though facilities like kallsyms are very careful to only refer to the
> region that starts at _stext when mapping virtual addresses to symbol
> names, let's avoid any confusion by switching to local .L prefixed symbol
> names for the EFI header, as none of them have any significance to the
> rest of the kernel.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm64/kernel/efi-header.S | 46 ++++++++++----------
>  1 file changed, 22 insertions(+), 24 deletions(-)

Acked-by: Will Deacon <will@kernel.org>

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] 14+ messages in thread

* Re: [PATCH 4/4] arm64: head: tidy up the Image header definition
  2020-10-27  7:32 ` [PATCH 4/4] arm64: head: tidy up the Image header definition Ard Biesheuvel
@ 2020-10-28 14:17   ` Will Deacon
  2020-10-28 17:56     ` Robin Murphy
  0 siblings, 1 reply; 14+ messages in thread
From: Will Deacon @ 2020-10-28 14:17 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: mark.rutland, catalin.marinas, james.morse, linux-arm-kernel

On Tue, Oct 27, 2020 at 08:32:09AM +0100, Ard Biesheuvel wrote:
> Even though support for EFI boot remains entirely optional for arm64,
> it is unlikely that we will ever be able to repurpose the image header
> fields that the EFI loader relies on, i.e., the magic NOP at offset
> 0x0 and the PE header address at offset 0x3c.
> 
> So let's factor out the differences into a 'magic_nop' macro and a local
> symbol representing the PE header address, and move the conditional
> definitions into efi-header.S, taking into account whether CONFIG_EFI is
> enabled or not.

How many architectures can claim to have both a "magic nop" and a
"mysterious nop", hey?

> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm64/kernel/efi-header.S | 43 +++++++++++++++-----
>  arch/arm64/kernel/head.S       | 19 +--------
>  2 files changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
> index ddaf57d825b5..7b7ac4316d95 100644
> --- a/arch/arm64/kernel/efi-header.S
> +++ b/arch/arm64/kernel/efi-header.S
> @@ -7,7 +7,27 @@
>  #include <linux/pe.h>
>  #include <linux/sizes.h>
>  
> +	.macro	magic_nop
> +#ifdef CONFIG_EFI
> +.L_head:
> +	/*
> +	 * This add instruction has no meaningful effect except that
> +	 * its opcode forms the magic "MZ" signature required by UEFI.
> +	 */
> +	add	x13, x18, #0x16

It's probably faster too ;)

> +#else
> +	/*
> +	 * Bootloaders may inspect the opcode at the start of the kernel
> +	 * image to decide if the kernel is capable of booting via UEFI.
> +	 * So put an ordinary NOP here, not the "MZ.." pseudo-nop above.
> +	 */
> +	nop

Let's just hope nobody was decoding the branch instruction...

Acked-by: Will Deacon >will@kernel.org>

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] 14+ messages in thread

* Re: [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it
  2020-10-27  7:32 [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it Ard Biesheuvel
                   ` (3 preceding siblings ...)
  2020-10-27  7:32 ` [PATCH 4/4] arm64: head: tidy up the Image header definition Ard Biesheuvel
@ 2020-10-28 15:12 ` Will Deacon
  4 siblings, 0 replies; 14+ messages in thread
From: Will Deacon @ 2020-10-28 15:12 UTC (permalink / raw)
  To: linux-arm-kernel, Ard Biesheuvel
  Cc: mark.rutland, catalin.marinas, kernel-team, james.morse, Will Deacon

On Tue, 27 Oct 2020 08:32:05 +0100, Ard Biesheuvel wrote:
> The EFI header definition was updated recently to increase the section
> alignment to 64 KB, which causes the EFI loader to put the kernel Image
> at an offset that is guaranteed to be compatible with the kernel's image
> placement policy when CONFIG_RELOCATABLE=y, removing the need to move it
> around in DRAM before boot.
> 
> This change failed to take into account that the first section in the
> PE/COFF description should start at a 64 KB aligned boundary as well,
> and so even though EFI loaders don't seem to care, the current PE/COFF
> layout is not 100% compliant.
> 
> [...]

Applied the first patch to arm64 (for-next/fixes), thanks!

[1/4] arm64: efi: increase EFI PE/COFF header padding to 64 KB
      https://git.kernel.org/arm64/c/a2d50c1c77aa

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
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] 14+ messages in thread

* Re: [PATCH 4/4] arm64: head: tidy up the Image header definition
  2020-10-28 14:17   ` Will Deacon
@ 2020-10-28 17:56     ` Robin Murphy
  2020-10-29  7:30       ` Ard Biesheuvel
  0 siblings, 1 reply; 14+ messages in thread
From: Robin Murphy @ 2020-10-28 17:56 UTC (permalink / raw)
  To: Will Deacon, Ard Biesheuvel
  Cc: mark.rutland, catalin.marinas, james.morse, linux-arm-kernel

On 2020-10-28 14:17, Will Deacon wrote:
> On Tue, Oct 27, 2020 at 08:32:09AM +0100, Ard Biesheuvel wrote:
>> Even though support for EFI boot remains entirely optional for arm64,
>> it is unlikely that we will ever be able to repurpose the image header
>> fields that the EFI loader relies on, i.e., the magic NOP at offset
>> 0x0 and the PE header address at offset 0x3c.
>>
>> So let's factor out the differences into a 'magic_nop' macro and a local
>> symbol representing the PE header address, and move the conditional
>> definitions into efi-header.S, taking into account whether CONFIG_EFI is
>> enabled or not.
> 
> How many architectures can claim to have both a "magic nop" and a
> "mysterious nop", hey?

It's fun 'n'all, but putting my serious hat on for a moment, if the name 
still requires a comment to explain it at point of use, it's not a very 
good name :(

At worst magic_nop is even potentially misleading, since it's not 
necessarily a nop; there's no mention of the implicit dependency on a 
context where the side-effect of executing it wouldn't affect anything 
important.

Could we call the macro itself something clear and self-explanatory like 
efi_signature_insn please? I'm happy for it to be *commented* as "Magic 
NOP" if you want parity with the VDSO :D

Robin.

>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   arch/arm64/kernel/efi-header.S | 43 +++++++++++++++-----
>>   arch/arm64/kernel/head.S       | 19 +--------
>>   2 files changed, 35 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
>> index ddaf57d825b5..7b7ac4316d95 100644
>> --- a/arch/arm64/kernel/efi-header.S
>> +++ b/arch/arm64/kernel/efi-header.S
>> @@ -7,7 +7,27 @@
>>   #include <linux/pe.h>
>>   #include <linux/sizes.h>
>>   
>> +	.macro	magic_nop
>> +#ifdef CONFIG_EFI
>> +.L_head:
>> +	/*
>> +	 * This add instruction has no meaningful effect except that
>> +	 * its opcode forms the magic "MZ" signature required by UEFI.
>> +	 */
>> +	add	x13, x18, #0x16
> 
> It's probably faster too ;)
> 
>> +#else
>> +	/*
>> +	 * Bootloaders may inspect the opcode at the start of the kernel
>> +	 * image to decide if the kernel is capable of booting via UEFI.
>> +	 * So put an ordinary NOP here, not the "MZ.." pseudo-nop above.
>> +	 */
>> +	nop
> 
> Let's just hope nobody was decoding the branch instruction...
> 
> Acked-by: Will Deacon >will@kernel.org>
> 
> Will
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

_______________________________________________
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] 14+ messages in thread

* Re: [PATCH 4/4] arm64: head: tidy up the Image header definition
  2020-10-28 17:56     ` Robin Murphy
@ 2020-10-29  7:30       ` Ard Biesheuvel
  2020-10-29 13:06         ` Robin Murphy
  0 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-10-29  7:30 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Mark Rutland, Catalin Marinas, Will Deacon, James Morse, Linux ARM

On Wed, 28 Oct 2020 at 18:56, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-10-28 14:17, Will Deacon wrote:
> > On Tue, Oct 27, 2020 at 08:32:09AM +0100, Ard Biesheuvel wrote:
> >> Even though support for EFI boot remains entirely optional for arm64,
> >> it is unlikely that we will ever be able to repurpose the image header
> >> fields that the EFI loader relies on, i.e., the magic NOP at offset
> >> 0x0 and the PE header address at offset 0x3c.
> >>
> >> So let's factor out the differences into a 'magic_nop' macro and a local
> >> symbol representing the PE header address, and move the conditional
> >> definitions into efi-header.S, taking into account whether CONFIG_EFI is
> >> enabled or not.
> >
> > How many architectures can claim to have both a "magic nop" and a
> > "mysterious nop", hey?
>
> It's fun 'n'all, but putting my serious hat on for a moment, if the name
> still requires a comment to explain it at point of use, it's not a very
> good name :(
>
> At worst magic_nop is even potentially misleading, since it's not
> necessarily a nop; there's no mention of the implicit dependency on a
> context where the side-effect of executing it wouldn't affect anything
> important.
>
> Could we call the macro itself something clear and self-explanatory like
> efi_signature_insn please? I'm happy for it to be *commented* as "Magic
> NOP" if you want parity with the VDSO :D
>

Will efi_pseudo_nop do?

Also, do you think it would be better to use an opcode here that has
no architectural side effects, such as a PRFM (literal) instruction?
It is obviously not going to make a difference in practice, but it
always annoyed me that the pseudo NOP is not a NOP.





> >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >> ---
> >>   arch/arm64/kernel/efi-header.S | 43 +++++++++++++++-----
> >>   arch/arm64/kernel/head.S       | 19 +--------
> >>   2 files changed, 35 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
> >> index ddaf57d825b5..7b7ac4316d95 100644
> >> --- a/arch/arm64/kernel/efi-header.S
> >> +++ b/arch/arm64/kernel/efi-header.S
> >> @@ -7,7 +7,27 @@
> >>   #include <linux/pe.h>
> >>   #include <linux/sizes.h>
> >>
> >> +    .macro  magic_nop
> >> +#ifdef CONFIG_EFI
> >> +.L_head:
> >> +    /*
> >> +     * This add instruction has no meaningful effect except that
> >> +     * its opcode forms the magic "MZ" signature required by UEFI.
> >> +     */
> >> +    add     x13, x18, #0x16
> >
> > It's probably faster too ;)
> >
> >> +#else
> >> +    /*
> >> +     * Bootloaders may inspect the opcode at the start of the kernel
> >> +     * image to decide if the kernel is capable of booting via UEFI.
> >> +     * So put an ordinary NOP here, not the "MZ.." pseudo-nop above.
> >> +     */
> >> +    nop
> >
> > Let's just hope nobody was decoding the branch instruction...
> >
> > Acked-by: Will Deacon >will@kernel.org>
> >
> > Will
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >

_______________________________________________
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] 14+ messages in thread

* Re: [PATCH 4/4] arm64: head: tidy up the Image header definition
  2020-10-29  7:30       ` Ard Biesheuvel
@ 2020-10-29 13:06         ` Robin Murphy
  2020-11-03  7:13           ` Ard Biesheuvel
  0 siblings, 1 reply; 14+ messages in thread
From: Robin Murphy @ 2020-10-29 13:06 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Mark Rutland, Catalin Marinas, Will Deacon, James Morse, Linux ARM

On 2020-10-29 07:30, Ard Biesheuvel wrote:
> On Wed, 28 Oct 2020 at 18:56, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-10-28 14:17, Will Deacon wrote:
>>> On Tue, Oct 27, 2020 at 08:32:09AM +0100, Ard Biesheuvel wrote:
>>>> Even though support for EFI boot remains entirely optional for arm64,
>>>> it is unlikely that we will ever be able to repurpose the image header
>>>> fields that the EFI loader relies on, i.e., the magic NOP at offset
>>>> 0x0 and the PE header address at offset 0x3c.
>>>>
>>>> So let's factor out the differences into a 'magic_nop' macro and a local
>>>> symbol representing the PE header address, and move the conditional
>>>> definitions into efi-header.S, taking into account whether CONFIG_EFI is
>>>> enabled or not.
>>>
>>> How many architectures can claim to have both a "magic nop" and a
>>> "mysterious nop", hey?
>>
>> It's fun 'n'all, but putting my serious hat on for a moment, if the name
>> still requires a comment to explain it at point of use, it's not a very
>> good name :(
>>
>> At worst magic_nop is even potentially misleading, since it's not
>> necessarily a nop; there's no mention of the implicit dependency on a
>> context where the side-effect of executing it wouldn't affect anything
>> important.
>>
>> Could we call the macro itself something clear and self-explanatory like
>> efi_signature_insn please? I'm happy for it to be *commented* as "Magic
>> NOP" if you want parity with the VDSO :D
>>
> 
> Will efi_pseudo_nop do?

Again, what's the defining significance of the instruction that this 
macro stands for - that it does nothing; that it does pseudo-nothing; or 
that it has a specific signature encoding? I know this probably sounds 
like bikeshedding to most, but I firmly believe that good, accurate 
names really do matter :)

> Also, do you think it would be better to use an opcode here that has
> no architectural side effects, such as a PRFM (literal) instruction?
> It is obviously not going to make a difference in practice, but it
> always annoyed me that the pseudo NOP is not a NOP.

Yeah, it's a shame there's no way to get a guaranteed non-taken 
conditional branch in A64, and nearly every good candidate for a 
non-destructive operation with an arbitrary immediate seems to rely on 
an rt=31 encoding... 'prfm PLIL3STRM, . + 2888' is utterly impenetrable, 
but should indeed work; 'ccmp x18, #0, #0xd, pl' is probably the least 
destructive ALU option (only a chance of changing the flags).

Robin.

>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>> ---
>>>>    arch/arm64/kernel/efi-header.S | 43 +++++++++++++++-----
>>>>    arch/arm64/kernel/head.S       | 19 +--------
>>>>    2 files changed, 35 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
>>>> index ddaf57d825b5..7b7ac4316d95 100644
>>>> --- a/arch/arm64/kernel/efi-header.S
>>>> +++ b/arch/arm64/kernel/efi-header.S
>>>> @@ -7,7 +7,27 @@
>>>>    #include <linux/pe.h>
>>>>    #include <linux/sizes.h>
>>>>
>>>> +    .macro  magic_nop
>>>> +#ifdef CONFIG_EFI
>>>> +.L_head:
>>>> +    /*
>>>> +     * This add instruction has no meaningful effect except that
>>>> +     * its opcode forms the magic "MZ" signature required by UEFI.
>>>> +     */
>>>> +    add     x13, x18, #0x16
>>>
>>> It's probably faster too ;)
>>>
>>>> +#else
>>>> +    /*
>>>> +     * Bootloaders may inspect the opcode at the start of the kernel
>>>> +     * image to decide if the kernel is capable of booting via UEFI.
>>>> +     * So put an ordinary NOP here, not the "MZ.." pseudo-nop above.
>>>> +     */
>>>> +    nop
>>>
>>> Let's just hope nobody was decoding the branch instruction...
>>>
>>> Acked-by: Will Deacon >will@kernel.org>
>>>
>>> Will
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>

_______________________________________________
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] 14+ messages in thread

* Re: [PATCH 4/4] arm64: head: tidy up the Image header definition
  2020-10-29 13:06         ` Robin Murphy
@ 2020-11-03  7:13           ` Ard Biesheuvel
  2020-11-04 11:29             ` Robin Murphy
  0 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-11-03  7:13 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Mark Rutland, Catalin Marinas, Will Deacon, James Morse, Linux ARM

On Thu, 29 Oct 2020 at 14:07, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-10-29 07:30, Ard Biesheuvel wrote:
> > On Wed, 28 Oct 2020 at 18:56, Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> On 2020-10-28 14:17, Will Deacon wrote:
> >>> On Tue, Oct 27, 2020 at 08:32:09AM +0100, Ard Biesheuvel wrote:
> >>>> Even though support for EFI boot remains entirely optional for arm64,
> >>>> it is unlikely that we will ever be able to repurpose the image header
> >>>> fields that the EFI loader relies on, i.e., the magic NOP at offset
> >>>> 0x0 and the PE header address at offset 0x3c.
> >>>>
> >>>> So let's factor out the differences into a 'magic_nop' macro and a local
> >>>> symbol representing the PE header address, and move the conditional
> >>>> definitions into efi-header.S, taking into account whether CONFIG_EFI is
> >>>> enabled or not.
> >>>
> >>> How many architectures can claim to have both a "magic nop" and a
> >>> "mysterious nop", hey?
> >>
> >> It's fun 'n'all, but putting my serious hat on for a moment, if the name
> >> still requires a comment to explain it at point of use, it's not a very
> >> good name :(
> >>
> >> At worst magic_nop is even potentially misleading, since it's not
> >> necessarily a nop; there's no mention of the implicit dependency on a
> >> context where the side-effect of executing it wouldn't affect anything
> >> important.
> >>
> >> Could we call the macro itself something clear and self-explanatory like
> >> efi_signature_insn please? I'm happy for it to be *commented* as "Magic
> >> NOP" if you want parity with the VDSO :D
> >>
> >
> > Will efi_pseudo_nop do?
>
> Again, what's the defining significance of the instruction that this
> macro stands for - that it does nothing; that it does pseudo-nothing; or
> that it has a specific signature encoding? I know this probably sounds
> like bikeshedding to most, but I firmly believe that good, accurate
> names really do matter :)
>

Fair enough. But by that reasoning, putting NOP in the name is
important, given that efi_signature_insn does not explain what the
instruction does.

So, efi_signature_nop then?

> > Also, do you think it would be better to use an opcode here that has
> > no architectural side effects, such as a PRFM (literal) instruction?
> > It is obviously not going to make a difference in practice, but it
> > always annoyed me that the pseudo NOP is not a NOP.
>
> Yeah, it's a shame there's no way to get a guaranteed non-taken
> conditional branch in A64, and nearly every good candidate for a
> non-destructive operation with an arbitrary immediate seems to rely on
> an rt=31 encoding... 'prfm PLIL3STRM, . + 2888' is utterly impenetrable,
> but should indeed work; 'ccmp x18, #0, #0xd, pl' is probably the least
> destructive ALU option (only a chance of changing the flags).
>

I think ccmp is probably a better choice, given that PRFM PLI
instructions issued with the MMU off are more likely to trigger
something unexpected.




>
> >>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>>> ---
> >>>>    arch/arm64/kernel/efi-header.S | 43 +++++++++++++++-----
> >>>>    arch/arm64/kernel/head.S       | 19 +--------
> >>>>    2 files changed, 35 insertions(+), 27 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
> >>>> index ddaf57d825b5..7b7ac4316d95 100644
> >>>> --- a/arch/arm64/kernel/efi-header.S
> >>>> +++ b/arch/arm64/kernel/efi-header.S
> >>>> @@ -7,7 +7,27 @@
> >>>>    #include <linux/pe.h>
> >>>>    #include <linux/sizes.h>
> >>>>
> >>>> +    .macro  magic_nop
> >>>> +#ifdef CONFIG_EFI
> >>>> +.L_head:
> >>>> +    /*
> >>>> +     * This add instruction has no meaningful effect except that
> >>>> +     * its opcode forms the magic "MZ" signature required by UEFI.
> >>>> +     */
> >>>> +    add     x13, x18, #0x16
> >>>
> >>> It's probably faster too ;)
> >>>
> >>>> +#else
> >>>> +    /*
> >>>> +     * Bootloaders may inspect the opcode at the start of the kernel
> >>>> +     * image to decide if the kernel is capable of booting via UEFI.
> >>>> +     * So put an ordinary NOP here, not the "MZ.." pseudo-nop above.
> >>>> +     */
> >>>> +    nop
> >>>
> >>> Let's just hope nobody was decoding the branch instruction...
> >>>
> >>> Acked-by: Will Deacon >will@kernel.org>
> >>>
> >>> Will
> >>>
> >>> _______________________________________________
> >>> linux-arm-kernel mailing list
> >>> linux-arm-kernel@lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>

_______________________________________________
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] 14+ messages in thread

* Re: [PATCH 4/4] arm64: head: tidy up the Image header definition
  2020-11-03  7:13           ` Ard Biesheuvel
@ 2020-11-04 11:29             ` Robin Murphy
  0 siblings, 0 replies; 14+ messages in thread
From: Robin Murphy @ 2020-11-04 11:29 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Mark Rutland, Catalin Marinas, Will Deacon, James Morse, Linux ARM

On 2020-11-03 07:13, Ard Biesheuvel wrote:
> On Thu, 29 Oct 2020 at 14:07, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-10-29 07:30, Ard Biesheuvel wrote:
>>> On Wed, 28 Oct 2020 at 18:56, Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> On 2020-10-28 14:17, Will Deacon wrote:
>>>>> On Tue, Oct 27, 2020 at 08:32:09AM +0100, Ard Biesheuvel wrote:
>>>>>> Even though support for EFI boot remains entirely optional for arm64,
>>>>>> it is unlikely that we will ever be able to repurpose the image header
>>>>>> fields that the EFI loader relies on, i.e., the magic NOP at offset
>>>>>> 0x0 and the PE header address at offset 0x3c.
>>>>>>
>>>>>> So let's factor out the differences into a 'magic_nop' macro and a local
>>>>>> symbol representing the PE header address, and move the conditional
>>>>>> definitions into efi-header.S, taking into account whether CONFIG_EFI is
>>>>>> enabled or not.
>>>>>
>>>>> How many architectures can claim to have both a "magic nop" and a
>>>>> "mysterious nop", hey?
>>>>
>>>> It's fun 'n'all, but putting my serious hat on for a moment, if the name
>>>> still requires a comment to explain it at point of use, it's not a very
>>>> good name :(
>>>>
>>>> At worst magic_nop is even potentially misleading, since it's not
>>>> necessarily a nop; there's no mention of the implicit dependency on a
>>>> context where the side-effect of executing it wouldn't affect anything
>>>> important.
>>>>
>>>> Could we call the macro itself something clear and self-explanatory like
>>>> efi_signature_insn please? I'm happy for it to be *commented* as "Magic
>>>> NOP" if you want parity with the VDSO :D
>>>>
>>>
>>> Will efi_pseudo_nop do?
>>
>> Again, what's the defining significance of the instruction that this
>> macro stands for - that it does nothing; that it does pseudo-nothing; or
>> that it has a specific signature encoding? I know this probably sounds
>> like bikeshedding to most, but I firmly believe that good, accurate
>> names really do matter :)
>>
> 
> Fair enough. But by that reasoning, putting NOP in the name is
> important, given that efi_signature_insn does not explain what the
> instruction does.
> 
> So, efi_signature_nop then?

Sure; I think arguing semantics beyond that point *would* be bikeshedding :)

>>> Also, do you think it would be better to use an opcode here that has
>>> no architectural side effects, such as a PRFM (literal) instruction?
>>> It is obviously not going to make a difference in practice, but it
>>> always annoyed me that the pseudo NOP is not a NOP.
>>
>> Yeah, it's a shame there's no way to get a guaranteed non-taken
>> conditional branch in A64, and nearly every good candidate for a
>> non-destructive operation with an arbitrary immediate seems to rely on
>> an rt=31 encoding... 'prfm PLIL3STRM, . + 2888' is utterly impenetrable,
>> but should indeed work; 'ccmp x18, #0, #0xd, pl' is probably the least
>> destructive ALU option (only a chance of changing the flags).
>>
> 
> I think ccmp is probably a better choice, given that PRFM PLI
> instructions issued with the MMU off are more likely to trigger
> something unexpected.

At least it happened to be PLI rather than PLD - the latter I'd 
definitely be scared of...

Robin.

>>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>>>> ---
>>>>>>     arch/arm64/kernel/efi-header.S | 43 +++++++++++++++-----
>>>>>>     arch/arm64/kernel/head.S       | 19 +--------
>>>>>>     2 files changed, 35 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
>>>>>> index ddaf57d825b5..7b7ac4316d95 100644
>>>>>> --- a/arch/arm64/kernel/efi-header.S
>>>>>> +++ b/arch/arm64/kernel/efi-header.S
>>>>>> @@ -7,7 +7,27 @@
>>>>>>     #include <linux/pe.h>
>>>>>>     #include <linux/sizes.h>
>>>>>>
>>>>>> +    .macro  magic_nop
>>>>>> +#ifdef CONFIG_EFI
>>>>>> +.L_head:
>>>>>> +    /*
>>>>>> +     * This add instruction has no meaningful effect except that
>>>>>> +     * its opcode forms the magic "MZ" signature required by UEFI.
>>>>>> +     */
>>>>>> +    add     x13, x18, #0x16
>>>>>
>>>>> It's probably faster too ;)
>>>>>
>>>>>> +#else
>>>>>> +    /*
>>>>>> +     * Bootloaders may inspect the opcode at the start of the kernel
>>>>>> +     * image to decide if the kernel is capable of booting via UEFI.
>>>>>> +     * So put an ordinary NOP here, not the "MZ.." pseudo-nop above.
>>>>>> +     */
>>>>>> +    nop
>>>>>
>>>>> Let's just hope nobody was decoding the branch instruction...
>>>>>
>>>>> Acked-by: Will Deacon >will@kernel.org>
>>>>>
>>>>> Will
>>>>>
>>>>> _______________________________________________
>>>>> linux-arm-kernel mailing list
>>>>> linux-arm-kernel@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>>

_______________________________________________
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] 14+ messages in thread

end of thread, other threads:[~2020-11-04 11:31 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-27  7:32 [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it Ard Biesheuvel
2020-10-27  7:32 ` [PATCH 1/4] arm64: efi: increase EFI PE/COFF header padding to 64 KB Ard Biesheuvel
2020-10-27  7:32 ` [PATCH 2/4] arm64: omit [_text, _stext) from permanent kernel mapping Ard Biesheuvel
2020-10-28 14:10   ` Will Deacon
2020-10-27  7:32 ` [PATCH 3/4] arm64/head: avoid symbol names pointing into first 64 KB of kernel image Ard Biesheuvel
2020-10-28 14:12   ` Will Deacon
2020-10-27  7:32 ` [PATCH 4/4] arm64: head: tidy up the Image header definition Ard Biesheuvel
2020-10-28 14:17   ` Will Deacon
2020-10-28 17:56     ` Robin Murphy
2020-10-29  7:30       ` Ard Biesheuvel
2020-10-29 13:06         ` Robin Murphy
2020-11-03  7:13           ` Ard Biesheuvel
2020-11-04 11:29             ` Robin Murphy
2020-10-28 15:12 ` [PATCH 0/4] arm64: head: pad Image header to 64 KB and unmap it Will Deacon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.