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.gori...@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 +
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
