x86: congatec: add secureboot enabled defconfig for conga-qeval20-qa3-e3845

From: Markus Valentin <[email protected]>

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 <[email protected]>

Signed-off-by Anatolij Gustschin <[email protected]>

---
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(-)

arch/x86/Kconfig +2 -1

index 9ead3eb..8cea393 100644
@@ -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.

arch/x86/Kconfig +1 -0

+
 config ENABLE_MRC_CACHE
        bool "Enable MRC cache"
        depends on !EFI && !SYS_COREBOOT

arch/x86/include/asm/fsp/fsp_support.h +0 -0

index 61d811f..bae17bc 100644

arch/x86/include/asm/fsp/fsp_support.h +0 -0

 /**

arch/x86/lib/fsp/fsp_support.c +8 -0

index a480361..0bbd9ae 100644
@@ -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)
 

Hi Anatolij,

Quoted 40 lines · Anatolij Gustschin

From: Markus Valentin <[email protected]>

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 <[email protected]>

Signed-off-by Anatolij Gustschin <[email protected]>

---
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(-)

arch/x86/Kconfig +2 -1

index 9ead3eb..8cea393 100644
@@ -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.
 

arch/x86/Kconfig config FSP_BROKEN_HOB · line 404

Quoted 1 line

arch/x86/Kconfig +1 -0

+config BAYTRAIL_SECURE_BOOT  

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

arch/x86/Kconfig config FSP_BROKEN_HOB · line 409

Quoted 6 lines

arch/x86/Kconfig +5 -0

+       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

arch/x86/Kconfig config FSP_BROKEN_HOB · line 412

Quoted 3 lines

arch/x86/Kconfig +3 -0

+         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

Quoted 7 lines

arch/x86/Kconfig +0 -0

 config ENABLE_MRC_CACHE
        bool "Enable MRC cache"
        depends on !EFI && !SYS_COREBOOT

arch/x86/include/asm/fsp/fsp_support.h +0 -0

index 61d811f..bae17bc 100644

arch/x86/include/asm/fsp/fsp_support.h line 25

Quoted 6 lines

arch/x86/include/asm/fsp/fsp_support.h +2 -0

@@ -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?

Quoted 23 lines

arch/x86/lib/fsp/fsp_support.c +8 -0

index a480361..0bbd9ae 100644
@@ -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;
 

arch/x86/lib/fsp/fsp_support.c void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) · line 152

Quoted 8 lines

arch/x86/lib/fsp/fsp_support.c +8 -0

+       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.

From: Markus Valentin <[email protected]>

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 <[email protected]>

Signed-off-by Anatolij Gustschin <[email protected]>

---
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

arch/x86/cpu/baytrail/Makefile +1 -0

index a0216f3..dbf9a82 100644
@@ -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

arch/x86/cpu/baytrail/secure_boot.c new file +0 -0

new file mode 100644
index 0000000..97acea8

arch/x86/cpu/baytrail/secure_boot.c new file +36 -0

+#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
+

arch/x86/cpu/baytrail/secure_boot.c new file +32 -0

+       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);
+}

arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h +5 -0

index e539890..a07853c 100644
@@ -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.

arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h +1 -0

+
 #endif /* __FSP_CONFIGS_H__ */

arch/x86/lib/fsp/fsp_support.c +3 -0

