Re: [U-Boot] [PATCH v3 2/6] x86: baytrail: secureboot: Add functions for verification of U-Boot
Hi Simon,
On Mon, 20 Nov 2017 08:40:22 -0700
Simon Glass s...@chromium.org wrote:
> Hi Anatolij,
>
> On 16 November 2017 at 18:14, Anatolij Gustschin wrote:
> > From: Markus Valentin
> >
> > 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
> > Signed-off-by: Anatolij Gustschin
> > ---
> > 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
>
> 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 00..eaf35c6e24
> > --- /dev/null
> > +++ b/arch/x86/cpu/baytrail/secure_boot.c
> > @@ -0,0 +1,117 @@
> > +/*
> > + * Copyright (C) 2017 Markus Valentin
> > + *
> > + * SPDX-License-Identifier: GPL-2.0+
> > + */
> > +
> > +#include
> > +
> > +#define SB_MANIFEST_BASE 0xfffe
>
> 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.
Yes, this is the fixed offset inside the blob and must not be changed.
Otherwise booting will fail. I'll add a comment.
...
> > +#define SB_MANIFEST_SIZE 0x400
> > +#define SB_MANIFEST_OEM_DATA_OFFSET0x58
> > +#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.
We allways used "dev" as a hint, but yes, the key name could vary. We use
only one key hash in the manifest, I'll change to check all keys in
subnodes of /signature node.
...
> > +/**
> > + * 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
OK, will fix.
...
> > + /* 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.
OK, will do in v4.
...
> > 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_suppor
Re: [U-Boot] [PATCH v3 2/6] x86: baytrail: secureboot: Add functions for verification of U-Boot
Hi Anatolij,
On 16 November 2017 at 18:14, Anatolij Gustschin wrote:
> From: Markus Valentin
>
> 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
> Signed-off-by: Anatolij Gustschin
> ---
> 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
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 00..eaf35c6e24
> --- /dev/null
> +++ b/arch/x86/cpu/baytrail/secure_boot.c
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (C) 2017 Markus Valentin
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include
> +
> +#define SB_MANIFEST_BASE 0xfffe
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_OFFSET0x58
> +#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:
> + * 0xfff to 0xfffc
> + */
> +#define U_BOOT_STAGE_START CONFIG_SYS_TEXT_BASE
> +#define U_BOOT_STAGE_SIZE 0xc
> +
> +/*
> + * 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_IDX0
> +#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
[U-Boot] [PATCH v3 2/6] x86: baytrail: secureboot: Add functions for verification of U-Boot
From: Markus Valentin
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
Signed-off-by: Anatolij Gustschin
---
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
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 00..eaf35c6e24
--- /dev/null
+++ b/arch/x86/cpu/baytrail/secure_boot.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright (C) 2017 Markus Valentin
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include
+
+#define SB_MANIFEST_BASE 0xfffe
+#define SB_MANIFEST_SIZE 0x400
+#define SB_MANIFEST_OEM_DATA_OFFSET0x58
+#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"
+#define PUB_KEY_MODULUS_SIZE 0x100
+
+/*
+ * U-Boot in RAM stage payload from reset vector to FSP Stage2:
+ * 0xfff to 0xfffc
+ */
+#define U_BOOT_STAGE_START CONFIG_SYS_TEXT_BASE
+#define U_BOOT_STAGE_SIZE 0xc
+
+/*
+ * 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_IDX0
+#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
+ */
+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;
+}
+
+bool fsp_verify_u_boot_bin(void)
+{
+ return ver
