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

2018-04-04 Thread 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 
> ---
>  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   0x4550 /* 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 0x5032RISC-V 32-bit address space
IMAGE_FILE_MACHINE_RISCV64 0x5064RISC-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 *)>OptionalHeader +
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2018-04-04 Thread Ivan Gorinov 
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 
---
 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_SIGNATURE0x5A4D /* MZ   */
 #define IMAGE_NT_SIGNATURE 0x4550 /* PE00 */
 
+#define IMAGE_FILE_MACHINE_INTEL3860x014c
 #define IMAGE_FILE_MACHINE_ARM 0x01c0
 #define IMAGE_FILE_MACHINE_THUMB   0x01c2
 #define IMAGE_FILE_MACHINE_ARMNT   0x01c4
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;
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 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 *)>OptionalHeader +
-- 
2.7.4

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot