[kernel-hardening] Re: [PATCH 6/7] arm64: efi: replace open coded constants with symbolic ones

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Feb 06, 2017 at 04:24:34PM +0000, Ard Biesheuvel wrote:
> Replace open coded constants with symbolic ones throughout the
> Image and the EFI headers. Note that in two cases, this removes
> a value that the PE/COFF spec does not allow:
> - NumberOfSymbols in the PE header should be 0
> - PE/COFF executable sections (as opposed to sections in object files)
>   should not use the section alignment flags

I take it that EDK2 doesn't care about either of these, but perhaps we
should fix those in a preparatory patch, at the start of the series, to
separate fixes from rework.

That also makes it easier to binary-diff an Image after this patch, to
ensure that the mnemonicisation is correct. ;)

Otherwise, this generally looks fine.

Thanks,
Mark.

> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/efi-header.S | 37 ++++++++++----------
>  arch/arm64/kernel/head.S       |  5 +--
>  2 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
> index 35b11654ecc5..c87c23336c86 100644
> --- a/arch/arm64/kernel/efi-header.S
> +++ b/arch/arm64/kernel/efi-header.S
> @@ -7,6 +7,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/pe.h>
> +
>  	.macro	__jmp, target
>  #ifdef CONFIG_EFI
>  	/*
> @@ -33,21 +35,20 @@
>  	.long	pe_header - _head		// Offset to the PE header.
>  
>  pe_header:
> -	.ascii	"PE"
> -	.short 	0
> +	.long	PE_MAGIC
>  coff_header:
> -	.short	0xaa64					// AArch64
> -	.short	1					// nr_sections
> +	.short	IMAGE_FILE_MACHINE_ARM64		// Machine
> +	.short	1					// NumberOfSections
>  	.long	0 					// TimeDateStamp
>  	.long	0					// PointerToSymbolTable
> -	.long	1					// NumberOfSymbols
> +	.long	0					// NumberOfSymbols
>  	.short	section_table - optional_header		// SizeOfOptionalHeader
> -	.short	0x206					// Characteristics.
> -							// IMAGE_FILE_DEBUG_STRIPPED |
> -							// IMAGE_FILE_EXECUTABLE_IMAGE |
> -							// IMAGE_FILE_LINE_NUMS_STRIPPED
> +	.short	IMAGE_FILE_DEBUG_STRIPPED | \
> +		IMAGE_FILE_EXECUTABLE_IMAGE | \
> +		IMAGE_FILE_LINE_NUMS_STRIPPED		// Characteristics
> +
>  optional_header:
> -	.short	0x20b					// PE32+ format
> +	.short	PE_OPT_MAGIC_PE32PLUS			// PE32+ format
>  	.byte	0x02					// MajorLinkerVersion
>  	.byte	0x14					// MinorLinkerVersion
>  	.long	_end - efi_header_end			// SizeOfCode
> @@ -58,7 +59,7 @@ optional_header:
>  
>  extra_header_fields:
>  	.quad	0					// ImageBase
> -	.long	0x1000					// SectionAlignment
> +	.long	SZ_4K					// SectionAlignment
>  	.long	PECOFF_FILE_ALIGNMENT			// FileAlignment
>  	.short	0					// MajorOperatingSystemVersion
>  	.short	0					// MinorOperatingSystemVersion
> @@ -73,7 +74,7 @@ extra_header_fields:
>  	// Everything before the kernel image is considered part of the header
>  	.long	efi_header_end - _head			// SizeOfHeaders
>  	.long	0					// CheckSum
> -	.short	0xa					// Subsystem (EFI application)
> +	.short	IMAGE_SUBSYSTEM_EFI_APPLICATION		// Subsystem
>  	.short	0					// DllCharacteristics
>  	.quad	0					// SizeOfStackReserve
>  	.quad	0					// SizeOfStackCommit
> @@ -96,10 +97,7 @@ extra_header_fields:
>  
>  	// Section table
>  section_table:
> -	.ascii	".text"
> -	.byte	0
> -	.byte	0
> -	.byte	0        				// end of 0 padding of section name
> +	.ascii	".text\0\0\0"
>  	.long	_end - efi_header_end			// VirtualSize
>  	.long	efi_header_end - _head			// VirtualAddress
>  	.long	_edata - efi_header_end			// SizeOfRawData
> @@ -109,7 +107,10 @@ section_table:
>  	.long	0					// PointerToLineNumbers
>  	.short	0					// NumberOfRelocations
>  	.short	0					// NumberOfLineNumbers
> -	.long	0xe0500020				// Characteristics
> +	.long	IMAGE_SCN_CNT_CODE | \
> +		IMAGE_SCN_MEM_EXECUTE | \
> +		IMAGE_SCN_MEM_READ | \
> +		IMAGE_SCN_MEM_WRITE			// Characteristics
>  
>  #ifdef CONFIG_DEBUG_EFI
>  	/*
> @@ -131,7 +132,7 @@ efi_debug_table:
>  	.long	0					// TimeDateStamp
>  	.short	0					// MajorVersion
>  	.short	0					// MinorVersion
> -	.long	2					// Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW
> +	.long	IMAGE_DEBUG_TYPE_CODEVIEW		// Type
>  	.long	efi_debug_entry_size			// SizeOfData
>  	.long	0					// RVA
>  	.long	efi_debug_entry - _head			// FileOffset
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index aca9b184035a..055735ba3600 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -81,10 +81,7 @@ _head:
>  	.quad	0				// reserved
>  	.quad	0				// reserved
>  	.quad	0				// reserved
> -	.byte	0x41				// Magic number, "ARM\x64"
> -	.byte	0x52
> -	.byte	0x4d
> -	.byte	0x64
> +	.ascii	"ARM\x64"			// Magic number
>  
>  	__EFI_HEADER
>  
> -- 
> 2.7.4
>