[U-Boot] [PATCH] efi_loader: Check machine type in the image header

[Heinrich Schuchardt]

On 04/04/2018 11:59 PM, Ivan Gorinov wrote:
> Check FileHeader.Machine to make sure the EFI executable image is built
> for the same architecture. After this change, 32-bit U-Boot on x86 will
> print an error message instead of loading an x86_64 image and crashing.
> 
> Signed-off-by: Ivan Gorinov <ivan.gorinov at intel.com>
> ---
>  include/pe.h                      |  1 +
>  lib/efi_loader/efi_image_loader.c | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/pe.h b/include/pe.h
> index c3a19ce..2435069 100644
> --- a/include/pe.h
> +++ b/include/pe.h
> @@ -38,6 +38,7 @@ typedef struct _IMAGE_DOS_HEADER {
>  #define IMAGE_DOS_SIGNATURE		0x5A4D     /* MZ   */
>  #define IMAGE_NT_SIGNATURE		0x00004550 /* PE00 */
>  
> +#define IMAGE_FILE_MACHINE_INTEL386	0x014c

Please, use the names from the "Microsoft Portable Executable and Common
Object File Format Specification":

IMAGE_FILE_MACHINE_I386 0x14c
// Intel 386 or later processors and compatible processors

>  #define IMAGE_FILE_MACHINE_ARM		0x01c0
>  #define IMAGE_FILE_MACHINE_THUMB	0x01c2
>  #define IMAGE_FILE_MACHINE_ARMNT	0x01c4

Please, also add

IMAGE_FILE_MACHINE_RISCV32 0x5032    RISC-V 32-bit address space
IMAGE_FILE_MACHINE_RISCV64 0x5064    RISC-V 64-bit address space

> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 74c6a9f..9b4db62 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -126,15 +126,23 @@ void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info)
>  	void *entry;
>  	uint64_t image_size;
>  	unsigned long virt_size = 0;
> +	int machine_type = 0;

Why do you need a variable for the check?
Isn't this more concise:

	switch (nt->FileHeader.Machine) {
#ifdef CONFIG_ARM64
	case IMAGE_FILE_MACHINE_ARM64:
		break;
#elif defined(CONFIG_ARM)
	case IMAGE_FILE_MACHINE_THUMB:
		break;
#elif defined(CONGIG_X86_64)
	case IMAGE_FILE_MACHINE_AMD64:
		break;
#elif defined(CONFIG_X86)
	case IMAGE_FILE_MACHINE_I386:
		break;
#elif defined(CONFIG_CPU_RISCV_64)
	case IMAGE_FILE_MACHINE_RISCV64:
		break;
#elif defined(CONFIG_CPU_RISCV_32)
	case IMAGE_FILE_MACHINE_RISCV32:
		break;
#endif
	default:
		printf("%s: Image type 0x%04x not supported\n", \
		       __func__, nt->FileHeader.Machine);
		return NULL;
	}

>  	bool can_run_nt64 = true;
>  	bool can_run_nt32 = true;
>  
>  #if defined(CONFIG_ARM64)
> +	machine_type = IMAGE_FILE_MACHINE_ARM64;
>  	can_run_nt32 = false;
>  #elif defined(CONFIG_ARM)
>  	can_run_nt64 = false;
>  #endif

If we correctly check the machine type, can't we eliminate can_run_nt64
and can_run_nt32? If you would leave the variables you would have to set
them for all architectures.

Below we check the optional header type against the bitness.
The PE spec does not forbid to use PE32+ for 32 bit images.

Best regards

Heinrich

>  
> +#if defined(CONFIG_X86_64)
> +	machine_type = IMAGE_FILE_MACHINE_AMD64;
> +#elif defined(CONFIG_X86)
> +	machine_type = IMAGE_FILE_MACHINE_INTEL386;
> +#endif
> +
>  	dos = efi;
>  	if (dos->e_magic != IMAGE_DOS_SIGNATURE) {
>  		printf("%s: Invalid DOS Signature\n", __func__);
> @@ -147,6 +155,11 @@ void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info)
>  		return NULL;
>  	}
>  
> +	if (machine_type && nt->FileHeader.Machine != machine_type) {
> +		printf("%s: Incorrect machine type\n", __func__);
> +		return NULL;
> +	}
> +
>  	/* Calculate upper virtual address boundary */
>  	num_sections = nt->FileHeader.NumberOfSections;
>  	sections = (void *)&nt->OptionalHeader +
>