Hi Andreas, Simon, > -----Original Message----- > From: s...@google.com [mailto:s...@google.com] On Behalf Of Simon Glass > Sent: Friday, June 10, 2016 6:05 AM > To: Andreas Dannenberg <dannenb...@ti.com> > Cc: Sumit Garg <sumit.g...@nxp.com>; U-Boot Mailing List <u- > b...@lists.denx.de>; york sun <york....@nxp.com>; Ruchika Gupta > <ruchika.gu...@nxp.com>; Prabhakar Kushwaha > <prabhakar.kushw...@nxp.com>; Teddy Reed V <teddy.r...@gmail.com>; > Aneesh Bansal <aneesh.ban...@nxp.com> > Subject: Re: [PATCH v4 1/2] powerpc/mpc85xx: SECURE BOOT- Enable chain of > trust in SPL > > Hi, > > On 9 June 2016 at 09:05, Andreas Dannenberg <dannenb...@ti.com> wrote: > > Hi Sumit, > > > > On Thu, Jun 02, 2016 at 08:16:37AM -0400, Sumit Garg wrote: > >> As part of Chain of Trust for Secure boot, the SPL U-Boot will > >> validate the next level U-boot image. Add a new function > >> spl_validate_uboot to perform the validation. > > > > I noticed your patch series just now and I certainly don't want to > > derail what you are trying to do here but I wanted to see what you > > think about something that's pretty much related. > > > > In order to be more flexible in how we go from SPL to U-Boot (we have > > a need to inject ROM-API calls for decryption in addition to > > authentication) I've been experimenting with a method that basically > > injects a generic post-process function call into > > spl_load_simple_fit() that the platform-specific layer can > > provide/override. This function will basically get invoked on the > > U-Boot firmware itself as well as for the selected DTB after it gets > > loaded, allowing to do things like custom authentication and/or > > decryption through plugging in for example our ROM API calls. But this > > should also be a starting point for someone to plug in the U-Boot RSA > > libraries directly in a way that doesn't require using CONFIG_SPL_DM (for > memory-constrained devices).
It’s a good idea to plug ROM API calls for decryption and authentication in U-boot firmware. I think your main concern is DM framework in SPL for memory constrained devices. But for our platforms memory constraint is not a barrier in this case, so for time being we could use CONFIG_SPL_DM. So if you come up with framework to add U-Boot RSA libraries without using DM in SPL, we could enable it on our platforms too. > > > > So mostly I wanted to see if this is something that could be useful > > for you as well, and any comments/concerns you may have on that. > > > > Thanks and Regards, > > > > -- > > Andreas Dannenberg > > Texas Instruments Inc > > > > > >> Enable hardware crypto operations in SPL using SEC block. > >> In case of Secure Boot, PAMU is not bypassed. For allowing SEC block > >> access to CPC configured as SRAM, configure PAMU. > >> > >> Reviewed-by: Ruchika Gupta <ruchika.gu...@nxp.com> > >> Signed-off-by: Aneesh Bansal <aneesh.ban...@nxp.com> > >> Signed-off-by: Sumit Garg <sumit.g...@nxp.com> > >> --- > >> Changes in v2: > >> Patches rebased > >> > >> Changes in v3: > >> Patches rebased > >> > >> Changes in v4: > >> Generic changes in lib, drivers, common Makefiles removed from this > >> patchset. Rebased this patchset on top of patch [1], so this patchset > >> is dependent on patch [1]. > >> > >> [1]https://patchwork.ozlabs.org/patch/627664/ > >> > >> arch/powerpc/cpu/mpc8xxx/fsl_pamu.c | 8 +++++ > >> arch/powerpc/cpu/mpc8xxx/pamu_table.c | 8 +++++ > >> arch/powerpc/include/asm/fsl_secure_boot.h | 28 ++++++++++++++++ > >> board/freescale/common/fsl_chain_of_trust.c | 50 > +++++++++++++++++++++++++++++ > >> drivers/crypto/fsl/jr.c | 16 +++++++++ > >> drivers/mtd/nand/fsl_ifc_spl.c | 24 ++++++++++++++ > >> include/fsl_validate.h | 1 + > >> 7 files changed, 135 insertions(+) > >> > >> diff --git a/arch/powerpc/cpu/mpc8xxx/fsl_pamu.c > >> b/arch/powerpc/cpu/mpc8xxx/fsl_pamu.c > >> index 9421f1e..ede8e66 100644 > >> --- a/arch/powerpc/cpu/mpc8xxx/fsl_pamu.c > >> +++ b/arch/powerpc/cpu/mpc8xxx/fsl_pamu.c > >> @@ -239,15 +239,23 @@ int pamu_init(void) > >> spaact_size = sizeof(struct paace) * NUM_SPAACT_ENTRIES; > >> > >> /* Allocate space for Primary PAACT Table */ > >> +#if (defined(CONFIG_SPL_BUILD) && > defined(CONFIG_SPL_PPAACT_ADDR)) > >> + ppaact = (void *)CONFIG_SPL_PPAACT_ADDR; #else > >> ppaact = memalign(PAMU_TABLE_ALIGNMENT, ppaact_size); > >> if (!ppaact) > >> return -1; > >> +#endif > >> memset(ppaact, 0, ppaact_size); > >> > >> /* Allocate space for Secondary PAACT Table */ > >> +#if (defined(CONFIG_SPL_BUILD) && > defined(CONFIG_SPL_SPAACT_ADDR)) > >> + sec = (void *)CONFIG_SPL_SPAACT_ADDR; #else > >> sec = memalign(PAMU_TABLE_ALIGNMENT, spaact_size); > >> if (!sec) > >> return -1; > >> +#endif > >> memset(sec, 0, spaact_size); > >> > >> ppaact_phys = virt_to_phys((void *)ppaact); diff --git > >> a/arch/powerpc/cpu/mpc8xxx/pamu_table.c > >> b/arch/powerpc/cpu/mpc8xxx/pamu_table.c > >> index 26c5ea4..a8e6f51 100644 > >> --- a/arch/powerpc/cpu/mpc8xxx/pamu_table.c > >> +++ b/arch/powerpc/cpu/mpc8xxx/pamu_table.c > >> @@ -28,6 +28,14 @@ void construct_pamu_addr_table(struct > >> pamu_addr_tbl *tbl, int *num_entries) > >> > >> i++; > >> #endif > >> +#if (defined(CONFIG_SPL_BUILD) && (CONFIG_SYS_INIT_L3_VADDR)) > >> + tbl->start_addr[i] = > >> + (uint64_t)virt_to_phys((void *)CONFIG_SYS_INIT_L3_VADDR); > >> + tbl->size[i] = 256 * 1024; /* 256K CPC flash */ > >> + tbl->end_addr[i] = tbl->start_addr[i] + tbl->size[i] - 1; > >> + > >> + i++; > >> +#endif > >> debug("PAMU address\t\t\tsize\n"); > >> for (j = 0; j < i ; j++) > >> debug("%llx \t\t\t%llx\n", tbl->start_addr[j], > >> tbl->size[j]); diff --git > >> a/arch/powerpc/include/asm/fsl_secure_boot.h > >> b/arch/powerpc/include/asm/fsl_secure_boot.h > >> index 826f9c9..99eec7f 100644 > >> --- a/arch/powerpc/include/asm/fsl_secure_boot.h > >> +++ b/arch/powerpc/include/asm/fsl_secure_boot.h > >> @@ -72,6 +72,32 @@ > >> > >> #ifdef CONFIG_CHAIN_OF_TRUST > >> > >> +#ifdef CONFIG_SPL_BUILD > >> +#define CONFIG_SPL_DM 1 > >> +#define CONFIG_SPL_CRYPTO_SUPPORT > >> +#define CONFIG_SPL_HASH_SUPPORT > >> +#define CONFIG_SPL_RSA > >> +#define CONFIG_SPL_DRIVERS_MISC_SUPPORT > >> +/* > >> + * PPAACT and SPAACT table for PAMU must be placed on DDR after DDR > >> +init > >> + * due to space crunch on CPC and thus malloc will not work. > >> + */ > >> +#define CONFIG_SPL_PPAACT_ADDR 0x2e000000 > >> +#define CONFIG_SPL_SPAACT_ADDR 0x2f000000 > >> +#define CONFIG_SPL_JR0_LIODN_S 454 > >> +#define CONFIG_SPL_JR0_LIODN_NS 458 > >> +/* > >> + * Define the key hash for U-Boot here if public/private key pair > >> +used to > >> + * sign U-boot are different from the SRK hash put in the fuse > >> + * Example of defining KEY_HASH is > >> + * #define CONFIG_SPL_UBOOT_KEY_HASH \ > >> + * > "41066b564c6ffcef40ccbc1e0a5d0d519604000c785d97bbefd25e4d288d1c8b" > >> + * else leave it defined as NULL > >> + */ > >> + > >> +#define CONFIG_SPL_UBOOT_KEY_HASH NULL > >> +#endif /* ifdef CONFIG_SPL_BUILD */ > >> + > >> #define CONFIG_CMD_ESBC_VALIDATE > >> #define CONFIG_CMD_BLOB > >> #define CONFIG_FSL_SEC_MON > >> @@ -82,6 +108,7 @@ > >> #define CONFIG_FSL_CAAM > >> #endif > >> > >> +#ifndef CONFIG_SPL_BUILD > >> /* fsl_setenv_chain_of_trust() must be called from > >> * board_late_init() > >> */ > >> @@ -119,5 +146,6 @@ > >> #endif /* #ifdef CONFIG_BOOTSCRIPT_COPY_RAM */ > >> > >> #include <config_fsl_chain_trust.h> > >> +#endif /* #ifndef CONFIG_SPL_BUILD */ > >> #endif /* #ifdef CONFIG_CHAIN_OF_TRUST */ #endif diff --git > >> a/board/freescale/common/fsl_chain_of_trust.c > >> b/board/freescale/common/fsl_chain_of_trust.c > >> index ecfcc82..992babf 100644 > >> --- a/board/freescale/common/fsl_chain_of_trust.c > >> +++ b/board/freescale/common/fsl_chain_of_trust.c > >> @@ -6,7 +6,17 @@ > >> > >> #include <common.h> > >> #include <fsl_validate.h> > >> +#include <fsl_secboot_err.h> > >> #include <fsl_sfp.h> > >> +#include <dm/root.h> > >> + > >> +#ifdef CONFIG_ADDR_MAP > >> +#include <asm/mmu.h> > >> +#endif > >> + > >> +#ifdef CONFIG_FSL_CORENET > >> +#include <asm/fsl_pamu.h> > >> +#endif > >> > >> #ifdef CONFIG_LS102XA > >> #include <asm/arch/immap_ls102xa.h> > >> @@ -52,6 +62,7 @@ int fsl_check_boot_mode_secure(void) > >> return 0; > >> } > >> > >> +#ifndef CONFIG_SPL_BUILD > >> int fsl_setenv_chain_of_trust(void) > >> { > >> /* Check Boot Mode > >> @@ -68,3 +79,42 @@ int fsl_setenv_chain_of_trust(void) > >> setenv("bootcmd", CONFIG_CHAIN_BOOT_CMD); > >> return 0; > >> } > >> +#endif > >> + > >> +#ifdef CONFIG_SPL_BUILD > >> +void spl_validate_uboot(uint32_t hdr_addr, uintptr_t img_addr) { > >> + int res; > >> + > >> + /* Check Boot Mode > >> + * If Boot Mode is Non-Secure, skip validation > >> + */ > >> + if (fsl_check_boot_mode_secure() == 0) > >> + return; > >> + > >> + printf("SPL: Validating U-Boot image\n"); > >> + > >> +#ifdef CONFIG_ADDR_MAP > >> + init_addr_map(); > >> +#endif > >> + > >> +#ifdef CONFIG_FSL_CORENET > >> + if (pamu_init() < 0) > >> + fsl_secboot_handle_error(ERROR_ESBC_PAMU_INIT); > >> +#endif > >> + > >> +#ifdef CONFIG_FSL_CAAM > >> + if (sec_init() < 0) > >> + fsl_secboot_handle_error(ERROR_ESBC_SEC_INIT); > >> +#endif > >> + > >> +#if defined(CONFIG_DM) > >> + dm_init_and_scan(false); > > Eek, this is duplicating code and adding a board-specific hack. I agree that > it > would be better to use SPL FIT to handle this. Currently our powerpc based platforms do not use generic SPL framework but uses board specific SPL framework (/board/freescale/<platform>/spl.c) and do not use DM in SPL either. So to include RSA library I called it here once rather than calling in every board specific file. Yeah you are correct I should use CONFIG_SPL_DM here instead of CONFIG_DM. > > >> +#endif > >> + res = fsl_secboot_validate(hdr_addr, CONFIG_SPL_UBOOT_KEY_HASH, > >> + &img_addr); > >> + > >> + if (res == 0) > >> + printf("SPL: Validation of U-boot successful\n"); } > >> +#endif > >> diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c index > >> 510fa4e..1d4dd32 100644 > >> --- a/drivers/crypto/fsl/jr.c > >> +++ b/drivers/crypto/fsl/jr.c > >> @@ -599,10 +599,26 @@ int sec_init_idx(uint8_t sec_idx) > >> sec_out32(&sec->mcfgr, mcr); > >> > >> #ifdef CONFIG_FSL_CORENET > >> +#ifdef CONFIG_SPL_BUILD > >> + /* For SPL Build, Set the Liodns in SEC JR0 for > >> + * creating PAMU entries corresponding to these. > >> + * For normal build, these are set in set_liodns(). > >> + */ > >> + liodn_ns = CONFIG_SPL_JR0_LIODN_NS & JRNSLIODN_MASK; > >> + liodn_s = CONFIG_SPL_JR0_LIODN_S & JRSLIODN_MASK; > >> + > >> + liodnr = sec_in32(&sec->jrliodnr[0].ls) & > >> + ~(JRNSLIODN_MASK | JRSLIODN_MASK); > >> + liodnr = liodnr | > >> + (liodn_ns << JRNSLIODN_SHIFT) | > >> + (liodn_s << JRSLIODN_SHIFT); > >> + sec_out32(&sec->jrliodnr[0].ls, liodnr); #else > >> liodnr = sec_in32(&sec->jrliodnr[0].ls); > >> liodn_ns = (liodnr & JRNSLIODN_MASK) >> JRNSLIODN_SHIFT; > >> liodn_s = (liodnr & JRSLIODN_MASK) >> JRSLIODN_SHIFT; #endif > >> +#endif > >> > >> ret = jr_init(sec_idx); > >> if (ret < 0) { > >> diff --git a/drivers/mtd/nand/fsl_ifc_spl.c > >> b/drivers/mtd/nand/fsl_ifc_spl.c > >> index cbeb74a..30aa966 100644 > >> --- a/drivers/mtd/nand/fsl_ifc_spl.c > >> +++ b/drivers/mtd/nand/fsl_ifc_spl.c > >> @@ -11,6 +11,9 @@ > >> #include <asm/io.h> > >> #include <fsl_ifc.h> > >> #include <linux/mtd/nand.h> > >> +#ifdef CONFIG_CHAIN_OF_TRUST > >> +#include <fsl_validate.h> > >> +#endif > >> > >> static inline int is_blank(uchar *addr, int page_size) > >> { > >> @@ -268,6 +271,27 @@ void nand_boot(void) > >> */ > >> flush_cache(CONFIG_SYS_NAND_U_BOOT_DST, > CONFIG_SYS_NAND_U_BOOT_SIZE); > >> #endif > >> + > >> +#ifdef CONFIG_CHAIN_OF_TRUST > >> + /* > >> + * As U-Boot header is appended at end of U-boot image, so > >> + * calculate U-boot header address using U-boot header size. > >> + */ > >> +#define CONFIG_U_BOOT_HDR_ADDR \ > >> + ((CONFIG_SYS_NAND_U_BOOT_START + \ > >> + CONFIG_SYS_NAND_U_BOOT_SIZE) - \ > >> + CONFIG_U_BOOT_HDR_SIZE) > >> + spl_validate_uboot(CONFIG_U_BOOT_HDR_ADDR, > >> + CONFIG_SYS_NAND_U_BOOT_START); > >> + /* > >> + * In case of failure in validation, spl_validate_uboot would > >> + * not return back in case of Production environment with ITS=1. > >> + * Thus U-Boot will not start. > >> + * In Development environment (ITS=0 and SB_EN=1), the function > >> + * may return back in case of non-fatal failures. > >> + */ > >> +#endif > >> + > >> uboot = (void *)CONFIG_SYS_NAND_U_BOOT_START; > >> uboot(); > >> } > >> diff --git a/include/fsl_validate.h b/include/fsl_validate.h > >> index a71e1ce..7695b30 100644 > >> --- a/include/fsl_validate.h > >> +++ b/include/fsl_validate.h > >> @@ -254,4 +254,5 @@ int fsl_secboot_blob_decap(cmd_tbl_t *cmdtp, int > flag, int argc, > >> > >> int fsl_check_boot_mode_secure(void); > >> int fsl_setenv_chain_of_trust(void); > >> +void spl_validate_uboot(uint32_t hdr_addr, uintptr_t img_addr); > >> #endif > >> -- > >> 1.8.1.4 > >> > > Regards, > Simon Regards, Sumit _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
