[U-Boot] [PATCH 2/5] x86: baytrail: Add fsp-header verification for secure boot fsp

[Simon Glass]

On 11 May 2017 at 09:14, Anatolij Gustschin <agust at denx.de> wrote:
> From: Markus Valentin <mv at denx.de>
>
> 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 <mv at denx.de>
> ---
>  arch/x86/Kconfig                       | 13 ++++++++++++-
>  arch/x86/include/asm/fsp/fsp_support.h |  2 ++
>  arch/x86/lib/fsp/fsp_support.c         | 16 ++++++++++++++++
>  3 files changed, 30 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg at chromium.org>

But please see below

> 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..3a537d0 100644
> --- a/arch/x86/lib/fsp/fsp_support.c
> +++ b/arch/x86/lib/fsp/fsp_support.c
> @@ -119,6 +119,13 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
>                 /* No valid FSP info header was found */
>                 panic("Invalid FSP header");
>         }
> +#ifdef CONFIG_BAYTRAIL_SECURE_BOOT

Can you use if (IS_ENABLED(CONFIG_BAYTRAIL_SECURE_BOOT) instead of
#ifdef? It reduces the number of build paths.

> +       /* 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("SB: first & secondary FSP headers don't match");

How about s/SB/Secure Boot/?

> +#endif
>
>         config_data.common.fsp_hdr = fsp_hdr;
>         config_data.common.stack_top = stack_top;
> @@ -134,6 +141,15 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
>
>         fsp_upd = &config_data.fsp_upd;
>
> +#ifdef 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");
> +#endif
>         /* Copy default data from Flash */
>         memcpy(fsp_upd, (void *)(fsp_hdr->img_base + fsp_vpd->upd_offset),
>                sizeof(struct upd_region));
> --
> 2.7.4
>