Hi Bin,

On Tue, 16 May 2017 22:39:23 +0800 Bin Meng bmeng.cn@gmail.com wrote:

Hi Anatolij,

On Tue, May 16, 2017 at 3:55 PM, Anatolij Gustschin agust@denx.de wrote:

...

From: Markus Valentin mv@denx.de

Introduce a new Kconfig variable for secure boot on baytrail based platforms. If this variable is set the build process tries to use fsp-sb.bin instead of fsp.bin (-sb is the secure boot enabled fsp).

Also check the two fsp headers against each other and print if secure boot is enabled or not.

Signed-off-by: Markus Valentin mv@denx.de Signed-off-by: Anatolij Gustschin agust@denx.de

---

Changes in v2:

• use if (IS_ENABLED(CONFIG_*)) instead of #ifdef
• s/SB/Secure Boot/
• minor Kconfig help cleanup

arch/x86/Kconfig | 13 ++++++++++++- arch/x86/include/asm/fsp/fsp_support.h | 2 ++ arch/x86/lib/fsp/fsp_support.c | 18 ++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 9ead3eb..8cea393 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -348,7 +348,8 @@ config HAVE_FSP config FSP_FILE string "Firmware Support Package binary filename" depends on HAVE_FSP

•   default "fsp.bin"
  

•   default "fsp.bin" if !BAYTRAIL_SECURE_BOOT
  

•   default "fsp-sb.bin" if BAYTRAIL_SECURE_BOOT
    help
      The filename of the file to use as Firmware Support Package binary
      in the board directory.
  

@@ -400,6 +401,16 @@ config FSP_BROKEN_HOB do not overwrite the important boot service data which is used by FSP, otherwise the subsequent call to fsp_notify() will fail.

+config BAYTRAIL_SECURE_BOOT

Should this be in arch/x86/cpu/baytrail/Kconfig instead?

right, I'll move it to baytrail subdir.

...

•   bool "Enable Secure Boot on BayTrail"
  

•   depends on HAVE_FSP
  

•   default n
  

•   help
  

•     Use the SecureBoot Features of the BayTrail platform. This switch  
  

nits: secure boot feature

OK.

...

•     enables the usage of the secure-boot enabled fsp.bin (fsp-sb.bin)
  

•     for your board you need to provide this yourself. You can reconfigure
  

•     your fsp with the Intel BCT tool to enable SecureBoot.  
  

nits: secure boot

OK.

...

config ENABLE_MRC_CACHE bool "Enable MRC cache" depends on !EFI && !SYS_COREBOOT diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index 61d811f..bae17bc 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -21,6 +21,8 @@ #define FSP_LOWMEM_BASE 0x100000UL #define FSP_HIGHMEM_BASE 0x100000000ULL #define UPD_TERMINATOR 0x55AA +#define FSP_FIRST_HEADER_OFFSET 0x94 +#define FSP_SECOND_HEADER_OFFSET 0x20494

Are these two offsets common to all FSP, or BayTrail-specific values?

I think that 0x204094 is BayTrail-specific. 0x94 is common, I have seen this value in all FSP integration guide files, when it is documented there.

...

diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index a480361..0bbd9ae 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -120,6 +120,14 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) panic("Invalid FSP header"); }

•   if (IS_ENABLED(CONFIG_BAYTRAIL_SECURE_BOOT)) {
  

•           /* compare primary and secondary header */
  

•           if (memcmp((void *)(CONFIG_FSP_ADDR + FSP_FIRST_HEADER_OFFSET),
  

•                      (void *)(CONFIG_FSP_ADDR + FSP_SECOND_HEADER_OFFSET),
  

•                      fsp_hdr->hdr_len))
  

•                   panic("Secure Boot: 1st & 2nd FSP headers don't match");
  

•   }
  

•   config_data.common.fsp_hdr = fsp_hdr;
    config_data.common.stack_top = stack_top;
    config_data.common.boot_mode = boot_mode;
  

@@ -134,6 +142,16 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)

    fsp_upd = &config_data.fsp_upd;

