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