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

[Anatolij Gustschin]

Hi Simon,

On Mon, 20 Nov 2017 08:40:22 -0700
Simon Glass sjg at chromium.org wrote:

> Hi Anatolij,
> 
> On 16 November 2017 at 18:14, Anatolij Gustschin <agust at denx.de> wrote:
> > From: Markus Valentin <mv at 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 <mv at denx.de>
> > Signed-off-by: Anatolij Gustschin <agust at 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 <sjg at 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 <mv at 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.

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

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

This comment text was originally from Markus, so I'm not sure if my
interpretation of it is correct, but I think it means that is is nice
to see the integrity of the public key. The original code was printing
a 'success' message here if the key check was successful. I'll reword
the comment.

Thanks,
Anatolij