Quantcast

[PATCH v2 1/3] x86: congatec: add secureboot enabled defconfig for conga-qeval20-qa3-e3845

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 1/3] x86: congatec: add secureboot enabled defconfig for conga-qeval20-qa3-e3845

Anatolij Gustschin
From: Markus Valentin <[hidden email]>

Signed-off-by: Markus Valentin <[hidden email]>
[agust: rebased, fixed to build with v2017.05]
Signed-off-by: Anatolij Gustschin <[hidden email]>
Reviewed-by: Simon Glass <[hidden email]>
---
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

diff --git a/configs/conga-qeval20-qa3-e3845-internal-uart-secure-boot_defconfig b/configs/conga-qeval20-qa3-e3845-internal-uart-secure-boot_defconfig
new file mode 100644
index 0000000..b78f90e
--- /dev/null
+++ b/configs/conga-qeval20-qa3-e3845-internal-uart-secure-boot_defconfig
@@ -0,0 +1,77 @@
+CONFIG_X86=y
+CONFIG_VENDOR_CONGATEC=y
+CONFIG_TARGET_CONGA_QEVAL20_QA3_E3845=y
+CONFIG_DEFAULT_DEVICE_TREE="conga-qeval20-qa3-e3845"
+CONFIG_INTERNAL_UART=y
+CONFIG_HAVE_INTEL_ME=y
+CONFIG_BAYTRAIL_SECURE_BOOT=y
+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
+CONFIG_USB=y
+CONFIG_DM_USB=y
+CONFIG_USB_STORAGE=y
+CONFIG_USB_KEYBOARD=y
+CONFIG_DM_VIDEO=y
+CONFIG_VIDEO_VESA=y
+CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
+CONFIG_FRAMEBUFFER_VESA_MODE_114=y
+CONFIG_CONSOLE_SCROLL_LINES=5
+CONFIG_USE_PRIVATE_LIBGCC=y
--
2.7.4

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 2/3] x86: baytrail: Add fsp-header verification for secure boot fsp

Anatolij Gustschin
From: Markus Valentin <[hidden email]>

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 <[hidden email]>
Signed-off-by: Anatolij Gustschin <[hidden email]>
---
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
+ bool "Enable Secure Boot on BayTrail"
+ depends on HAVE_FSP
+ default n
+ help
+  Use the SecureBoot Features of the BayTrail platform. This switch
+  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.
+
 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
 
 
 /**
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");
+ }
+
  /* Copy default data from Flash */
  memcpy(fsp_upd, (void *)(fsp_hdr->img_base + fsp_vpd->upd_offset),
        sizeof(struct upd_region));
--
2.7.4

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 3/3] x86: baytrail: secureboot: Add functions for verification of U-Boot

Anatolij Gustschin
In reply to this post by Anatolij Gustschin
From: Markus Valentin <[hidden email]>

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 <[hidden email]>
Signed-off-by: Anatolij Gustschin <[hidden email]>
---
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 <[hidden email]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+
+#define SB_MANIFEST_BASE 0xFFFE0000
+#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
+#define U_BOOT_OFFSET 0x2CA0
+
+#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
+
+#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 */
+ 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
+ * 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);
+
 #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
+ * 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");
  }
 
  /* Copy default data from Flash */
--
2.7.4

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 1/3] x86: congatec: add secureboot enabled defconfig for conga-qeval20-qa3-e3845

Bin Meng
In reply to this post by Anatolij Gustschin
Hi Anatolij,

On Tue, May 16, 2017 at 3:55 PM, Anatolij Gustschin <[hidden email]> wrote:
> From: Markus Valentin <[hidden email]>
>

Can you add a simple sentence for this commit?