index 0bbd9ae..5e3ce13 100644
@@ -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()) {
+                       /*

Quoted 1 line

+#define SB_MANIFEST_BASE 0xFFFE0000

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

arch/x86/cpu/baytrail/secure_boot.c line 18

Quoted 2 lines

arch/x86/cpu/baytrail/secure_boot.c new file +2 -0

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

What is this?

Quoted 1 line

+#define U_BOOT_OFFSET 0x2CA0

and this?

arch/x86/cpu/baytrail/secure_boot.c line 27

Quoted 7 lines

arch/x86/cpu/baytrail/secure_boot.c new file +7 -0

+#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?

Quoted 1 line

+ /* compare the two hash values */

nits: two spaces after 'hash'

Quoted 9 lines

arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h +9 -0

+/**
+ * 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);

arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h struct fspinit_rtbuf { · line 33

Quoted 6 lines

arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h +6 -0

+
+/**
+ * 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

Quoted 6 lines

+ * 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?

arch/x86/lib/fsp/fsp_support.c void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) · line 165

Quoted 3 lines

arch/x86/lib/fsp/fsp_support.c +3 -0

+               /*
+                * Verification of the public key happens with verification of
+                * the devicetree binary (that's where it's stored), this check  

nits: device tree

arch/x86/lib/fsp/fsp_support.c void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) · line 169

Quoted 4 lines

arch/x86/lib/fsp/fsp_support.c +4 -0

+                * 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?

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

From: Markus Valentin <[email protected]>

Signed-off-by Markus Valentin <[email protected]>

[agust: rebased, fixed to build with v2017.05]

Signed-off-by Anatolij Gustschin <[email protected]>

Reviewed-by Simon Glass <[email protected]>

---
Changes in v2:
- add Reviewed-by tag

 ...0-qa3-e3845-internal-uart-secure-boot_defconfig | 77 ++++++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 configs/conga-qeval20-qa3-e3845-internal-uart-secure-boot_defconfig

configs/conga-qeval20-qa3-e3845-internal-uart-secure-boot_defconfig new file +0 -0

new file mode 100644
index 0000000..b78f90e

configs/conga-qeval20-qa3-e3845-internal-uart-secure-boot_defconfig new file +60 -0

+CONFIG_ENABLE_MRC_CACHE=y
+CONFIG_SMP=y
+CONFIG_HAVE_VGA_BIOS=y
+CONFIG_VGA_BIOS_ADDR=0xfffa0000
+CONFIG_GENERATE_PIRQ_TABLE=y
+CONFIG_GENERATE_MP_TABLE=y
+CONFIG_GENERATE_ACPI_TABLE=y
+CONFIG_SEABIOS=y
+CONFIG_FIT=y
+CONFIG_FIT_SIGNATURE=y
+CONFIG_BOOTSTAGE=y
+CONFIG_BOOTSTAGE_REPORT=y
+CONFIG_SYS_CONSOLE_INFO_QUIET=y
+CONFIG_HUSH_PARSER=y
+CONFIG_CMD_CPU=y
+# CONFIG_CMD_IMLS is not set
+# CONFIG_CMD_FLASH is not set
+CONFIG_CMD_MMC=y
+CONFIG_CMD_SF=y
+CONFIG_CMD_SPI=y
+CONFIG_CMD_I2C=y
+CONFIG_CMD_USB=y
+CONFIG_CMD_GPIO=y
+# CONFIG_CMD_SETEXPR is not set
+CONFIG_CMD_DHCP=y
+# CONFIG_CMD_NFS is not set
+CONFIG_CMD_PING=y
+CONFIG_CMD_TIME=y
+CONFIG_CMD_BOOTSTAGE=y
+CONFIG_CMD_EXT2=y
+CONFIG_CMD_EXT4=y
+CONFIG_CMD_EXT4_WRITE=y
+CONFIG_CMD_FAT=y
+CONFIG_CMD_FS_GENERIC=y
+CONFIG_OF_CONTROL=y
+CONFIG_REGMAP=y
+CONFIG_SYSCON=y
+CONFIG_CPU=y
+CONFIG_DM_I2C=y
+CONFIG_SYS_I2C_INTEL=y
+CONFIG_WINBOND_W83627=y
+CONFIG_MMC=y
+CONFIG_MMC_PCI=y
+CONFIG_MMC_SDHCI=y
+CONFIG_MMC_SDHCI_SDMA=y
+CONFIG_SPI_FLASH=y
+CONFIG_SPI_FLASH_GIGADEVICE=y
+CONFIG_SPI_FLASH_MACRONIX=y
+CONFIG_SPI_FLASH_STMICRO=y
+CONFIG_SPI_FLASH_WINBOND=y
+CONFIG_DM_ETH=y
+CONFIG_E1000=y
+CONFIG_DM_PCI=y
+CONFIG_DM_RTC=y
+CONFIG_DEBUG_UART=y
+CONFIG_DEBUG_UART_BASE=0x3f8
+CONFIG_DEBUG_UART_CLOCK=1843200
+CONFIG_SYS_NS16550=y
+CONFIG_ICH_SPI=y
+CONFIG_TIMER=y

Hi Anatolij,

Quoted 2 lines · Anatolij Gustschin

From: Markus Valentin <[email protected]>

Can you add a simple sentence for this commit?

Quoted 1 line

+CONFIG_BAYTRAIL_SECURE_BOOT=y

This commit should come in later in this patch series, as CONFIG_BAYTRAIL_SECURE_BOOT is not available yet at this point.