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

[Anatolij Gustschin]

Hi Bin,

On Tue, 16 May 2017 22:39:23 +0800
Bin Meng bmeng.cn at gmail.com wrote:

> Hi Anatolij,
> 
> On Tue, May 16, 2017 at 3:55 PM, 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>
> > Signed-off-by: Anatolij Gustschin <agust at denx.de>
> > ---
> > Changes in v2:
> >  - use if (IS_ENABLED(CONFIG_*)) instead of #ifdef
> >  - s/SB/Secure Boot/
> >  - minor Kconfig help cleanup
> >
> >  arch/x86/Kconfig                       | 13 ++++++++++++-
> >  arch/x86/include/asm/fsp/fsp_support.h |  2 ++
> >  arch/x86/lib/fsp/fsp_support.c         | 18 ++++++++++++++++++
> >  3 files changed, 32 insertions(+), 1 deletion(-)
> >
> > 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  
> 
> Should this be in arch/x86/cpu/baytrail/Kconfig instead?

right, I'll move it to baytrail subdir.

> 
> > +       bool "Enable Secure Boot on BayTrail"
> > +       depends on HAVE_FSP
> > +       default n
> > +       help
> > +         Use the SecureBoot Features of the BayTrail platform. This switch  
> 
> nits: secure boot feature

OK.

> > +         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.  
> 
> nits: secure boot

OK.

> >  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  
> 
> Are these two offsets common to all FSP, or BayTrail-specific values?

I think that 0x204094 is BayTrail-specific. 0x94 is common, I have
seen this value in all FSP integration guide files, when it is
documented there.

> > diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c
> > index a480361..0bbd9ae 100644
> > --- a/arch/x86/lib/fsp/fsp_support.c
> > +++ b/arch/x86/lib/fsp/fsp_support.c
> > @@ -120,6 +120,14 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
> >                 panic("Invalid FSP header");
> >         }
> >
> > +       if (IS_ENABLED(CONFIG_BAYTRAIL_SECURE_BOOT)) {
> > +               /* 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("Secure Boot: 1st & 2nd FSP headers don't match");
> > +       }
> > +
> >         config_data.common.fsp_hdr = fsp_hdr;
> >         config_data.common.stack_top = stack_top;
> >         config_data.common.boot_mode = boot_mode;
> > @@ -134,6 +142,16 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
> >
> >         fsp_upd = &config_data.fsp_upd;
> >
> > +       if (IS_ENABLED(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");  
> 
> I believe this won't build for other FSP platforms due to no
> enable_secure_boot member in fsp_vpd structure.

Yes, this breaks crownbay_defconfig build. AFAIK, we do not have support
for multiple platforms in a single image, so I have to use ifdef here to
avoid build issues.

Thanks for review and comments!

--
Anatolij