> Signed-off-by: Markus Valentin <[hidden email]>
> [agust: rebased, fixed to build with v2017.05]
> Signed-off-by: Anatolij Gustschin <[hidden email]>
> Reviewed-by: Simon Glass <[hidden email]>
> ---
> 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
>
> diff --git a/configs/conga-qeval20-qa3-e3845-internal-uart-secure-boot_defconfig b/configs/conga-qeval20-qa3-e3845-internal-uart-secure-boot_defconfig
> new file mode 100644
> index 0000000..b78f90e
> --- /dev/null
> +++ b/configs/conga-qeval20-qa3-e3845-internal-uart-secure-boot_defconfig
> @@ -0,0 +1,77 @@
> +CONFIG_X86=y
> +CONFIG_VENDOR_CONGATEC=y
> +CONFIG_TARGET_CONGA_QEVAL20_QA3_E3845=y
> +CONFIG_DEFAULT_DEVICE_TREE="conga-qeval20-qa3-e3845"
> +CONFIG_INTERNAL_UART=y
> +CONFIG_HAVE_INTEL_ME=y
> +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.

> +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
> +CONFIG_USB=y
> +CONFIG_DM_USB=y
> +CONFIG_USB_STORAGE=y
> +CONFIG_USB_KEYBOARD=y
> +CONFIG_DM_VIDEO=y
> +CONFIG_VIDEO_VESA=y
> +CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
> +CONFIG_FRAMEBUFFER_VESA_MODE_114=y
> +CONFIG_CONSOLE_SCROLL_LINES=5
> +CONFIG_USE_PRIVATE_LIBGCC=y

Regards,
Bin
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 2/3] x86: baytrail: Add fsp-header verification for secure boot fsp

Bin Meng
In reply to this post by Anatolij Gustschin
Hi Anatolij,

On Tue, May 16, 2017 at 3:55 PM, Anatolij Gustschin <[hidden email]> wrote:

> From: Markus Valentin <[hidden email]>
>
> 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 <[hidden email]>
> Signed-off-by: Anatolij Gustschin <[hidden email]>
> ---
> 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?

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

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

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

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

> +       }
> +
>         /* Copy default data from Flash */
>         memcpy(fsp_upd, (void *)(fsp_hdr->img_base + fsp_vpd->upd_offset),
>                sizeof(struct upd_region));
> --

Regards,
Bin
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 3/3] x86: baytrail: secureboot: Add functions for verification of U-Boot

Bin Meng
In reply to this post by Anatolij Gustschin
Hi Anatolij,

On Tue, May 16, 2017 at 3:55 PM, Anatolij Gustschin <[hidden email]> wrote:

> From: Markus Valentin <[hidden email]>
>
> 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 <[hidden email]>
> Signed-off-by: Anatolij Gustschin <[hidden email]>
> ---
> 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 <[hidden email]>
> + *
> + * 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
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 2/3] x86: baytrail: Add fsp-header verification for secure boot fsp

Anatolij Gustschin
In reply to this post by Bin Meng
Hi Bin,

On Tue, 16 May 2017 22:39:23 +0800
Bin Meng [hidden email] wrote:

> Hi Anatolij,
>
> On Tue, May 16, 2017 at 3:55 PM, Anatolij Gustschin <[hidden email]> wrote:
> > From: Markus Valentin <[hidden email]>
> >
> > 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 <[hidden email]>
> > Signed-off-by: Anatolij Gustschin <[hidden email]>
> > ---
> > 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
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 1/3] x86: congatec: add secureboot enabled defconfig for conga-qeval20-qa3-e3845

Anatolij Gustschin
In reply to this post by Bin Meng
Hi Bin,

On Tue, 16 May 2017 22:39:17 +0800
Bin Meng [hidden email] wrote:

> Hi Anatolij,
>
> On Tue, May 16, 2017 at 3:55 PM, Anatolij Gustschin <[hidden email]> wrote:
> > From: Markus Valentin <[hidden email]>
> >  
>
> Can you add a simple sentence for this commit?

yes, will do in next version.

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

OK, will move it.

--
Anatolij
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Loading...