Hi Anatolij,
On 16 November 2017 at 18:14, Anatolij Gustschin <ag...@denx.de> wrote:
> From: Markus Valentin <m...@denx.de>
>
> Introduce functions that check the integrity of U-Boot by utilising
> the hashes stored in the OEM-data block in Secure Boot Manifest.
>
> The verification functions get called in fsp_init()
>
> Signed-off-by: Markus Valentin <m...@denx.de>
> Signed-off-by: Anatolij Gustschin <ag...@denx.de>
> ---
> Changes in v3:
> - lower case in hex numbers
> - fix RAM stage payload hash calculation and add comments
> for associated macros
> - add comments explaining used stage indexes, s/*_ID/*_IDX
> - fixed two spaces in comment
> - s/devicetree/device tree
> - extend the output messages to give more hints when FIT key
> verification fails
> -
>
> 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 | 117
> +++++++++++++++++++++
> .../include/asm/arch-baytrail/fsp/fsp_configs.h | 24 +++++
> arch/x86/lib/fsp/fsp_support.c | 18 ++++
> 4 files changed, 160 insertions(+)
> create mode 100644 arch/x86/cpu/baytrail/secure_boot.c
>
Reviewed-by: Simon Glass <s...@chromium.org>
Some nits below.
> diff --git a/arch/x86/cpu/baytrail/Makefile b/arch/x86/cpu/baytrail/Makefile
> index a0216f3059..dbf9a82c39 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 0000000000..eaf35c6e24
> --- /dev/null
> +++ b/arch/x86/cpu/baytrail/secure_boot.c
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (C) 2017 Markus Valentin <m...@denx.de>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +
> +#define SB_MANIFEST_BASE 0xfffe0000
Is this something inside a binary blob, or can it be changed by the
user? I'm just wondering if it should go in Kconfig.
> +#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 FIT_KEY_NAME "dev"
This is just supposed to be a hint. I think you should probably check
all keys, since you don't really know which one was used to sign. You
could check this one first I suppose. But any signature that works
should be good enough.
> +#define PUB_KEY_MODULUS_SIZE 0x100
> +
> +/*
> + * U-Boot in RAM stage payload from reset vector to FSP Stage2:
> + * 0xfff0000 to 0xfffc0000
> + */
> +#define U_BOOT_STAGE_START CONFIG_SYS_TEXT_BASE
> +#define U_BOOT_STAGE_SIZE 0xc0000
> +
> +/*
> + * Indexes of 32-byte blocks in the OEM-data area containing sha256 hashes
> + * of RAM stage payload, FSP Stage2 and other OEM specific data. The order
> + * of RAM stage payload and FSP Stage2 blocks is fixed (idx 0 and 1),
> + * do not change it. We do not verify FSP Stage2 here, it will be verified
> + * in FSP (FSP Stage2 idx 1 is not used). So, the first index of OEM specific
> + * data block hash must start from 2.
> + */
> +#define SHA256_U_BOOT_STAGE_IDX 0
> +#define SHA256_FIT_PUB_KEY_IDX 2
> +
> +/**
> + * 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_idx: index of oem-data block with 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
@returns
> + */
> +static bool verify_oem_sha256(unsigned int hash_idx,
> + 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_idx);
> +#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("%02X", 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("%02X", value[i]);
> + printf("\n");
> +#endif
> + /* Compare the two hash values */
> + if (memcmp(hash_to_verify, value, SHA256_SUM_LEN))
> + return false;
> + return true;
I suggest returning 0 on success and an negative error on failure.
> +}
> +
> +bool fsp_verify_u_boot_bin(void)
> +{
> + return verify_oem_sha256(SHA256_U_BOOT_STAGE_IDX,
> + (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);
> + int ret;
> +
> + fit_public_key_modulus = fdt_getprop(gd->fdt_blob, offset,
> + "rsa,modulus", NULL);
> + if (!fit_public_key_modulus) {
> + puts("No FIT public key in U-Boot device tree\n");
> + return false;
> + }
> +
> + ret = verify_oem_sha256(SHA256_FIT_PUB_KEY_IDX,
> + fit_public_key_modulus,
> + PUB_KEY_MODULUS_SIZE);
> + if (!ret)
> + puts("FIT public key checksum wrong\n");\
> +
> + return ret;
> +}
> 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 1c6c2479f0..2c8c32b9bf 100644
> --- a/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h
> +++ b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h
> @@ -103,4 +103,28 @@ struct fspinit_rtbuf {
> #define SCC_MODE_ACPI 0
> #define SCC_MODE_PCI 1
>
> +#ifndef __ASSEMBLY__
> +/**
> + * fsp_verify_u_boot_bin() - U-Boot binary verification
> + *
> + * This function verifies the integrity for U-Boot, its device tree and the
> + * ucode appended or inserted to the device tree.
> + *
> + * @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 device tree 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
> +
> #endif /* __FSP_CONFIGS_H__ */
> diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c
> index d79a6e900a..c7e0a9eff4 100644
> --- a/arch/x86/lib/fsp/fsp_support.c
> +++ b/arch/x86/lib/fsp/fsp_support.c
> @@ -152,6 +152,24 @@ 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
> + */
> + printf("%s Failed to verify U-Boot and dtb\n", SB_PRFX);
> + hang();
> + }
> +
> + /*
> + * Verification of the public key happens with verification of
> + * the device tree binary (that's where it's stored), this check
> + * is not necessary, but nice to see it's integer
What does "it's integer" mean? Do you mean 'its'? Can you reword to
explain what integer you mean?
> + */
> + if (!fsp_verify_public_key())
> + printf("%s Failed to verify public key for FIT image\n",
> + SB_PRFX);
> #endif
>
> /* Copy default data from Flash */
> --
> 2.11.0
>
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
