Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

Hi Ard,

On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
> After the EFI stub has done its business, it jumps into the kernel by
> branching to offset #0 of the loaded Image, which is where it expects
> to find the header containing a 'branch to stext' instruction.
> 
> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
> image loading:
> "A UEFI image is loaded into memory through the LoadImage() Boot
> Service. This service loads an image with a PE32+ format into memory.
> This PE32+ loader is required to load all sections of the PE32+ image
> into memory."
> 
> In other words, it is /not/ required to load parts of the image that are
> not covered by a PE/COFF section, so it may not have loaded the header
> at the expected offset, as it is not covered by any PE/COFF section.

What does this mean for handle_kernel_image? Given we might not have
_text through to _stext mapped, do we not need to take that into
account?

Also, have we seen problems on any systems yet?

Otherwise, this looks like a good fix for hte problem.

Thanks,
Mark.

> So instead, jump to 'stext' directly, which is at the base of the
> PE/COFF .text section, by supplying a symbol 'stext_offset' to
> efi-entry.o which contains the relative offset of stext into the Image.
> Also replace other open coded calculations of the same value with a
> reference to 'stext_offset'
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Changes since v2:
> - rebased onto 3.17+
> - added spec reference to commit message
> 
> Changes since v1:
> - drop :lo12: relocation against stext_offset in favor of using a literal
>   '=stext_offset' which is safer
> 
>  arch/arm64/kernel/efi-entry.S |  3 ++-
>  arch/arm64/kernel/head.S      | 10 ++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
> index 619b1dd7bcde..a0016d3a17da 100644
> --- a/arch/arm64/kernel/efi-entry.S
> +++ b/arch/arm64/kernel/efi-entry.S
> @@ -61,7 +61,8 @@ ENTRY(efi_stub_entry)
>  	 */
>  	mov	x20, x0		// DTB address
>  	ldr	x0, [sp, #16]	// relocated _text address
> -	mov	x21, x0
> +	ldr	x21, =stext_offset
> +	add	x21, x0, x21
>  
>  	/*
>  	 * Flush dcache covering current runtime addresses
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 0a6e4f924df8..8c06c9d269d2 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -132,6 +132,8 @@ efi_head:
>  #endif
>  
>  #ifdef CONFIG_EFI
> +	.globl	stext_offset
> +	.set	stext_offset, stext - efi_head
>  	.align 3
>  pe_header:
>  	.ascii	"PE"
> @@ -155,7 +157,7 @@ optional_header:
>  	.long	0				// SizeOfInitializedData
>  	.long	0				// SizeOfUninitializedData
>  	.long	efi_stub_entry - efi_head	// AddressOfEntryPoint
> -	.long	stext - efi_head		// BaseOfCode
> +	.long	stext_offset			// BaseOfCode
>  
>  extra_header_fields:
>  	.quad	0				// ImageBase
> @@ -172,7 +174,7 @@ extra_header_fields:
>  	.long	_end - efi_head			// SizeOfImage
>  
>  	// Everything before the kernel image is considered part of the header
> -	.long	stext - efi_head		// SizeOfHeaders
> +	.long	stext_offset			// SizeOfHeaders
>  	.long	0				// CheckSum
>  	.short	0xa				// Subsystem (EFI application)
>  	.short	0				// DllCharacteristics
> @@ -217,9 +219,9 @@ section_table:
>  	.byte	0
>  	.byte	0        		// end of 0 padding of section name
>  	.long	_end - stext		// VirtualSize
> -	.long	stext - efi_head	// VirtualAddress
> +	.long	stext_offset		// VirtualAddress
>  	.long	_edata - stext		// SizeOfRawData
> -	.long	stext - efi_head	// PointerToRawData
> +	.long	stext_offset		// PointerToRawData
>  
>  	.long	0		// PointerToRelocations (0 for executables)
>  	.long	0		// PointerToLineNumbers (0 for executables)
> -- 
> 1.8.3.2
> 
>