•   if (IS_ENABLED(CONFIG_BAYTRAIL_SECURE_BOOT)) {
  

•           /*
  

•            * if the enable secure boot flag is not 1, secure boot has not
  

•            * been activated in the FSP which results in the TXE-Engine not
  

•            * getting loaded
  

•            */
  

•           printf("FSP: Secure Boot %sabled\n",
  

•                  fsp_vpd->enable_secure_boot == 1 ? "en" : "dis");  
  

I believe this won't build for other FSP platforms due to no enable_secure_boot member in fsp_vpd structure.

Yes, this breaks crownbay_defconfig build. AFAIK, we do not have support for multiple platforms in a single image, so I have to use ifdef here to avoid build issues.

Thanks for review and comments!

-- Anatolij

Hi Anatolij,

On Tue, May 16, 2017 at 3:55 PM, Anatolij Gustschin agust@denx.de wrote:

From: Markus Valentin mv@denx.de

Introduce functions that check the integrity of U-Boot by utilising the hashes stored in the oem-data block.

The verification functions get called in fsp_init()

Signed-off-by: Markus Valentin mv@denx.de Signed-off-by: Anatolij Gustschin agust@denx.de

---

Changes in v2:

• use 'const void *' for fdt property ptr, drop cast
• s/u-boot/U-Boot/
• fix function comment style and move comments to header with prototypes. Use fsp_ prefix
• fix multiline comment style
• s/SB:/Secure Boot/ for non-debug messages

arch/x86/cpu/baytrail/Makefile | 1 + arch/x86/cpu/baytrail/secure_boot.c | 105 +++++++++++++++++++++ .../include/asm/arch-baytrail/fsp/fsp_configs.h | 22 +++++ arch/x86/lib/fsp/fsp_support.c | 17 ++++ 4 files changed, 145 insertions(+) create mode 100644 arch/x86/cpu/baytrail/secure_boot.c

diff --git a/arch/x86/cpu/baytrail/Makefile b/arch/x86/cpu/baytrail/Makefile index a0216f3..dbf9a82 100644 --- a/arch/x86/cpu/baytrail/Makefile +++ b/arch/x86/cpu/baytrail/Makefile @@ -8,4 +8,5 @@ obj-y += cpu.o obj-y += early_uart.o obj-y += fsp_configs.o obj-y += valleyview.o +obj-$(CONFIG_BAYTRAIL_SECURE_BOOT) += secure_boot.o obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi.o diff --git a/arch/x86/cpu/baytrail/secure_boot.c b/arch/x86/cpu/baytrail/secure_boot.c new file mode 100644 index 0000000..97acea8 --- /dev/null +++ b/arch/x86/cpu/baytrail/secure_boot.c @@ -0,0 +1,105 @@ +/*

• • Copyright (C) 2017 Markus Valentin mv@denx.de
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h>

+#define SB_MANIFEST_BASE 0xFFFE0000

nits: please use lower case hex and fix this globally in this file

+#define SB_MANIFEST_SIZE 0x400 +#define SB_MANIFEST_OEM_DATA_OFFSET 0x58 +#define SB_MANIFEST_OEM_HASH_OFFSET (SB_MANIFEST_OEM_DATA_OFFSET + 4) +#define SB_MANIFEST_OEM_HASH_BASE (SB_MANIFEST_BASE +\

•                                    SB_MANIFEST_OEM_HASH_OFFSET)
  

+#define SB_MANIFEST_END (SB_MANIFEST_BASE + SB_MANIFEST_SIZE)

+#define PUB_KEY_MODULUS_SIZE 0x100 +#define U_BOOT_STAGE_SIZE 0xDD360

What is this?

+#define U_BOOT_OFFSET 0x2CA0

and this?

+#define U_BOOT_STAGE_START (CONFIG_SYS_TEXT_BASE + U_BOOT_OFFSET) +#define U_BOOT_STAGE_END (U_BOOT_STAGE_START + U_BOOT_STAGE_SIZE)

+#define SHA256_U_BOOT_STAGE_ID 0 +#define SHA256_FSP_STAGE2_ID 1 +#define SHA256_FIT_PUB_KEY_ID 2

I believe a comment block is needed for explaining things like number 0/1/2, at least where are they coming?

+#define FIT_KEY_NAME "dev"

+/**

• • verify_oem_sha256() - oem data block verification
• • This function compares a hash which gets retrieved from the oem data block
• • with the runtime calculated hash of start_address+size. If they match,
• • this function returns true. If not, it returns false.
• • @hash_id: offset of oem-data block for hash to compare
• • @start_address: address where the hash calculation should start
• • @size: length of the region for hash calculation
• • @retval: true on success, false on error
• */

+static bool verify_oem_sha256(unsigned int hash_id,

•                         const void *start_address,
  

•                         size_t size)
  

+{

•   uint8_t value[SHA256_SUM_LEN];
  

•   int value_len;
  

•   /* calculate address of hash to compare in the oemdata block*/
  

•   void *hash_to_verify = (void *)SB_MANIFEST_OEM_HASH_BASE +
  

•                          (SHA256_SUM_LEN * hash_id);
  

+#ifdef DEBUG

•   unsigned int i = 0;
  

•   uint8_t oem_value[SHA256_SUM_LEN];
  

•   memcpy(oem_value, hash_to_verify, SHA256_SUM_LEN);
  

•   printf("SB: Hash to verify:\t");
  

•   for (i = 0; i < SHA256_SUM_LEN; i++)
  

•           printf("%X", oem_value[i]);
  

•   printf("\n");
  

+#endif

•   /* calculate the hash of the binary */
  

•   calculate_hash(start_address, size, "sha256", value, &value_len);
  

+#ifdef DEBUG

•   printf("SB: calculated hash:\t");
  

•   for (i = 0; i < SHA256_SUM_LEN; i++)
  

•           printf("%X", value[i]);
  

•   printf("\n");
  

+#endif

•   /* compare the two hash  values */
  

nits: two spaces after 'hash'

•   if (memcmp(hash_to_verify, value, SHA256_SUM_LEN))
  

•           return false;
  

•   return true;
  

+}

+bool fsp_verify_u_boot_bin(void) +{

•   return verify_oem_sha256(SHA256_U_BOOT_STAGE_ID,
  

•                            (const void *)U_BOOT_STAGE_START,
  

•                            U_BOOT_STAGE_SIZE);
  

+}

+bool fsp_verify_public_key(void) +{

•   const void *fit_public_key_modulus;
  

•   int offset = fdt_node_offset_by_prop_value(gd->fdt_blob, -1,
  

•                                              "key-name-hint",
  

•                                              FIT_KEY_NAME,
  

•                                              4);
  

•   fit_public_key_modulus =  fdt_getprop(gd->fdt_blob, offset,
  

•                                         "rsa,modulus", NULL);
  

•   if (!fit_public_key_modulus) {
  

•           debug("SB: Could not fetch public key from U-Boot Devicetree\n");
  

•           return false;
  

•   }
  

•   return verify_oem_sha256(SHA256_FIT_PUB_KEY_ID,
  

•                            fit_public_key_modulus,
  

•                            PUB_KEY_MODULUS_SIZE);
  

+} diff --git a/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h index e539890..a07853c 100644 --- a/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h +++ b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h @@ -16,4 +16,26 @@ struct fspinit_rtbuf { struct common_buf common; /* FSP common runtime data structure */ };

+/**

• • fsp_verify_u_boot_bin() - U-Boot binary verification
• • This function verifies the integrity for U-Boot, its devicetree and the ucode
• • appended or inserted to the devicetree.
• • @retval: true on success, false on error
• */

+bool fsp_verify_u_boot_bin(void);

+/**

• • fsp_verify_public_key() - integrity of public key for fit image verification
• • This function verifies the integrity for the modulus of the public key which
• • is stored in the U-Boot devicetree for fit image verification. It tries to

nits: device tree

• • find the "rsa,modulus" property in the dtb and then verifies it with the
• • checksum stored in the oem-data block
• • @retval: true on success, false on error
• */

+bool fsp_verify_public_key(void);

Again, are these two APIs common for all FSP based platforms?

#endif /* __FSP_CONFIGS_H__ */ diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index 0bbd9ae..5e3ce13 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -150,6 +150,23 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) */ printf("FSP: Secure Boot %sabled\n", fsp_vpd->enable_secure_boot == 1 ? "en" : "dis");

•           if (!fsp_verify_u_boot_bin()) {
  

•                   /*
  

•                    * If our U-Boot binary checksum isn't equal to
  

•                    * our expected checksum we need to stop booting
  

•                    */
  

•                   puts("Secure Boot: Failed to verify U-Boot and dtb\n");
  

•                   hang();
  

•           }
  

•           /*
  

•            * Verification of the public key happens with verification of
  

•            * the devicetree binary (that's where it's stored), this check
  

nits: device tree

•            * is not necessary, but nice to see it's integer
  

•            */
  

•           if (!fsp_verify_public_key())
  

•                   puts("Secure Boot: Failed to verify public key for fit-image\n");
  

As the comments say it's not necessary, which means it will never fail, right? But in case it fails, that means either hardware is doing wrong, or software is doing wrong? Can we adjust the output message to indicate some hints?

    }

    /* Copy default data from Flash */

--

BTW: I don't see device tree update for the secure boot enabled board, like a device tree that has the "rsa,modulus" stuff.

Regards, Bin