* [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs
@ 2016-06-15 19:26 Andreas Dannenberg
2016-06-15 19:26 ` [U-Boot] [RFC 1/9] spl: fit: add support for post-processing of images Andreas Dannenberg
` (8 more replies)
0 siblings, 9 replies; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
This is an RFC for a method that uses a "weak" post-process function call
that's injected into the SPL FIT loading process after each blob has been
extracted (U-Boot firmware, selected DTB) which is populated with a platform
specific function. In case of TI high-security (HS) device variants a ROM API
call is performed (via SMC) that transparently handles authentication and
optional decryption. This essentially allows authenticating (and optionally
decrypting) U-Boot from SPL. The post-processing injection is implemented such
to enable a more universal use of this feature by other platforms or for other
purposes.
Furthermore the build process has been modified to automatically invoke a TI
blob signing/encryption tool through the $TI_SECURE_DEV_PKG env variable
already introduced into U-Boot. This singing/encryption step happens for each
artifact that gets bundled into the final u-boot.img FIT blob.
Why do we need this for our platforms if some generic form of verified boot
already exists in U-Boot? The approach proposed here provides support for
decryption which is currently not available in U-Boot (and may not be easily
implementable generically for example due to the need for keeping symmetric
keys secure). Furthermore it allows for a consistent build as well as runtime
flow no matter authentication and/or decryption is used (as opposed to using
existing U-Boot verified boot for authentication and an additional TI-specific
ROM API call for decryption for example). It also allows for a faster and more
efficient boot process (auth and decryption can be performed in a single step
by the ROM APIs also utilizing crypto HW accelerators in a completely
transparent fashion). However anyone can still use the standard U-Boot verified
boot scheme from U-Boot onwards if they so chose and only need authentication.
The patch series has been tested on DRA7 HS, DRA72 HS, and AM57 HS device
variants. The AM43 HS support is still missing some Makefile changes but the
principle in the final implementation will be similar what's implemented for
the other devices.
Regards,
Andreas Dannenberg
PS: This patch series depends on a few recent patches sent by Lokesh, Madan,
and myself that enable SPL FIT support for various high-secure ICs. However
it should not be necessary to look up those patches for purposes of
digesting this RFC.
Daniel Allred (7):
spl: fit: add support for post-processing of images
arm: cache: add missing dummy functions for when dcache disabled
arm: omap-common: add secure smc entry
arm: omap-common: add secure rom call API for secure devices
arm: omap5: add secure ROM signature verify API
arm: omap5: add FIT image post process function
ti: omap-common: Update to generate secure FIT
Madan Srinivas (2):
arm: am4x: add secure ROM signature verify API
arm: am4x: add FIT image post process function
arch/arm/cpu/armv7/am33xx/Makefile | 2 +
arch/arm/cpu/armv7/am33xx/sec_fxns.c | 90 +++++++++++++++++++++++++
arch/arm/cpu/armv7/cache_v7.c | 8 +++
arch/arm/cpu/armv7/omap-common/Makefile | 4 ++
arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 +++++++++++++++-
arch/arm/cpu/armv7/omap-common/lowlevel_init.S | 47 +++++++++++--
arch/arm/cpu/armv7/omap-common/sec_bridge.c | 47 +++++++++++++
arch/arm/cpu/armv7/omap5/Makefile | 1 +
arch/arm/cpu/armv7/omap5/config.mk | 3 +
arch/arm/cpu/armv7/omap5/sec_fxns.c | 70 +++++++++++++++++++
arch/arm/include/asm/arch-am33xx/sys_proto.h | 6 +-
arch/arm/include/asm/arch-omap5/sys_proto.h | 4 ++
arch/arm/include/asm/omap_common.h | 6 ++
board/ti/am43xx/board.c | 7 ++
board/ti/am57xx/board.c | 7 ++
board/ti/dra7xx/evm.c | 7 ++
common/spl/spl_fit.c | 21 ++++--
include/configs/ti_omap5_common.h | 4 ++
include/image.h | 15 +++++
19 files changed, 391 insertions(+), 15 deletions(-)
create mode 100644 arch/arm/cpu/armv7/am33xx/sec_fxns.c
create mode 100644 arch/arm/cpu/armv7/omap-common/sec_bridge.c
create mode 100644 arch/arm/cpu/armv7/omap5/sec_fxns.c
--
2.6.4
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 1/9] spl: fit: add support for post-processing of images
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
@ 2016-06-15 19:26 ` Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-15 19:26 ` [U-Boot] [RFC 2/9] arm: cache: add missing dummy functions for when dcache disabled Andreas Dannenberg
` (7 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
From: Daniel Allred <d-allred@ti.com>
The next stage boot loader image and the selected FDT can be
post-processed by board/platform/device-specific code, which can include
modifying the size and altering the starting source address before
copying these binary blobs to their final desitination. This might be
desired to do things like strip headers or footers attached to the
images before they were packeaged into the FIT, or to perform operations
such as decryption or authentication.
Signed-off-by: Daniel Allred <d-allred@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
common/spl/spl_fit.c | 21 ++++++++++++++++-----
include/image.h | 15 +++++++++++++++
2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 9874708..ecbcb97 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -121,6 +121,10 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
return (data_size + info->bl_len - 1) / info->bl_len;
}
+void __weak board_fit_image_post_process(void **p_src, size_t *p_size)
+{
+}
+
int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
{
int sectors;
@@ -132,7 +136,7 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
int data_offset, data_size;
int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
int src_sector;
- void *dst;
+ void *dst, *src;
/*
* Figure out where the external images start. This is the base for the
@@ -206,8 +210,11 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
return -EIO;
debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,
data_size);
- memcpy(dst, dst + get_aligned_image_overhead(info, data_offset),
- data_size);
+ src = dst + get_aligned_image_overhead(info, data_offset);
+
+ board_fit_image_post_process((void **)&src, (size_t *)&data_size);
+
+ memcpy(dst, src, data_size);
/* Figure out which device tree the board wants to use */
fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset);
@@ -236,8 +243,12 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
*/
debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,
fdt_len);
- memcpy(load_ptr + data_size,
- dst + get_aligned_image_overhead(info, fdt_offset), fdt_len);
+ src = dst + get_aligned_image_overhead(info, fdt_offset);
+ dst = load_ptr + data_size;
+
+ board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
+
+ memcpy(dst, src, fdt_len);
return 0;
}
diff --git a/include/image.h b/include/image.h
index a8f6bd1..9536874 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1172,4 +1172,19 @@ ulong android_image_get_kload(const struct andr_img_hdr *hdr);
*/
int board_fit_config_name_match(const char *name);
+/**
+ * board_fit_image_post_process() - Do any post-process on FIT binary data
+ *
+ * This is used to do any sort of image manipulation, verification, decryption
+ * etc. in a platform or board specific way. Obviously, anything done here would
+ * need to be comprehended in how the images were prepared before being injected
+ * into the FIT creation (i.e. the binary blobs would have been pre-processed
+ * before being added to the FIT image).
+ *
+ * @image: pointer to the image start pointer
+ * @size: pointer to the image size
+ * @return no return value (failure should be handled internally)
+ */
+void board_fit_image_post_process(void **p_image, size_t *p_size);
+
#endif /* __IMAGE_H__ */
--
2.6.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 2/9] arm: cache: add missing dummy functions for when dcache disabled
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
2016-06-15 19:26 ` [U-Boot] [RFC 1/9] spl: fit: add support for post-processing of images Andreas Dannenberg
@ 2016-06-15 19:26 ` Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-20 2:13 ` Tom Rini
2016-06-15 19:26 ` [U-Boot] [RFC 3/9] arm: omap-common: add secure smc entry Andreas Dannenberg
` (6 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
From: Daniel Allred <d-allred@ti.com>
Adds missing flush_dcache_range and invalidate_dcache_range dummy
(empty) placeholder functions to the #else portion of the #ifndef
CONFIG_SYS_DCACHE_OFF, where full implementations of these functions
are defined.
Signed-off-by: Daniel Allred <d-allred@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
arch/arm/cpu/armv7/cache_v7.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
index dc309da..24fe0c5 100644
--- a/arch/arm/cpu/armv7/cache_v7.c
+++ b/arch/arm/cpu/armv7/cache_v7.c
@@ -195,6 +195,14 @@ void flush_dcache_all(void)
{
}
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+}
+
+void flush_dcache_range(unsigned long start, unsigned long stop)
+{
+}
+
void arm_init_before_mmu(void)
{
}
--
2.6.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 3/9] arm: omap-common: add secure smc entry
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
2016-06-15 19:26 ` [U-Boot] [RFC 1/9] spl: fit: add support for post-processing of images Andreas Dannenberg
2016-06-15 19:26 ` [U-Boot] [RFC 2/9] arm: cache: add missing dummy functions for when dcache disabled Andreas Dannenberg
@ 2016-06-15 19:26 ` Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-15 19:26 ` [U-Boot] [RFC 4/9] arm: omap-common: add secure rom call API for secure devices Andreas Dannenberg
` (5 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
From: Daniel Allred <d-allred@ti.com>
Adds an interface for calling secure ROM APIs across a range of OMAP and
OMAP compatible devices.
Signed-off-by: Daniel Allred <d-allred@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
arch/arm/cpu/armv7/omap-common/lowlevel_init.S | 47 ++++++++++++++++++++++----
arch/arm/include/asm/omap_common.h | 2 ++
2 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/arch/arm/cpu/armv7/omap-common/lowlevel_init.S b/arch/arm/cpu/armv7/omap-common/lowlevel_init.S
index 5283135..2a710ce 100644
--- a/arch/arm/cpu/armv7/omap-common/lowlevel_init.S
+++ b/arch/arm/cpu/armv7/omap-common/lowlevel_init.S
@@ -16,6 +16,8 @@
#include <asm/arch/spl.h>
#include <linux/linkage.h>
+.arch_extension sec
+
#ifdef CONFIG_SPL
ENTRY(save_boot_params)
@@ -26,14 +28,45 @@ ENDPROC(save_boot_params)
#endif
ENTRY(omap_smc1)
- PUSH {r4-r12, lr} @ save registers - ROM code may pollute
+ push {r4-r12, lr} @ save registers - ROM code may pollute
@ our registers
- MOV r12, r0 @ Service
- MOV r0, r1 @ Argument
- DSB
- DMB
- .word 0xe1600070 @ SMC #0 - hand assembled for GCC versions
+ mov r12, r0 @ Service
+ mov r0, r1 @ Argument
+ dsb
+ dmb
+ smc 0 @ SMC #0 to enter monitor mode
@ call ROM Code API for the service requested
- POP {r4-r12, pc}
+ pop {r4-r12, pc}
ENDPROC(omap_smc1)
+
+ENTRY(omap_smc_sec)
+ push {r4-r12, lr} @ save registers - ROM code may pollute
+
+ mov r6, #0xFF @ Indicate new Task call
+
+ mov r12, #0x00 @ Secure Service ID in R12
+
+ dsb
+ dmb
+
+ smc 0 @ SMC #0 to enter monitor mode
+ b omap_smc_sec_end /* exit at end of the service execution */
+ nop
+
+ /* In case of IRQ happening in Secure, then ARM will branch here.
+ * At that moment, IRQ will be pending and ARM will jump to Non Secure
+ * IRQ handler
+ */
+ mov r12, #0xFE
+
+ dsb
+ dmb
+
+ smc 0 @ SMC #0 to enter monitor mode
+
+omap_smc_sec_end:
+ pop {r4-r12, lr}
+ bx lr
+
+ENDPROC(omap_smc_sec)
diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
index 07f3848..5943e6f 100644
--- a/arch/arm/include/asm/omap_common.h
+++ b/arch/arm/include/asm/omap_common.h
@@ -627,6 +627,8 @@ void recalibrate_iodelay(void);
void omap_smc1(u32 service, u32 val);
+u32 omap_smc_sec(u32 service, u32 proc_id, u32 flag, u32 *params);
+
void enable_edma3_clocks(void);
void disable_edma3_clocks(void);
--
2.6.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 4/9] arm: omap-common: add secure rom call API for secure devices
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
` (2 preceding siblings ...)
2016-06-15 19:26 ` [U-Boot] [RFC 3/9] arm: omap-common: add secure smc entry Andreas Dannenberg
@ 2016-06-15 19:26 ` Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-17 4:18 ` Lokesh Vutla
2016-06-15 19:26 ` [U-Boot] [RFC 5/9] arm: omap5: add secure ROM signature verify API Andreas Dannenberg
` (4 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
From: Daniel Allred <d-allred@ti.com>
Adds a generic C-callable API for making secure ROM calls on OMAP and
OMAP-compatible devices. This API provides the important function of
flushing the ROM call arguments to memory from the cache, so that the
secure world will have a coherent view of those arguments. Then is
simply calls the omap_smc_sec routine.
Signed-off-by: Daniel Allred <d-allred@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
arch/arm/cpu/armv7/omap-common/Makefile | 4 +++
arch/arm/cpu/armv7/omap-common/sec_bridge.c | 47 +++++++++++++++++++++++++++++
arch/arm/include/asm/omap_common.h | 4 +++
3 files changed, 55 insertions(+)
create mode 100644 arch/arm/cpu/armv7/omap-common/sec_bridge.c
diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile
index 87a7ac0..4fc3926 100644
--- a/arch/arm/cpu/armv7/omap-common/Makefile
+++ b/arch/arm/cpu/armv7/omap-common/Makefile
@@ -28,6 +28,10 @@ obj-y += pipe3-phy.o
obj-$(CONFIG_SCSI_AHCI_PLAT) += sata.o
endif
+ifneq ($(CONFIG_TI_SECURE_DEVICE),)
+obj-y += sec_bridge.o
+endif
+
ifeq ($(CONFIG_SYS_DCACHE_OFF),)
obj-y += omap-cache.o
endif
diff --git a/arch/arm/cpu/armv7/omap-common/sec_bridge.c b/arch/arm/cpu/armv7/omap-common/sec_bridge.c
new file mode 100644
index 0000000..4eaba8e
--- /dev/null
+++ b/arch/arm/cpu/armv7/omap-common/sec_bridge.c
@@ -0,0 +1,47 @@
+/*
+ *
+ * Common bridge function to make OMAP secure ROM calls
+ *
+ * (C) Copyright 2016
+ * Texas Instruments, <www.ti.com>
+ *
+ * Daniel Allred <d-allred@ti.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <stdarg.h>
+
+#include <asm/arch/sys_proto.h>
+#include <asm/omap_common.h>
+
+static uint32_t secure_rom_call_args[5] __aligned(ARCH_DMA_MINALIGN);
+
+u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...)
+{
+ int i;
+ u32 num_args;
+ va_list ap;
+
+ va_start(ap, flag);
+
+ num_args = va_arg(ap, u32);
+
+ /* Copy args to aligned args structure */
+ for (i = 0; i < num_args; i++)
+ secure_rom_call_args[i + 1] = va_arg(ap, u32);
+
+ secure_rom_call_args[0] = num_args;
+
+ va_end(ap);
+
+ /* if data cache is enabled, flush the aligned args structure */
+#ifndef CONFIG_SYS_DCACHE_OFF
+ flush_dcache_range(
+ (unsigned int)&secure_rom_call_args[0],
+ (unsigned int)&secure_rom_call_args[0] +
+ roundup(sizeof(secure_rom_call_args), ARCH_DMA_MINALIGN));
+#endif
+ return omap_smc_sec(service, proc_id, flag, secure_rom_call_args);
+}
diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
index 5943e6f..cb02c88 100644
--- a/arch/arm/include/asm/omap_common.h
+++ b/arch/arm/include/asm/omap_common.h
@@ -629,6 +629,10 @@ void omap_smc1(u32 service, u32 val);
u32 omap_smc_sec(u32 service, u32 proc_id, u32 flag, u32 *params);
+#ifdef CONFIG_TI_SECURE_DEVICE
+u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...);
+#endif
+
void enable_edma3_clocks(void);
void disable_edma3_clocks(void);
--
2.6.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 5/9] arm: omap5: add secure ROM signature verify API
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
` (3 preceding siblings ...)
2016-06-15 19:26 ` [U-Boot] [RFC 4/9] arm: omap-common: add secure rom call API for secure devices Andreas Dannenberg
@ 2016-06-15 19:26 ` Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-15 19:26 ` [U-Boot] [RFC 6/9] arm: omap5: add FIT image post process function Andreas Dannenberg
` (3 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
From: Daniel Allred <d-allred@ti.com>
Adds an API that verifies a signature attached to an image (binary
blob). This API is basically a entry to a secure ROM service provided by
the device and accessed via an SMC call, using a particular calling
convention.
Signed-off-by: Daniel Allred <d-allred@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
arch/arm/cpu/armv7/omap5/Makefile | 1 +
arch/arm/cpu/armv7/omap5/sec_fxns.c | 70 +++++++++++++++++++++++++++++
arch/arm/include/asm/arch-omap5/sys_proto.h | 4 ++
3 files changed, 75 insertions(+)
create mode 100644 arch/arm/cpu/armv7/omap5/sec_fxns.c
diff --git a/arch/arm/cpu/armv7/omap5/Makefile b/arch/arm/cpu/armv7/omap5/Makefile
index 3caba86..d373bf4 100644
--- a/arch/arm/cpu/armv7/omap5/Makefile
+++ b/arch/arm/cpu/armv7/omap5/Makefile
@@ -14,3 +14,4 @@ obj-y += hw_data.o
obj-y += abb.o
obj-y += fdt.o
obj-$(CONFIG_IODELAY_RECALIBRATION) += dra7xx_iodelay.o
+obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o
diff --git a/arch/arm/cpu/armv7/omap5/sec_fxns.c b/arch/arm/cpu/armv7/omap5/sec_fxns.c
new file mode 100644
index 0000000..766333a
--- /dev/null
+++ b/arch/arm/cpu/armv7/omap5/sec_fxns.c
@@ -0,0 +1,70 @@
+/*
+ *
+ * Common security functions that rely on secure ROM services
+ *
+ * (C) Copyright 2016
+ * Texas Instruments, <www.ti.com>
+ *
+ * Daniel Allred <d-allred@ti.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/omap_common.h>
+
+#define SIGNATURE_LENGTH (0x118)
+
+/* API Index for OMAP5, DRA7xx */
+#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX (0x0000000E)
+
+int secure_boot_verify_image(void **image, size_t *size)
+{
+ int result = 1;
+ u32 cert_addr, sig_addr;
+ size_t cert_size;
+
+#ifndef CONFIG_SYS_DCACHE_OFF
+ /* Perform cache writeback on input buffer */
+ flush_dcache_range(
+ (u32)*image,
+ (u32)*image + roundup(*size, ARCH_DMA_MINALIGN));
+#endif
+ cert_addr = (uint32_t)*image;
+ *size -= SIGNATURE_LENGTH; /* Subtract out the signature size */
+ cert_size = *size;
+ sig_addr = cert_addr + cert_size;
+
+ /* Check if image load address is 32-bit aligned */
+ if (0 != (0x3 & cert_addr)) {
+ puts("Image is not 4-byte aligned.\n");
+ result = 1;
+ goto auth_exit;
+ }
+
+ /* Image size also should be multiple of 4 */
+ if (0 != (0x3 & cert_size)) {
+ puts("Image size is not 4-byte aligned.\n");
+ result = 1;
+ goto auth_exit;
+ }
+
+ /* Call ROM HAL API to verify certificate signature */
+ debug("%s: load_addr = %x, size = %x, sig_addr = %x\n", __func__,
+ cert_addr, cert_size, sig_addr);
+
+ result = secure_rom_call(
+ API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX, 0, 0,
+ 4, cert_addr, cert_size, sig_addr, 0xFFFFFFFF);
+auth_exit:
+ if (result != 0) {
+ puts("Authentication failed!\n");
+ printf("Return Value = %08X\n", result);
+ hang();
+ }
+
+ printf("Authentication passed: %s\n", (char *)sig_addr);
+
+ return result;
+}
diff --git a/arch/arm/include/asm/arch-omap5/sys_proto.h b/arch/arm/include/asm/arch-omap5/sys_proto.h
index ab0e7fa..b175124 100644
--- a/arch/arm/include/asm/arch-omap5/sys_proto.h
+++ b/arch/arm/include/asm/arch-omap5/sys_proto.h
@@ -84,4 +84,8 @@ static inline u32 usec_to_32k(u32 usec)
#define OMAP5_SERVICE_L2ACTLR_SET 0x104
#define OMAP5_SERVICE_ACR_SET 0x107
+#ifdef CONFIG_TI_SECURE_DEVICE
+int secure_boot_verify_image(void **p_image, size_t *p_size);
+#endif
+
#endif
--
2.6.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 6/9] arm: omap5: add FIT image post process function
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
` (4 preceding siblings ...)
2016-06-15 19:26 ` [U-Boot] [RFC 5/9] arm: omap5: add secure ROM signature verify API Andreas Dannenberg
@ 2016-06-15 19:26 ` Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-17 4:26 ` Lokesh Vutla
2016-06-15 19:26 ` [U-Boot] [RFC 7/9] arm: am4x: add secure ROM signature verify API Andreas Dannenberg
` (2 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
From: Daniel Allred <d-allred@ti.com>
Adds a board specific FIT image post processing function for when
CONFIG_SECURE_BOOT is defined. Also update the omap common config
header to enable CONFIG_SECURE_BOOT always for secure TI devices
(CONFIG_TI_SECURE_DEVICE is defined).
Signed-off-by: Daniel Allred <d-allred@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
board/ti/am57xx/board.c | 7 +++++++
board/ti/dra7xx/evm.c | 7 +++++++
include/configs/ti_omap5_common.h | 4 ++++
3 files changed, 18 insertions(+)
diff --git a/board/ti/am57xx/board.c b/board/ti/am57xx/board.c
index 08cf14d..a9635c2 100644
--- a/board/ti/am57xx/board.c
+++ b/board/ti/am57xx/board.c
@@ -750,3 +750,10 @@ int board_fit_config_name_match(const char *name)
return -1;
}
#endif
+
+#ifdef CONFIG_SECURE_BOOT
+void board_fit_image_post_process(void **p_image, size_t *p_size)
+{
+ secure_boot_verify_image(p_image, p_size);
+}
+#endif
diff --git a/board/ti/dra7xx/evm.c b/board/ti/dra7xx/evm.c
index 3fbbc9b..03eefb6 100644
--- a/board/ti/dra7xx/evm.c
+++ b/board/ti/dra7xx/evm.c
@@ -739,3 +739,10 @@ int board_fit_config_name_match(const char *name)
return -1;
}
#endif
+
+#ifdef CONFIG_SECURE_BOOT
+void board_fit_image_post_process(void **p_image, size_t *p_size)
+{
+ secure_boot_verify_image(p_image, p_size);
+}
+#endif
diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h
index 2e4c8e9..9db6da2 100644
--- a/include/configs/ti_omap5_common.h
+++ b/include/configs/ti_omap5_common.h
@@ -138,6 +138,10 @@
* print some information.
*/
#ifdef CONFIG_TI_SECURE_DEVICE
+
+/* Always enforce for secure devices */
+#define CONFIG_SECURE_BOOT
+
/*
* For memory booting on HS parts, the first 4KB of the internal RAM is
* reserved for secure world use and the flash loader image is
--
2.6.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 7/9] arm: am4x: add secure ROM signature verify API
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
` (5 preceding siblings ...)
2016-06-15 19:26 ` [U-Boot] [RFC 6/9] arm: omap5: add FIT image post process function Andreas Dannenberg
@ 2016-06-15 19:26 ` Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-15 19:26 ` [U-Boot] [RFC 8/9] arm: am4x: add FIT image post process function Andreas Dannenberg
2016-06-15 19:26 ` [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT Andreas Dannenberg
8 siblings, 1 reply; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
From: Madan Srinivas <madans@ti.com>
Adds an API that verifies a signature attached to an image (binary
blob). This API is basically a entry to a secure ROM service provided by
the device and accessed via an SMC call, using a particular calling
convention. This API is common across AM3x HS and AM4x HS devices.
Signed-off-by: Madan Srinivas <madans@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
arch/arm/cpu/armv7/am33xx/Makefile | 2 +
arch/arm/cpu/armv7/am33xx/sec_fxns.c | 90 ++++++++++++++++++++++++++++
arch/arm/include/asm/arch-am33xx/sys_proto.h | 6 +-
3 files changed, 97 insertions(+), 1 deletion(-)
create mode 100644 arch/arm/cpu/armv7/am33xx/sec_fxns.c
diff --git a/arch/arm/cpu/armv7/am33xx/Makefile b/arch/arm/cpu/armv7/am33xx/Makefile
index 6fda482..d2b3e37 100644
--- a/arch/arm/cpu/armv7/am33xx/Makefile
+++ b/arch/arm/cpu/armv7/am33xx/Makefile
@@ -20,3 +20,5 @@ obj-y += board.o
obj-y += mux.o
obj-$(CONFIG_CLOCK_SYNTHESIZER) += clk_synthesizer.o
+
+obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o
diff --git a/arch/arm/cpu/armv7/am33xx/sec_fxns.c b/arch/arm/cpu/armv7/am33xx/sec_fxns.c
new file mode 100644
index 0000000..560297c
--- /dev/null
+++ b/arch/arm/cpu/armv7/am33xx/sec_fxns.c
@@ -0,0 +1,90 @@
+/*
+ * sec_fxns.c
+ *
+ * Common security functions for AMxx devices that rely
+ * on secure ROM services.
+ *
+ * Copyright (C) 2013, Texas Instruments, Incorporated - http://www.ti.com/
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/omap_common.h>
+
+/* Index for signature verify ROM API*/
+#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX (0x0000000E)
+
+static u32 find_sig_start(char *image, size_t size)
+{
+ char *image_end = image + size;
+ char *sig_start_magic = "CERT_";
+ int magic_str_len = strlen(sig_start_magic);
+ char *ch;
+
+ while (--image_end > image) {
+ if (*image_end == '_') {
+ ch = image_end - magic_str_len + 1;
+ if (!strncmp(ch, sig_start_magic, magic_str_len))
+ return (u32)ch;
+ }
+ }
+ return 0;
+}
+
+int secure_boot_verify_image(void **image, size_t *size)
+{
+ int result = 1;
+ u32 cert_addr, sig_addr;
+ size_t cert_size;
+
+#ifndef CONFIG_SYS_DCACHE_OFF
+ /* Perform cache writeback on input buffer */
+ flush_dcache_range(
+ (u32)*image,
+ (u32)*image + roundup(*size, ARCH_DMA_MINALIGN));
+#endif
+ cert_addr = (uint32_t)*image;
+ sig_addr = find_sig_start((char *)*image, *size);
+
+ if (sig_addr == 0) {
+ puts("No signature found in image.\n");
+ result = 1;
+ goto auth_exit;
+ }
+
+ *size = sig_addr - cert_addr; /* Subtract out the signature size */
+ cert_size = *size;
+
+ /* Check if image load address is 32-bit aligned */
+ if (0 != (0x3 & cert_addr)) {
+ puts("Image is not 4-byte aligned.\n");
+ result = 1;
+ goto auth_exit;
+ }
+
+ /* Image size also should be multiple of 4 */
+ if (0 != (0x3 & cert_size)) {
+ puts("Image size is not 4-byte aligned.\n");
+ result = 1;
+ goto auth_exit;
+ }
+
+ /* Call ROM HAL API to verify certificate signature */
+ debug("%s: load_addr = %x, size = %x, sig_addr = %x\n", __func__,
+ cert_addr, cert_size, sig_addr);
+
+ result = secure_rom_call(
+ API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX, 0, 0,
+ 4, cert_addr, cert_size, sig_addr, 0xFFFFFFFF);
+auth_exit:
+ if (result != 0) {
+ puts("Authentication failed!\n");
+ printf("Return Value = %08X\n", result);
+ hang();
+ }
+
+ printf("Authentication passed: %s\n", (char *)sig_addr);
+ return result;
+}
diff --git a/arch/arm/include/asm/arch-am33xx/sys_proto.h b/arch/arm/include/asm/arch-am33xx/sys_proto.h
index 8f573d2..f5fc916 100644
--- a/arch/arm/include/asm/arch-am33xx/sys_proto.h
+++ b/arch/arm/include/asm/arch-am33xx/sys_proto.h
@@ -41,7 +41,11 @@ void enable_norboot_pin_mux(void);
void am33xx_spl_board_init(void);
int am335x_get_efuse_mpu_max_freq(struct ctrl_dev *cdev);
int am335x_get_tps65910_mpu_vdd(int sil_rev, int frequency);
-#endif
void enable_usb_clocks(int index);
void disable_usb_clocks(int index);
+
+#ifdef CONFIG_TI_SECURE_DEVICE
+int secure_boot_verify_image(void **p_image, size_t *p_size);
+#endif
+#endif
--
2.6.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 8/9] arm: am4x: add FIT image post process function
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
` (6 preceding siblings ...)
2016-06-15 19:26 ` [U-Boot] [RFC 7/9] arm: am4x: add secure ROM signature verify API Andreas Dannenberg
@ 2016-06-15 19:26 ` Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-17 4:27 ` Lokesh Vutla
2016-06-15 19:26 ` [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT Andreas Dannenberg
8 siblings, 2 replies; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
From: Madan Srinivas <madans@ti.com>
Adds a board specific FIT image post processing function when u-boot is
compiled for the high-secure (HS) device variant.
Signed-off-by: Madan Srinivas <madans@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
board/ti/am43xx/board.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/board/ti/am43xx/board.c b/board/ti/am43xx/board.c
index f005762..fc0b38b 100644
--- a/board/ti/am43xx/board.c
+++ b/board/ti/am43xx/board.c
@@ -862,3 +862,10 @@ int board_fit_config_name_match(const char *name)
return -1;
}
#endif
+
+#ifdef CONFIG_TI_SECURE_DEVICE
+void board_fit_image_post_process(void **p_image, size_t *p_size)
+{
+ secure_boot_verify_image(p_image, p_size);
+}
+#endif
--
2.6.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
` (7 preceding siblings ...)
2016-06-15 19:26 ` [U-Boot] [RFC 8/9] arm: am4x: add FIT image post process function Andreas Dannenberg
@ 2016-06-15 19:26 ` Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
8 siblings, 1 reply; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-15 19:26 UTC (permalink / raw)
To: u-boot
From: Daniel Allred <d-allred@ti.com>
Adds commands so that when a secure device is in use and the SPL is
built to load a FIT image (with combined u-boot binary and various
DTBs), these components that get fed into the FIT are all processed to
be signed/encrypted/etc. as per the operations performed by the
secure-binary-image script of the TI SECDEV package.
Signed-off-by: Daniel Allred <d-allred@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++-
arch/arm/cpu/armv7/omap5/config.mk | 3 ++
2 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk
index c7bb101..c4514ad 100644
--- a/arch/arm/cpu/armv7/omap-common/config_secure.mk
+++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk
@@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
$(if $(KBUILD_VERBOSE:1=), >/dev/null)
else
cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
- $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
- $(if $(KBUILD_VERBOSE:1=), >/dev/null)
+ $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
+ $(if $(KBUILD_VERBOSE:1=), >/dev/null)
endif
else
cmd_mkomapsecimg = echo "WARNING:" \
@@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
"variable must be defined for TI secure devices. $@ was NOT created!"
endif
+ifdef CONFIG_SPL_LOAD_FIT
+quiet_cmd_omapsecureimg = SECURE $@
+ifneq ($(TI_SECURE_DEV_PKG),)
+ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),)
+cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \
+ $< $@ \
+ $(if $(KBUILD_VERBOSE:1=), >/dev/null)
+else
+cmd_omapsecureimg = echo "WARNING:" \
+ "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
+ "$@ was NOT created!"; cp $< $@
+endif
+else
+cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
+ "variable must be defined for TI secure devices." \
+ "$@ was NOT created!"; cp $< $@
+endif
+endif
+
+
# Standard X-LOADER target (QPSI, NOR flash)
u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin
$(call if_changed,mkomapsecimg)
@@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin
# the mkomapsecimg command looks for a u-boot-HS_* prefix
u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin
$(call if_changed,mkomapsecimg)
+
+# For supporting the SPL loading and interpreting
+# of FIT images whose components are pre-processed
+# before being integrated into the FIT image in order
+# to secure them in some way
+ifdef CONFIG_SPL_LOAD_FIT
+
+MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
+ -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
+ -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
+ $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
+
+OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
+$(OF_LIST_TARGETS): dtbs
+
+%_HS.dtb: %.dtb
+ $(call if_changed,omapsecureimg)
+ $(Q)if [ -f $@ ]; then \
+ cp -f $@ $<; \
+ fi
+
+u-boot-nodtb_HS.bin: u-boot-nodtb.bin
+ $(call if_changed,omapsecureimg)
+
+u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))
+ $(call if_changed,mkimage)
+ $(Q)if [ -f $@ ]; then \
+ cp -f $@ u-boot.img; \
+ fi
+
+.NOTPARALLEL: dtbs
+
+endif
diff --git a/arch/arm/cpu/armv7/omap5/config.mk b/arch/arm/cpu/armv7/omap5/config.mk
index a7e55a5..503f31c 100644
--- a/arch/arm/cpu/armv7/omap5/config.mk
+++ b/arch/arm/cpu/armv7/omap5/config.mk
@@ -15,5 +15,8 @@ else
ALL-y += MLO
endif
else
+ifeq ($(CONFIG_TI_SECURE_DEVICE)$(CONFIG_SECURE_BOOT),yy)
+ALL-y += u-boot_HS.img
+endif
ALL-y += u-boot.img
endif
--
2.6.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 1/9] spl: fit: add support for post-processing of images
2016-06-15 19:26 ` [U-Boot] [RFC 1/9] spl: fit: add support for post-processing of images Andreas Dannenberg
@ 2016-06-17 3:52 ` Simon Glass
0 siblings, 0 replies; 29+ messages in thread
From: Simon Glass @ 2016-06-17 3:52 UTC (permalink / raw)
To: u-boot
Hi Andreas,
On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> The next stage boot loader image and the selected FDT can be
> post-processed by board/platform/device-specific code, which can include
> modifying the size and altering the starting source address before
> copying these binary blobs to their final desitination. This might be
> desired to do things like strip headers or footers attached to the
> images before they were packeaged into the FIT, or to perform operations
> such as decryption or authentication.
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> common/spl/spl_fit.c | 21 ++++++++++++++++-----
> include/image.h | 15 +++++++++++++++
> 2 files changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 9874708..ecbcb97 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -121,6 +121,10 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
> return (data_size + info->bl_len - 1) / info->bl_len;
> }
>
> +void __weak board_fit_image_post_process(void **p_src, size_t *p_size)
> +{
> +}
Instead of making this weak, can you add a Kconfig option to enable
this feature?
> +
> int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
> {
> int sectors;
> @@ -132,7 +136,7 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
> int data_offset, data_size;
> int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
> int src_sector;
> - void *dst;
> + void *dst, *src;
>
> /*
> * Figure out where the external images start. This is the base for the
> @@ -206,8 +210,11 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
> return -EIO;
> debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,
> data_size);
> - memcpy(dst, dst + get_aligned_image_overhead(info, data_offset),
> - data_size);
> + src = dst + get_aligned_image_overhead(info, data_offset);
> +
> + board_fit_image_post_process((void **)&src, (size_t *)&data_size);
> +
> + memcpy(dst, src, data_size);
>
> /* Figure out which device tree the board wants to use */
> fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset);
> @@ -236,8 +243,12 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
> */
> debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,
> fdt_len);
> - memcpy(load_ptr + data_size,
> - dst + get_aligned_image_overhead(info, fdt_offset), fdt_len);
> + src = dst + get_aligned_image_overhead(info, fdt_offset);
> + dst = load_ptr + data_size;
> +
> + board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
> +
> + memcpy(dst, src, fdt_len);
>
> return 0;
> }
> diff --git a/include/image.h b/include/image.h
> index a8f6bd1..9536874 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1172,4 +1172,19 @@ ulong android_image_get_kload(const struct andr_img_hdr *hdr);
> */
> int board_fit_config_name_match(const char *name);
>
> +/**
> + * board_fit_image_post_process() - Do any post-process on FIT binary data
> + *
> + * This is used to do any sort of image manipulation, verification, decryption
> + * etc. in a platform or board specific way. Obviously, anything done here would
> + * need to be comprehended in how the images were prepared before being injected
> + * into the FIT creation (i.e. the binary blobs would have been pre-processed
> + * before being added to the FIT image).
> + *
> + * @image: pointer to the image start pointer
> + * @size: pointer to the image size
> + * @return no return value (failure should be handled internally)
> + */
> +void board_fit_image_post_process(void **p_image, size_t *p_size);
> +
> #endif /* __IMAGE_H__ */
> --
> 2.6.4
>
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 2/9] arm: cache: add missing dummy functions for when dcache disabled
2016-06-15 19:26 ` [U-Boot] [RFC 2/9] arm: cache: add missing dummy functions for when dcache disabled Andreas Dannenberg
@ 2016-06-17 3:52 ` Simon Glass
2016-06-20 2:13 ` Tom Rini
1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2016-06-17 3:52 UTC (permalink / raw)
To: u-boot
On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> Adds missing flush_dcache_range and invalidate_dcache_range dummy
> (empty) placeholder functions to the #else portion of the #ifndef
> CONFIG_SYS_DCACHE_OFF, where full implementations of these functions
> are defined.
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> arch/arm/cpu/armv7/cache_v7.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 3/9] arm: omap-common: add secure smc entry
2016-06-15 19:26 ` [U-Boot] [RFC 3/9] arm: omap-common: add secure smc entry Andreas Dannenberg
@ 2016-06-17 3:52 ` Simon Glass
0 siblings, 0 replies; 29+ messages in thread
From: Simon Glass @ 2016-06-17 3:52 UTC (permalink / raw)
To: u-boot
On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> Adds an interface for calling secure ROM APIs across a range of OMAP and
> OMAP compatible devices.
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> arch/arm/cpu/armv7/omap-common/lowlevel_init.S | 47 ++++++++++++++++++++++----
> arch/arm/include/asm/omap_common.h | 2 ++
> 2 files changed, 42 insertions(+), 7 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
But please see below.
>
> diff --git a/arch/arm/cpu/armv7/omap-common/lowlevel_init.S b/arch/arm/cpu/armv7/omap-common/lowlevel_init.S
> index 5283135..2a710ce 100644
> --- a/arch/arm/cpu/armv7/omap-common/lowlevel_init.S
> +++ b/arch/arm/cpu/armv7/omap-common/lowlevel_init.S
> @@ -16,6 +16,8 @@
> #include <asm/arch/spl.h>
> #include <linux/linkage.h>
>
> +.arch_extension sec
> +
> #ifdef CONFIG_SPL
> ENTRY(save_boot_params)
>
> @@ -26,14 +28,45 @@ ENDPROC(save_boot_params)
> #endif
>
> ENTRY(omap_smc1)
> - PUSH {r4-r12, lr} @ save registers - ROM code may pollute
> + push {r4-r12, lr} @ save registers - ROM code may pollute
> @ our registers
> - MOV r12, r0 @ Service
> - MOV r0, r1 @ Argument
> - DSB
> - DMB
> - .word 0xe1600070 @ SMC #0 - hand assembled for GCC versions
> + mov r12, r0 @ Service
> + mov r0, r1 @ Argument
> + dsb
> + dmb
> + smc 0 @ SMC #0 to enter monitor mode
> @ call ROM Code API for the service requested
>
> - POP {r4-r12, pc}
> + pop {r4-r12, pc}
> ENDPROC(omap_smc1)
> +
> +ENTRY(omap_smc_sec)
> + push {r4-r12, lr} @ save registers - ROM code may pollute
> +
> + mov r6, #0xFF @ Indicate new Task call
> +
> + mov r12, #0x00 @ Secure Service ID in R12
> +
> + dsb
> + dmb
> +
> + smc 0 @ SMC #0 to enter monitor mode
> + b omap_smc_sec_end /* exit at end of the service execution */
> + nop
> +
> + /* In case of IRQ happening in Secure, then ARM will branch here.
> + * At that moment, IRQ will be pending and ARM will jump to Non Secure
> + * IRQ handler
> + */
> + mov r12, #0xFE
> +
> + dsb
> + dmb
> +
> + smc 0 @ SMC #0 to enter monitor mode
> +
> +omap_smc_sec_end:
> + pop {r4-r12, lr}
> + bx lr
> +
> +ENDPROC(omap_smc_sec)
> diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
> index 07f3848..5943e6f 100644
> --- a/arch/arm/include/asm/omap_common.h
> +++ b/arch/arm/include/asm/omap_common.h
> @@ -627,6 +627,8 @@ void recalibrate_iodelay(void);
>
> void omap_smc1(u32 service, u32 val);
>
> +u32 omap_smc_sec(u32 service, u32 proc_id, u32 flag, u32 *params);
Please add function comments
> +
> void enable_edma3_clocks(void);
> void disable_edma3_clocks(void);
>
> --
> 2.6.4
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 4/9] arm: omap-common: add secure rom call API for secure devices
2016-06-15 19:26 ` [U-Boot] [RFC 4/9] arm: omap-common: add secure rom call API for secure devices Andreas Dannenberg
@ 2016-06-17 3:52 ` Simon Glass
2016-06-17 4:18 ` Lokesh Vutla
1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2016-06-17 3:52 UTC (permalink / raw)
To: u-boot
On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> Adds a generic C-callable API for making secure ROM calls on OMAP and
> OMAP-compatible devices. This API provides the important function of
> flushing the ROM call arguments to memory from the cache, so that the
> secure world will have a coherent view of those arguments. Then is
> simply calls the omap_smc_sec routine.
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> arch/arm/cpu/armv7/omap-common/Makefile | 4 +++
> arch/arm/cpu/armv7/omap-common/sec_bridge.c | 47 +++++++++++++++++++++++++++++
> arch/arm/include/asm/omap_common.h | 4 +++
> 3 files changed, 55 insertions(+)
> create mode 100644 arch/arm/cpu/armv7/omap-common/sec_bridge.c
Reviewed-by: Simon Glass <sjg@chromium.org>
But please see below.
>
> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile
> index 87a7ac0..4fc3926 100644
> --- a/arch/arm/cpu/armv7/omap-common/Makefile
> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
> @@ -28,6 +28,10 @@ obj-y += pipe3-phy.o
> obj-$(CONFIG_SCSI_AHCI_PLAT) += sata.o
> endif
>
> +ifneq ($(CONFIG_TI_SECURE_DEVICE),)
> +obj-y += sec_bridge.o
> +endif
> +
> ifeq ($(CONFIG_SYS_DCACHE_OFF),)
> obj-y += omap-cache.o
> endif
> diff --git a/arch/arm/cpu/armv7/omap-common/sec_bridge.c b/arch/arm/cpu/armv7/omap-common/sec_bridge.c
> new file mode 100644
> index 0000000..4eaba8e
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/omap-common/sec_bridge.c
> @@ -0,0 +1,47 @@
> +/*
> + *
> + * Common bridge function to make OMAP secure ROM calls
> + *
> + * (C) Copyright 2016
> + * Texas Instruments, <www.ti.com>
> + *
> + * Daniel Allred <d-allred@ti.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <stdarg.h>
> +
> +#include <asm/arch/sys_proto.h>
> +#include <asm/omap_common.h>
> +
> +static uint32_t secure_rom_call_args[5] __aligned(ARCH_DMA_MINALIGN);
> +
> +u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...)
> +{
> + int i;
> + u32 num_args;
> + va_list ap;
> +
> + va_start(ap, flag);
> +
> + num_args = va_arg(ap, u32);
> +
> + /* Copy args to aligned args structure */
> + for (i = 0; i < num_args; i++)
> + secure_rom_call_args[i + 1] = va_arg(ap, u32);
> +
> + secure_rom_call_args[0] = num_args;
> +
> + va_end(ap);
> +
> + /* if data cache is enabled, flush the aligned args structure */
> +#ifndef CONFIG_SYS_DCACHE_OFF
> + flush_dcache_range(
> + (unsigned int)&secure_rom_call_args[0],
> + (unsigned int)&secure_rom_call_args[0] +
> + roundup(sizeof(secure_rom_call_args), ARCH_DMA_MINALIGN));
> +#endif
> + return omap_smc_sec(service, proc_id, flag, secure_rom_call_args);
> +}
> diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
> index 5943e6f..cb02c88 100644
> --- a/arch/arm/include/asm/omap_common.h
> +++ b/arch/arm/include/asm/omap_common.h
> @@ -629,6 +629,10 @@ void omap_smc1(u32 service, u32 val);
>
> u32 omap_smc_sec(u32 service, u32 proc_id, u32 flag, u32 *params);
>
> +#ifdef CONFIG_TI_SECURE_DEVICE
> +u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...);
You don't need the #ifdef here (but it's up to you).
Also please add a function comment.
> +#endif
> +
> void enable_edma3_clocks(void);
> void disable_edma3_clocks(void);
>
> --
> 2.6.4
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 5/9] arm: omap5: add secure ROM signature verify API
2016-06-15 19:26 ` [U-Boot] [RFC 5/9] arm: omap5: add secure ROM signature verify API Andreas Dannenberg
@ 2016-06-17 3:52 ` Simon Glass
2016-06-20 2:13 ` Tom Rini
0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2016-06-17 3:52 UTC (permalink / raw)
To: u-boot
On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> Adds an API that verifies a signature attached to an image (binary
> blob). This API is basically a entry to a secure ROM service provided by
> the device and accessed via an SMC call, using a particular calling
> convention.
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> arch/arm/cpu/armv7/omap5/Makefile | 1 +
> arch/arm/cpu/armv7/omap5/sec_fxns.c | 70 +++++++++++++++++++++++++++++
> arch/arm/include/asm/arch-omap5/sys_proto.h | 4 ++
> 3 files changed, 75 insertions(+)
> create mode 100644 arch/arm/cpu/armv7/omap5/sec_fxns.c
Reviewed-by: Simon Glass <sjg@chromium.org>
Please see below.
>
> diff --git a/arch/arm/cpu/armv7/omap5/Makefile b/arch/arm/cpu/armv7/omap5/Makefile
> index 3caba86..d373bf4 100644
> --- a/arch/arm/cpu/armv7/omap5/Makefile
> +++ b/arch/arm/cpu/armv7/omap5/Makefile
> @@ -14,3 +14,4 @@ obj-y += hw_data.o
> obj-y += abb.o
> obj-y += fdt.o
> obj-$(CONFIG_IODELAY_RECALIBRATION) += dra7xx_iodelay.o
> +obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o
> diff --git a/arch/arm/cpu/armv7/omap5/sec_fxns.c b/arch/arm/cpu/armv7/omap5/sec_fxns.c
> new file mode 100644
> index 0000000..766333a
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/omap5/sec_fxns.c
> @@ -0,0 +1,70 @@
> +/*
> + *
> + * Common security functions that rely on secure ROM services
> + *
> + * (C) Copyright 2016
> + * Texas Instruments, <www.ti.com>
> + *
> + * Daniel Allred <d-allred@ti.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <asm/arch/sys_proto.h>
> +#include <asm/omap_common.h>
> +
> +#define SIGNATURE_LENGTH (0x118)
> +
> +/* API Index for OMAP5, DRA7xx */
> +#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX (0x0000000E)
> +
> +int secure_boot_verify_image(void **image, size_t *size)
> +{
> + int result = 1;
> + u32 cert_addr, sig_addr;
> + size_t cert_size;
> +
> +#ifndef CONFIG_SYS_DCACHE_OFF
> + /* Perform cache writeback on input buffer */
> + flush_dcache_range(
> + (u32)*image,
> + (u32)*image + roundup(*size, ARCH_DMA_MINALIGN));
> +#endif
> + cert_addr = (uint32_t)*image;
> + *size -= SIGNATURE_LENGTH; /* Subtract out the signature size */
> + cert_size = *size;
> + sig_addr = cert_addr + cert_size;
> +
> + /* Check if image load address is 32-bit aligned */
> + if (0 != (0x3 & cert_addr)) {
> + puts("Image is not 4-byte aligned.\n");
> + result = 1;
> + goto auth_exit;
> + }
> +
> + /* Image size also should be multiple of 4 */
> + if (0 != (0x3 & cert_size)) {
> + puts("Image size is not 4-byte aligned.\n");
> + result = 1;
> + goto auth_exit;
> + }
> +
> + /* Call ROM HAL API to verify certificate signature */
> + debug("%s: load_addr = %x, size = %x, sig_addr = %x\n", __func__,
> + cert_addr, cert_size, sig_addr);
> +
> + result = secure_rom_call(
> + API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX, 0, 0,
> + 4, cert_addr, cert_size, sig_addr, 0xFFFFFFFF);
> +auth_exit:
> + if (result != 0) {
> + puts("Authentication failed!\n");
> + printf("Return Value = %08X\n", result);
> + hang();
> + }
> +
> + printf("Authentication passed: %s\n", (char *)sig_addr);
> +
> + return result;
> +}
> diff --git a/arch/arm/include/asm/arch-omap5/sys_proto.h b/arch/arm/include/asm/arch-omap5/sys_proto.h
> index ab0e7fa..b175124 100644
> --- a/arch/arm/include/asm/arch-omap5/sys_proto.h
> +++ b/arch/arm/include/asm/arch-omap5/sys_proto.h
> @@ -84,4 +84,8 @@ static inline u32 usec_to_32k(u32 usec)
> #define OMAP5_SERVICE_L2ACTLR_SET 0x104
> #define OMAP5_SERVICE_ACR_SET 0x107
>
> +#ifdef CONFIG_TI_SECURE_DEVICE
> +int secure_boot_verify_image(void **p_image, size_t *p_size);
Function comment please.
> +#endif
> +
> #endif
> --
> 2.6.4
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 6/9] arm: omap5: add FIT image post process function
2016-06-15 19:26 ` [U-Boot] [RFC 6/9] arm: omap5: add FIT image post process function Andreas Dannenberg
@ 2016-06-17 3:52 ` Simon Glass
2016-06-17 4:26 ` Lokesh Vutla
1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2016-06-17 3:52 UTC (permalink / raw)
To: u-boot
On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> Adds a board specific FIT image post processing function for when
> CONFIG_SECURE_BOOT is defined. Also update the omap common config
> header to enable CONFIG_SECURE_BOOT always for secure TI devices
> (CONFIG_TI_SECURE_DEVICE is defined).
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> board/ti/am57xx/board.c | 7 +++++++
> board/ti/dra7xx/evm.c | 7 +++++++
> include/configs/ti_omap5_common.h | 4 ++++
> 3 files changed, 18 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 7/9] arm: am4x: add secure ROM signature verify API
2016-06-15 19:26 ` [U-Boot] [RFC 7/9] arm: am4x: add secure ROM signature verify API Andreas Dannenberg
@ 2016-06-17 3:52 ` Simon Glass
0 siblings, 0 replies; 29+ messages in thread
From: Simon Glass @ 2016-06-17 3:52 UTC (permalink / raw)
To: u-boot
Hi Andreas,
On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> From: Madan Srinivas <madans@ti.com>
>
> Adds an API that verifies a signature attached to an image (binary
> blob). This API is basically a entry to a secure ROM service provided by
> the device and accessed via an SMC call, using a particular calling
> convention. This API is common across AM3x HS and AM4x HS devices.
>
> Signed-off-by: Madan Srinivas <madans@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> arch/arm/cpu/armv7/am33xx/Makefile | 2 +
> arch/arm/cpu/armv7/am33xx/sec_fxns.c | 90 ++++++++++++++++++++++++++++
> arch/arm/include/asm/arch-am33xx/sys_proto.h | 6 +-
> 3 files changed, 97 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/cpu/armv7/am33xx/sec_fxns.c
>
> diff --git a/arch/arm/cpu/armv7/am33xx/Makefile b/arch/arm/cpu/armv7/am33xx/Makefile
> index 6fda482..d2b3e37 100644
> --- a/arch/arm/cpu/armv7/am33xx/Makefile
> +++ b/arch/arm/cpu/armv7/am33xx/Makefile
> @@ -20,3 +20,5 @@ obj-y += board.o
> obj-y += mux.o
>
> obj-$(CONFIG_CLOCK_SYNTHESIZER) += clk_synthesizer.o
> +
> +obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o
> diff --git a/arch/arm/cpu/armv7/am33xx/sec_fxns.c b/arch/arm/cpu/armv7/am33xx/sec_fxns.c
> new file mode 100644
> index 0000000..560297c
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/am33xx/sec_fxns.c
> @@ -0,0 +1,90 @@
> +/*
> + * sec_fxns.c
> + *
> + * Common security functions for AMxx devices that rely
> + * on secure ROM services.
> + *
> + * Copyright (C) 2013, Texas Instruments, Incorporated - http://www.ti.com/
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <asm/arch/sys_proto.h>
> +#include <asm/omap_common.h>
> +
> +/* Index for signature verify ROM API*/
> +#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX (0x0000000E)
> +
> +static u32 find_sig_start(char *image, size_t size)
> +{
> + char *image_end = image + size;
> + char *sig_start_magic = "CERT_";
> + int magic_str_len = strlen(sig_start_magic);
> + char *ch;
> +
> + while (--image_end > image) {
> + if (*image_end == '_') {
> + ch = image_end - magic_str_len + 1;
> + if (!strncmp(ch, sig_start_magic, magic_str_len))
> + return (u32)ch;
> + }
> + }
> + return 0;
> +}
> +
> +int secure_boot_verify_image(void **image, size_t *size)
> +{
> + int result = 1;
> + u32 cert_addr, sig_addr;
> + size_t cert_size;
> +
> +#ifndef CONFIG_SYS_DCACHE_OFF
> + /* Perform cache writeback on input buffer */
> + flush_dcache_range(
> + (u32)*image,
> + (u32)*image + roundup(*size, ARCH_DMA_MINALIGN));
> +#endif
> + cert_addr = (uint32_t)*image;
> + sig_addr = find_sig_start((char *)*image, *size);
> +
This seems similar to the code you added to arch/arm/cpu/armv7/omap5/sec_fxns.c.
Can you put the common code somewhere?
> + if (sig_addr == 0) {
> + puts("No signature found in image.\n");
> + result = 1;
> + goto auth_exit;
> + }
> +
> + *size = sig_addr - cert_addr; /* Subtract out the signature size */
> + cert_size = *size;
> +
> + /* Check if image load address is 32-bit aligned */
> + if (0 != (0x3 & cert_addr)) {
> + puts("Image is not 4-byte aligned.\n");
> + result = 1;
> + goto auth_exit;
> + }
> +
> + /* Image size also should be multiple of 4 */
> + if (0 != (0x3 & cert_size)) {
> + puts("Image size is not 4-byte aligned.\n");
> + result = 1;
> + goto auth_exit;
> + }
> +
> + /* Call ROM HAL API to verify certificate signature */
> + debug("%s: load_addr = %x, size = %x, sig_addr = %x\n", __func__,
> + cert_addr, cert_size, sig_addr);
> +
> + result = secure_rom_call(
> + API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX, 0, 0,
> + 4, cert_addr, cert_size, sig_addr, 0xFFFFFFFF);
> +auth_exit:
> + if (result != 0) {
> + puts("Authentication failed!\n");
Please use printf() instead of puts() unless you have a good reason.
> + printf("Return Value = %08X\n", result);
> + hang();
> + }
> +
> + printf("Authentication passed: %s\n", (char *)sig_addr);
> + return result;
> +}
> diff --git a/arch/arm/include/asm/arch-am33xx/sys_proto.h b/arch/arm/include/asm/arch-am33xx/sys_proto.h
> index 8f573d2..f5fc916 100644
> --- a/arch/arm/include/asm/arch-am33xx/sys_proto.h
> +++ b/arch/arm/include/asm/arch-am33xx/sys_proto.h
> @@ -41,7 +41,11 @@ void enable_norboot_pin_mux(void);
> void am33xx_spl_board_init(void);
> int am335x_get_efuse_mpu_max_freq(struct ctrl_dev *cdev);
> int am335x_get_tps65910_mpu_vdd(int sil_rev, int frequency);
> -#endif
>
> void enable_usb_clocks(int index);
> void disable_usb_clocks(int index);
> +
> +#ifdef CONFIG_TI_SECURE_DEVICE
> +int secure_boot_verify_image(void **p_image, size_t *p_size);
Please add function comment.
> +#endif
> +#endif
> --
> 2.6.4
>
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 8/9] arm: am4x: add FIT image post process function
2016-06-15 19:26 ` [U-Boot] [RFC 8/9] arm: am4x: add FIT image post process function Andreas Dannenberg
@ 2016-06-17 3:52 ` Simon Glass
2016-06-17 4:27 ` Lokesh Vutla
1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2016-06-17 3:52 UTC (permalink / raw)
To: u-boot
On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> From: Madan Srinivas <madans@ti.com>
>
> Adds a board specific FIT image post processing function when u-boot is
board-specific
> compiled for the high-secure (HS) device variant.
>
> Signed-off-by: Madan Srinivas <madans@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> board/ti/am43xx/board.c | 7 +++++++
> 1 file changed, 7 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT
2016-06-15 19:26 ` [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT Andreas Dannenberg
@ 2016-06-17 3:52 ` Simon Glass
2016-06-17 16:13 ` Andreas Dannenberg
0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2016-06-17 3:52 UTC (permalink / raw)
To: u-boot
Hi Andreas,
On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> Adds commands so that when a secure device is in use and the SPL is
> built to load a FIT image (with combined u-boot binary and various
> DTBs), these components that get fed into the FIT are all processed to
> be signed/encrypted/etc. as per the operations performed by the
> secure-binary-image script of the TI SECDEV package.
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++-
> arch/arm/cpu/armv7/omap5/config.mk | 3 ++
> 2 files changed, 58 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
But please can you add a README for this somewhere?
Also, can this tool be added to U-Boot instead of being external?
>
> diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk
> index c7bb101..c4514ad 100644
> --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk
> +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk
> @@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
> $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> else
> cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
> - $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
> - $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> + $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
> + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> endif
> else
> cmd_mkomapsecimg = echo "WARNING:" \
> @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
> "variable must be defined for TI secure devices. $@ was NOT created!"
> endif
>
> +ifdef CONFIG_SPL_LOAD_FIT
> +quiet_cmd_omapsecureimg = SECURE $@
> +ifneq ($(TI_SECURE_DEV_PKG),)
> +ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),)
> +cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \
> + $< $@ \
> + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> +else
> +cmd_omapsecureimg = echo "WARNING:" \
> + "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
> + "$@ was NOT created!"; cp $< $@
> +endif
> +else
> +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
> + "variable must be defined for TI secure devices." \
> + "$@ was NOT created!"; cp $< $@
> +endif
> +endif
> +
> +
> # Standard X-LOADER target (QPSI, NOR flash)
> u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin
> $(call if_changed,mkomapsecimg)
> @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin
> # the mkomapsecimg command looks for a u-boot-HS_* prefix
> u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin
> $(call if_changed,mkomapsecimg)
> +
> +# For supporting the SPL loading and interpreting
> +# of FIT images whose components are pre-processed
> +# before being integrated into the FIT image in order
> +# to secure them in some way
> +ifdef CONFIG_SPL_LOAD_FIT
> +
> +MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
> + -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> + -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
> + $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> +
> +OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> +$(OF_LIST_TARGETS): dtbs
> +
> +%_HS.dtb: %.dtb
> + $(call if_changed,omapsecureimg)
> + $(Q)if [ -f $@ ]; then \
> + cp -f $@ $<; \
> + fi
> +
> +u-boot-nodtb_HS.bin: u-boot-nodtb.bin
> + $(call if_changed,omapsecureimg)
> +
> +u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))
> + $(call if_changed,mkimage)
> + $(Q)if [ -f $@ ]; then \
> + cp -f $@ u-boot.img; \
> + fi
> +
> +.NOTPARALLEL: dtbs
Why is that needed?
> +
> +endif
> diff --git a/arch/arm/cpu/armv7/omap5/config.mk b/arch/arm/cpu/armv7/omap5/config.mk
> index a7e55a5..503f31c 100644
> --- a/arch/arm/cpu/armv7/omap5/config.mk
> +++ b/arch/arm/cpu/armv7/omap5/config.mk
> @@ -15,5 +15,8 @@ else
> ALL-y += MLO
> endif
> else
> +ifeq ($(CONFIG_TI_SECURE_DEVICE)$(CONFIG_SECURE_BOOT),yy)
> +ALL-y += u-boot_HS.img
> +endif
> ALL-y += u-boot.img
> endif
> --
> 2.6.4
>
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 4/9] arm: omap-common: add secure rom call API for secure devices
2016-06-15 19:26 ` [U-Boot] [RFC 4/9] arm: omap-common: add secure rom call API for secure devices Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
@ 2016-06-17 4:18 ` Lokesh Vutla
1 sibling, 0 replies; 29+ messages in thread
From: Lokesh Vutla @ 2016-06-17 4:18 UTC (permalink / raw)
To: u-boot
On Thursday 16 June 2016 12:56 AM, Andreas Dannenberg wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> Adds a generic C-callable API for making secure ROM calls on OMAP and
> OMAP-compatible devices. This API provides the important function of
> flushing the ROM call arguments to memory from the cache, so that the
> secure world will have a coherent view of those arguments. Then is
> simply calls the omap_smc_sec routine.
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> arch/arm/cpu/armv7/omap-common/Makefile | 4 +++
> arch/arm/cpu/armv7/omap-common/sec_bridge.c | 47 +++++++++++++++++++++++++++++
> arch/arm/include/asm/omap_common.h | 4 +++
> 3 files changed, 55 insertions(+)
> create mode 100644 arch/arm/cpu/armv7/omap-common/sec_bridge.c
>
> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile
> index 87a7ac0..4fc3926 100644
> --- a/arch/arm/cpu/armv7/omap-common/Makefile
> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
> @@ -28,6 +28,10 @@ obj-y += pipe3-phy.o
> obj-$(CONFIG_SCSI_AHCI_PLAT) += sata.o
> endif
>
> +ifneq ($(CONFIG_TI_SECURE_DEVICE),)
> +obj-y += sec_bridge.o
> +endif
can we use: obj-$(CONFIG_TI_SECURE_DEVICE) += sec_bridge.o ?
> +
> ifeq ($(CONFIG_SYS_DCACHE_OFF),)
> obj-y += omap-cache.o
> endif
> diff --git a/arch/arm/cpu/armv7/omap-common/sec_bridge.c b/arch/arm/cpu/armv7/omap-common/sec_bridge.c
> new file mode 100644
> index 0000000..4eaba8e
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/omap-common/sec_bridge.c
> @@ -0,0 +1,47 @@
> +/*
> + *
> + * Common bridge function to make OMAP secure ROM calls
> + *
> + * (C) Copyright 2016
> + * Texas Instruments, <www.ti.com>
> + *
> + * Daniel Allred <d-allred@ti.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <stdarg.h>
> +
> +#include <asm/arch/sys_proto.h>
> +#include <asm/omap_common.h>
> +
> +static uint32_t secure_rom_call_args[5] __aligned(ARCH_DMA_MINALIGN);
> +
> +u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...)
> +{
> + int i;
> + u32 num_args;
> + va_list ap;
> +
> + va_start(ap, flag);
> +
> + num_args = va_arg(ap, u32);
> +
Is there a cap on the num_args? can you add a check for that?
> + /* Copy args to aligned args structure */
> + for (i = 0; i < num_args; i++)
> + secure_rom_call_args[i + 1] = va_arg(ap, u32);
> +
> + secure_rom_call_args[0] = num_args;
> +
> + va_end(ap);
> +
> + /* if data cache is enabled, flush the aligned args structure */
> +#ifndef CONFIG_SYS_DCACHE_OFF
> + flush_dcache_range(
> + (unsigned int)&secure_rom_call_args[0],
> + (unsigned int)&secure_rom_call_args[0] +
> + roundup(sizeof(secure_rom_call_args), ARCH_DMA_MINALIGN));
> +#endif
I guess you do not need #ifndef here. Patch 2 should take care of it.
Thanks and regards,
Lokesh
> + return omap_smc_sec(service, proc_id, flag, secure_rom_call_args);
> +}
> diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
> index 5943e6f..cb02c88 100644
> --- a/arch/arm/include/asm/omap_common.h
> +++ b/arch/arm/include/asm/omap_common.h
> @@ -629,6 +629,10 @@ void omap_smc1(u32 service, u32 val);
>
> u32 omap_smc_sec(u32 service, u32 proc_id, u32 flag, u32 *params);
>
> +#ifdef CONFIG_TI_SECURE_DEVICE
> +u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...);
> +#endif
> +
> void enable_edma3_clocks(void);
> void disable_edma3_clocks(void);
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 6/9] arm: omap5: add FIT image post process function
2016-06-15 19:26 ` [U-Boot] [RFC 6/9] arm: omap5: add FIT image post process function Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
@ 2016-06-17 4:26 ` Lokesh Vutla
1 sibling, 0 replies; 29+ messages in thread
From: Lokesh Vutla @ 2016-06-17 4:26 UTC (permalink / raw)
To: u-boot
On Thursday 16 June 2016 12:56 AM, Andreas Dannenberg wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> Adds a board specific FIT image post processing function for when
> CONFIG_SECURE_BOOT is defined. Also update the omap common config
> header to enable CONFIG_SECURE_BOOT always for secure TI devices
> (CONFIG_TI_SECURE_DEVICE is defined).
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> board/ti/am57xx/board.c | 7 +++++++
> board/ti/dra7xx/evm.c | 7 +++++++
> include/configs/ti_omap5_common.h | 4 ++++
> 3 files changed, 18 insertions(+)
>
> diff --git a/board/ti/am57xx/board.c b/board/ti/am57xx/board.c
> index 08cf14d..a9635c2 100644
> --- a/board/ti/am57xx/board.c
> +++ b/board/ti/am57xx/board.c
> @@ -750,3 +750,10 @@ int board_fit_config_name_match(const char *name)
> return -1;
> }
> #endif
> +
> +#ifdef CONFIG_SECURE_BOOT
> +void board_fit_image_post_process(void **p_image, size_t *p_size)
> +{
> + secure_boot_verify_image(p_image, p_size);
> +}
> +#endif
> diff --git a/board/ti/dra7xx/evm.c b/board/ti/dra7xx/evm.c
> index 3fbbc9b..03eefb6 100644
> --- a/board/ti/dra7xx/evm.c
> +++ b/board/ti/dra7xx/evm.c
> @@ -739,3 +739,10 @@ int board_fit_config_name_match(const char *name)
> return -1;
> }
> #endif
> +
> +#ifdef CONFIG_SECURE_BOOT
> +void board_fit_image_post_process(void **p_image, size_t *p_size)
> +{
> + secure_boot_verify_image(p_image, p_size);
> +}
> +#endif
> diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h
> index 2e4c8e9..9db6da2 100644
> --- a/include/configs/ti_omap5_common.h
> +++ b/include/configs/ti_omap5_common.h
> @@ -138,6 +138,10 @@
> * print some information.
> */
> #ifdef CONFIG_TI_SECURE_DEVICE
> +
> +/* Always enforce for secure devices */
> +#define CONFIG_SECURE_BOOT
Can you make this a Kconfig option?
You are enabling it for GP devices as well. What happens in GP devices?
Thanks and regards,
Lokesh
> +
> /*
> * For memory booting on HS parts, the first 4KB of the internal RAM is
> * reserved for secure world use and the flash loader image is
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 8/9] arm: am4x: add FIT image post process function
2016-06-15 19:26 ` [U-Boot] [RFC 8/9] arm: am4x: add FIT image post process function Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
@ 2016-06-17 4:27 ` Lokesh Vutla
1 sibling, 0 replies; 29+ messages in thread
From: Lokesh Vutla @ 2016-06-17 4:27 UTC (permalink / raw)
To: u-boot
On Thursday 16 June 2016 12:56 AM, Andreas Dannenberg wrote:
> From: Madan Srinivas <madans@ti.com>
>
> Adds a board specific FIT image post processing function when u-boot is
> compiled for the high-secure (HS) device variant.
>
> Signed-off-by: Madan Srinivas <madans@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> board/ti/am43xx/board.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/board/ti/am43xx/board.c b/board/ti/am43xx/board.c
> index f005762..fc0b38b 100644
> --- a/board/ti/am43xx/board.c
> +++ b/board/ti/am43xx/board.c
> @@ -862,3 +862,10 @@ int board_fit_config_name_match(const char *name)
> return -1;
> }
> #endif
> +
> +#ifdef CONFIG_TI_SECURE_DEVICE
For dra7 platforms CONFIG_SECURE_BOOT is being used to define this
function. Can this be aligned for all platforms?
Thanks and regards,
Lokesh
> +void board_fit_image_post_process(void **p_image, size_t *p_size)
> +{
> + secure_boot_verify_image(p_image, p_size);
> +}
> +#endif
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT
2016-06-17 3:52 ` Simon Glass
@ 2016-06-17 16:13 ` Andreas Dannenberg
2016-06-20 22:40 ` Simon Glass
0 siblings, 1 reply; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-17 16:13 UTC (permalink / raw)
To: u-boot
Hi Simon,
many thanks for your feedback on the series... I know it takes a lot of
effort to digest all that stuff. We'll see how we can tackle the
feedback one by one and send out an updated series.
Regarding this patch, please see below...
On Thu, Jun 16, 2016 at 09:52:52PM -0600, Simon Glass wrote:
> Hi Andreas,
>
> On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> > From: Daniel Allred <d-allred@ti.com>
> >
> > Adds commands so that when a secure device is in use and the SPL is
> > built to load a FIT image (with combined u-boot binary and various
> > DTBs), these components that get fed into the FIT are all processed to
> > be signed/encrypted/etc. as per the operations performed by the
> > secure-binary-image script of the TI SECDEV package.
> >
> > Signed-off-by: Daniel Allred <d-allred@ti.com>
> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > ---
> > arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++-
> > arch/arm/cpu/armv7/omap5/config.mk | 3 ++
> > 2 files changed, 58 insertions(+), 2 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But please can you add a README for this somewhere?
>
> Also, can this tool be added to U-Boot instead of being external?
Yes we will definitely enhance the readme in the PATCH version, this
wasn't quite ready yet by the time we submitted the RFC.
Also regarding the external singing/encryption tool this is only
available from TI under NDA after customers engage with TI just like the
high-secure (HS) devices themselves (you can't just buy them on
Digi-Key). Personally I hope that we eventually lower this barrier of
entry (and this has been brought up more than once) but as many things
in the corporate world there is a complex decision process behind it. So
until this strategy changes (if ever) our poor developer's hands are
tied. All we can do for now is trying to allow this external tool to get
hooked into the U-Boot build process in the easiest way possible which
is already done today by way of the $TI_SECURE_DEV_PKG environmental
variable during SPL build.
> >
> > diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk
> > index c7bb101..c4514ad 100644
> > --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk
> > +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk
> > @@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
> > $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> > else
> > cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
> > - $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
> > - $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> > + $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> > endif
> > else
> > cmd_mkomapsecimg = echo "WARNING:" \
> > @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
> > "variable must be defined for TI secure devices. $@ was NOT created!"
> > endif
> >
> > +ifdef CONFIG_SPL_LOAD_FIT
> > +quiet_cmd_omapsecureimg = SECURE $@
> > +ifneq ($(TI_SECURE_DEV_PKG),)
> > +ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),)
> > +cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \
> > + $< $@ \
> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> > +else
> > +cmd_omapsecureimg = echo "WARNING:" \
> > + "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
> > + "$@ was NOT created!"; cp $< $@
> > +endif
> > +else
> > +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
> > + "variable must be defined for TI secure devices." \
> > + "$@ was NOT created!"; cp $< $@
> > +endif
> > +endif
> > +
> > +
> > # Standard X-LOADER target (QPSI, NOR flash)
> > u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin
> > $(call if_changed,mkomapsecimg)
> > @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin
> > # the mkomapsecimg command looks for a u-boot-HS_* prefix
> > u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin
> > $(call if_changed,mkomapsecimg)
> > +
> > +# For supporting the SPL loading and interpreting
> > +# of FIT images whose components are pre-processed
> > +# before being integrated into the FIT image in order
> > +# to secure them in some way
> > +ifdef CONFIG_SPL_LOAD_FIT
> > +
> > +MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
> > + -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> > + -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
> > + $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> > +
> > +OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> > +$(OF_LIST_TARGETS): dtbs
> > +
> > +%_HS.dtb: %.dtb
> > + $(call if_changed,omapsecureimg)
> > + $(Q)if [ -f $@ ]; then \
> > + cp -f $@ $<; \
> > + fi
> > +
> > +u-boot-nodtb_HS.bin: u-boot-nodtb.bin
> > + $(call if_changed,omapsecureimg)
> > +
> > +u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))
> > + $(call if_changed,mkimage)
> > + $(Q)if [ -f $@ ]; then \
> > + cp -f $@ u-boot.img; \
> > + fi
> > +
> > +.NOTPARALLEL: dtbs
>
> Why is that needed?
Good catch! This issue actually caused me a fair amount of grief when
running into it. During development we found the build process works
with make -j1 but would fail for parallel builds (like make -j8) with
the following error....
----------------------->8---------------------------
$ make mrproper
$ make dra7xx_hs_evm_defconfig
$ make -j8
....
LD u-boot
OBJCOPY u-boot-nodtb.bin
OBJCOPY u-boot.srec
SYM u-boot.sym
DTC arch/arm/dts/dra72-evm.dtb
DTC arch/arm/dts/dra7-evm.dtb
DTC arch/arm/dts/dra72-evm.dtb
DTC arch/arm/dts/dra7-evm.dtb
Error: arch/arm/dts/dra7-evm.dts:492.15-16 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [arch/arm/dts/dra7-evm.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
Error: arch/arm/dts/dra72-evm.dts:149.26-150.2 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [arch/arm/dts/dra72-evm.dtb] Error 1
make[1]: *** [arch-dtbs] Error 2
make: *** [dtbs] Error 2
make: *** Waiting for unfinished jobs....
SHIPPED dts/dt.dtb
----------------------->8---------------------------
However with the .NOTPARALLEL: dtbs line included it works. Also when
doing make a second time it works as well (clearly that's not a
solution:)
On my quest trying to digest/root cause this I concluded that this may
be caused by the DTB files somehow getting compiled twice (see snippet).
This in combination with the fact that we post-process files like
dta7-evm.dtb (signing them) and creating new DTB files with the same
name (like dra7-evm.dtb) replacing the old ones causes issues during the
DTB compile step.
Now I'm not a make guru but I tried to clean up the dependency chain as
good as I can but could not get it to the point where each DTB file is
only build once which most likely would allow us getting rid of
NOPARALLEL.
On a related note we wanted to keep the same names for the DTB files
(rather than turning dra7-evm.dtb into dra7-evm-sec.dtb for example, as
I had it initially) since it's important as they get bundled into the
u-boot.img FIT blob so that they can be later matched by the SPL to a
board. But it appears like that trying to keep the same names despite
the additional signing step seems to complicate things from a make
perspective.
If you or anybody else has any smart suggestion how to address this in
a better way I'd love to hear about it.
Thanks,
Andreas
> > +
> > +endif
> > diff --git a/arch/arm/cpu/armv7/omap5/config.mk b/arch/arm/cpu/armv7/omap5/config.mk
> > index a7e55a5..503f31c 100644
> > --- a/arch/arm/cpu/armv7/omap5/config.mk
> > +++ b/arch/arm/cpu/armv7/omap5/config.mk
> > @@ -15,5 +15,8 @@ else
> > ALL-y += MLO
> > endif
> > else
> > +ifeq ($(CONFIG_TI_SECURE_DEVICE)$(CONFIG_SECURE_BOOT),yy)
> > +ALL-y += u-boot_HS.img
> > +endif
> > ALL-y += u-boot.img
> > endif
> > --
> > 2.6.4
> >
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 2/9] arm: cache: add missing dummy functions for when dcache disabled
2016-06-15 19:26 ` [U-Boot] [RFC 2/9] arm: cache: add missing dummy functions for when dcache disabled Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
@ 2016-06-20 2:13 ` Tom Rini
1 sibling, 0 replies; 29+ messages in thread
From: Tom Rini @ 2016-06-20 2:13 UTC (permalink / raw)
To: u-boot
On Wed, Jun 15, 2016 at 02:26:34PM -0500, Andreas Dannenberg wrote:
> From: Daniel Allred <d-allred@ti.com>
>
> Adds missing flush_dcache_range and invalidate_dcache_range dummy
> (empty) placeholder functions to the #else portion of the #ifndef
> CONFIG_SYS_DCACHE_OFF, where full implementations of these functions
> are defined.
>
> Signed-off-by: Daniel Allred <d-allred@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160619/50d47745/attachment.sig>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 5/9] arm: omap5: add secure ROM signature verify API
2016-06-17 3:52 ` Simon Glass
@ 2016-06-20 2:13 ` Tom Rini
0 siblings, 0 replies; 29+ messages in thread
From: Tom Rini @ 2016-06-20 2:13 UTC (permalink / raw)
To: u-boot
On Thu, Jun 16, 2016 at 09:52:40PM -0600, Simon Glass wrote:
> On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> > From: Daniel Allred <d-allred@ti.com>
> >
> > Adds an API that verifies a signature attached to an image (binary
> > blob). This API is basically a entry to a secure ROM service provided by
> > the device and accessed via an SMC call, using a particular calling
> > convention.
> >
> > Signed-off-by: Daniel Allred <d-allred@ti.com>
> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > ---
> > arch/arm/cpu/armv7/omap5/Makefile | 1 +
> > arch/arm/cpu/armv7/omap5/sec_fxns.c | 70 +++++++++++++++++++++++++++++
> > arch/arm/include/asm/arch-omap5/sys_proto.h | 4 ++
> > 3 files changed, 75 insertions(+)
> > create mode 100644 arch/arm/cpu/armv7/omap5/sec_fxns.c
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Please see below.
>
> >
> > diff --git a/arch/arm/cpu/armv7/omap5/Makefile b/arch/arm/cpu/armv7/omap5/Makefile
> > index 3caba86..d373bf4 100644
> > --- a/arch/arm/cpu/armv7/omap5/Makefile
> > +++ b/arch/arm/cpu/armv7/omap5/Makefile
> > @@ -14,3 +14,4 @@ obj-y += hw_data.o
> > obj-y += abb.o
> > obj-y += fdt.o
> > obj-$(CONFIG_IODELAY_RECALIBRATION) += dra7xx_iodelay.o
> > +obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o
> > diff --git a/arch/arm/cpu/armv7/omap5/sec_fxns.c b/arch/arm/cpu/armv7/omap5/sec_fxns.c
> > new file mode 100644
> > index 0000000..766333a
> > --- /dev/null
> > +++ b/arch/arm/cpu/armv7/omap5/sec_fxns.c
> > @@ -0,0 +1,70 @@
> > +/*
> > + *
> > + * Common security functions that rely on secure ROM services
> > + *
> > + * (C) Copyright 2016
> > + * Texas Instruments, <www.ti.com>
> > + *
> > + * Daniel Allred <d-allred@ti.com>
> > + *
> > + * SPDX-License-Identifier: GPL-2.0+
> > + */
> > +
> > +#include <common.h>
> > +#include <asm/arch/sys_proto.h>
> > +#include <asm/omap_common.h>
> > +
> > +#define SIGNATURE_LENGTH (0x118)
> > +
> > +/* API Index for OMAP5, DRA7xx */
> > +#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX (0x0000000E)
> > +
> > +int secure_boot_verify_image(void **image, size_t *size)
> > +{
> > + int result = 1;
> > + u32 cert_addr, sig_addr;
> > + size_t cert_size;
> > +
> > +#ifndef CONFIG_SYS_DCACHE_OFF
> > + /* Perform cache writeback on input buffer */
> > + flush_dcache_range(
> > + (u32)*image,
> > + (u32)*image + roundup(*size, ARCH_DMA_MINALIGN));
> > +#endif
> > + cert_addr = (uint32_t)*image;
> > + *size -= SIGNATURE_LENGTH; /* Subtract out the signature size */
> > + cert_size = *size;
> > + sig_addr = cert_addr + cert_size;
> > +
> > + /* Check if image load address is 32-bit aligned */
> > + if (0 != (0x3 & cert_addr)) {
> > + puts("Image is not 4-byte aligned.\n");
> > + result = 1;
> > + goto auth_exit;
> > + }
> > +
> > + /* Image size also should be multiple of 4 */
> > + if (0 != (0x3 & cert_size)) {
> > + puts("Image size is not 4-byte aligned.\n");
> > + result = 1;
> > + goto auth_exit;
> > + }
> > +
> > + /* Call ROM HAL API to verify certificate signature */
> > + debug("%s: load_addr = %x, size = %x, sig_addr = %x\n", __func__,
> > + cert_addr, cert_size, sig_addr);
> > +
> > + result = secure_rom_call(
> > + API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX, 0, 0,
> > + 4, cert_addr, cert_size, sig_addr, 0xFFFFFFFF);
> > +auth_exit:
> > + if (result != 0) {
> > + puts("Authentication failed!\n");
> > + printf("Return Value = %08X\n", result);
> > + hang();
> > + }
> > +
> > + printf("Authentication passed: %s\n", (char *)sig_addr);
> > +
> > + return result;
> > +}
> > diff --git a/arch/arm/include/asm/arch-omap5/sys_proto.h b/arch/arm/include/asm/arch-omap5/sys_proto.h
> > index ab0e7fa..b175124 100644
> > --- a/arch/arm/include/asm/arch-omap5/sys_proto.h
> > +++ b/arch/arm/include/asm/arch-omap5/sys_proto.h
> > @@ -84,4 +84,8 @@ static inline u32 usec_to_32k(u32 usec)
> > #define OMAP5_SERVICE_L2ACTLR_SET 0x104
> > #define OMAP5_SERVICE_ACR_SET 0x107
> >
> > +#ifdef CONFIG_TI_SECURE_DEVICE
> > +int secure_boot_verify_image(void **p_image, size_t *p_size);
>
> Function comment please.
And no ifdef/endif (here and later in the series when adding other
calls), thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160619/1f6a8fb5/attachment.sig>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT
2016-06-17 16:13 ` Andreas Dannenberg
@ 2016-06-20 22:40 ` Simon Glass
2016-06-21 2:35 ` Andreas Dannenberg
0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2016-06-20 22:40 UTC (permalink / raw)
To: u-boot
+Masahiro for the Makefile question
Hi Andreas,
On 17 June 2016 at 10:13, Andreas Dannenberg <dannenberg@ti.com> wrote:
> Hi Simon,
> many thanks for your feedback on the series... I know it takes a lot of
> effort to digest all that stuff. We'll see how we can tackle the
> feedback one by one and send out an updated series.
>
> Regarding this patch, please see below...
>
> On Thu, Jun 16, 2016 at 09:52:52PM -0600, Simon Glass wrote:
>> Hi Andreas,
>>
>> On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
>> > From: Daniel Allred <d-allred@ti.com>
>> >
>> > Adds commands so that when a secure device is in use and the SPL is
>> > built to load a FIT image (with combined u-boot binary and various
>> > DTBs), these components that get fed into the FIT are all processed to
>> > be signed/encrypted/etc. as per the operations performed by the
>> > secure-binary-image script of the TI SECDEV package.
>> >
>> > Signed-off-by: Daniel Allred <d-allred@ti.com>
>> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>> > ---
>> > arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++-
>> > arch/arm/cpu/armv7/omap5/config.mk | 3 ++
>> > 2 files changed, 58 insertions(+), 2 deletions(-)
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> But please can you add a README for this somewhere?
>>
>> Also, can this tool be added to U-Boot instead of being external?
>
> Yes we will definitely enhance the readme in the PATCH version, this
> wasn't quite ready yet by the time we submitted the RFC.
>
> Also regarding the external singing/encryption tool this is only
> available from TI under NDA after customers engage with TI just like the
> high-secure (HS) devices themselves (you can't just buy them on
> Digi-Key). Personally I hope that we eventually lower this barrier of
> entry (and this has been brought up more than once) but as many things
> in the corporate world there is a complex decision process behind it. So
> until this strategy changes (if ever) our poor developer's hands are
> tied. All we can do for now is trying to allow this external tool to get
> hooked into the U-Boot build process in the easiest way possible which
> is already done today by way of the $TI_SECURE_DEV_PKG environmental
> variable during SPL build.
OK. That's odd because it should be the keys that are secure, not the
tool! If anything, the tool should have as many eyes on it as
possible.
>
>> >
>> > diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk
>> > index c7bb101..c4514ad 100644
>> > --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk
>> > +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk
>> > @@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
>> > $(if $(KBUILD_VERBOSE:1=), >/dev/null)
>> > else
>> > cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
>> > - $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
>> > - $(if $(KBUILD_VERBOSE:1=), >/dev/null)
>> > + $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
>> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
>> > endif
>> > else
>> > cmd_mkomapsecimg = echo "WARNING:" \
>> > @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
>> > "variable must be defined for TI secure devices. $@ was NOT created!"
>> > endif
>> >
>> > +ifdef CONFIG_SPL_LOAD_FIT
>> > +quiet_cmd_omapsecureimg = SECURE $@
>> > +ifneq ($(TI_SECURE_DEV_PKG),)
>> > +ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),)
>> > +cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \
>> > + $< $@ \
>> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
>> > +else
>> > +cmd_omapsecureimg = echo "WARNING:" \
>> > + "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
>> > + "$@ was NOT created!"; cp $< $@
>> > +endif
>> > +else
>> > +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
>> > + "variable must be defined for TI secure devices." \
>> > + "$@ was NOT created!"; cp $< $@
>> > +endif
>> > +endif
>> > +
>> > +
>> > # Standard X-LOADER target (QPSI, NOR flash)
>> > u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin
>> > $(call if_changed,mkomapsecimg)
>> > @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin
>> > # the mkomapsecimg command looks for a u-boot-HS_* prefix
>> > u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin
>> > $(call if_changed,mkomapsecimg)
>> > +
>> > +# For supporting the SPL loading and interpreting
>> > +# of FIT images whose components are pre-processed
>> > +# before being integrated into the FIT image in order
>> > +# to secure them in some way
>> > +ifdef CONFIG_SPL_LOAD_FIT
>> > +
>> > +MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
>> > + -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
>> > + -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
>> > + $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
>> > +
>> > +OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
>> > +$(OF_LIST_TARGETS): dtbs
>> > +
>> > +%_HS.dtb: %.dtb
>> > + $(call if_changed,omapsecureimg)
>> > + $(Q)if [ -f $@ ]; then \
>> > + cp -f $@ $<; \
>> > + fi
>> > +
>> > +u-boot-nodtb_HS.bin: u-boot-nodtb.bin
>> > + $(call if_changed,omapsecureimg)
>> > +
>> > +u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))
>> > + $(call if_changed,mkimage)
>> > + $(Q)if [ -f $@ ]; then \
>> > + cp -f $@ u-boot.img; \
>> > + fi
>> > +
>> > +.NOTPARALLEL: dtbs
>>
>> Why is that needed?
>
> Good catch! This issue actually caused me a fair amount of grief when
> running into it. During development we found the build process works
> with make -j1 but would fail for parallel builds (like make -j8) with
> the following error....
>
> ----------------------->8---------------------------
> $ make mrproper
> $ make dra7xx_hs_evm_defconfig
> $ make -j8
> ....
> LD u-boot
> OBJCOPY u-boot-nodtb.bin
> OBJCOPY u-boot.srec
> SYM u-boot.sym
> DTC arch/arm/dts/dra72-evm.dtb
> DTC arch/arm/dts/dra7-evm.dtb
> DTC arch/arm/dts/dra72-evm.dtb
> DTC arch/arm/dts/dra7-evm.dtb
> Error: arch/arm/dts/dra7-evm.dts:492.15-16 syntax error
> FATAL ERROR: Unable to parse input tree
> make[2]: *** [arch/arm/dts/dra7-evm.dtb] Error 1
> make[2]: *** Waiting for unfinished jobs....
> Error: arch/arm/dts/dra72-evm.dts:149.26-150.2 syntax error
> FATAL ERROR: Unable to parse input tree
> make[2]: *** [arch/arm/dts/dra72-evm.dtb] Error 1
> make[1]: *** [arch-dtbs] Error 2
> make: *** [dtbs] Error 2
> make: *** Waiting for unfinished jobs....
> SHIPPED dts/dt.dtb
> ----------------------->8---------------------------
Probably you are missing a dependency somewhere - but the failure to
parse is odd. What is it missing?
>
> However with the .NOTPARALLEL: dtbs line included it works. Also when
> doing make a second time it works as well (clearly that's not a
> solution:)
>
> On my quest trying to digest/root cause this I concluded that this may
> be caused by the DTB files somehow getting compiled twice (see snippet).
> This in combination with the fact that we post-process files like
> dta7-evm.dtb (signing them) and creating new DTB files with the same
> name (like dra7-evm.dtb) replacing the old ones causes issues during the
> DTB compile step.
Yes I think it is better to create new files, since otherwise the
build system can't determine which version of the file it should
target...it assumes a single version as I understand it.
>
> Now I'm not a make guru but I tried to clean up the dependency chain as
> good as I can but could not get it to the point where each DTB file is
> only build once which most likely would allow us getting rid of
> NOPARALLEL.
>
> On a related note we wanted to keep the same names for the DTB files
> (rather than turning dra7-evm.dtb into dra7-evm-sec.dtb for example, as
> I had it initially) since it's important as they get bundled into the
> u-boot.img FIT blob so that they can be later matched by the SPL to a
> board. But it appears like that trying to keep the same names despite
> the additional signing step seems to complicate things from a make
> perspective.
Yes - best to create a new u-boot-sec.img I think.
>
> If you or anybody else has any smart suggestion how to address this in
> a better way I'd love to hear about it.
Masahiro is the expert, but my ideas are above.
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT
2016-06-20 22:40 ` Simon Glass
@ 2016-06-21 2:35 ` Andreas Dannenberg
2016-06-23 4:59 ` Masahiro Yamada
0 siblings, 1 reply; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-21 2:35 UTC (permalink / raw)
To: u-boot
Hi Simon,
thanks for the continued feedback. Comments below...
On Mon, Jun 20, 2016 at 04:40:10PM -0600, Simon Glass wrote:
> +Masahiro for the Makefile question
>
> Hi Andreas,
>
> On 17 June 2016 at 10:13, Andreas Dannenberg <dannenberg@ti.com> wrote:
> > Hi Simon,
> > many thanks for your feedback on the series... I know it takes a lot of
> > effort to digest all that stuff. We'll see how we can tackle the
> > feedback one by one and send out an updated series.
> >
> > Regarding this patch, please see below...
> >
> > On Thu, Jun 16, 2016 at 09:52:52PM -0600, Simon Glass wrote:
> >> Hi Andreas,
> >>
> >> On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> >> > From: Daniel Allred <d-allred@ti.com>
> >> >
> >> > Adds commands so that when a secure device is in use and the SPL is
> >> > built to load a FIT image (with combined u-boot binary and various
> >> > DTBs), these components that get fed into the FIT are all processed to
> >> > be signed/encrypted/etc. as per the operations performed by the
> >> > secure-binary-image script of the TI SECDEV package.
> >> >
> >> > Signed-off-by: Daniel Allred <d-allred@ti.com>
> >> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >> > ---
> >> > arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++-
> >> > arch/arm/cpu/armv7/omap5/config.mk | 3 ++
> >> > 2 files changed, 58 insertions(+), 2 deletions(-)
> >>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>
> >> But please can you add a README for this somewhere?
> >>
> >> Also, can this tool be added to U-Boot instead of being external?
> >
> > Yes we will definitely enhance the readme in the PATCH version, this
> > wasn't quite ready yet by the time we submitted the RFC.
> >
> > Also regarding the external singing/encryption tool this is only
> > available from TI under NDA after customers engage with TI just like the
> > high-secure (HS) devices themselves (you can't just buy them on
> > Digi-Key). Personally I hope that we eventually lower this barrier of
> > entry (and this has been brought up more than once) but as many things
> > in the corporate world there is a complex decision process behind it. So
> > until this strategy changes (if ever) our poor developer's hands are
> > tied. All we can do for now is trying to allow this external tool to get
> > hooked into the U-Boot build process in the easiest way possible which
> > is already done today by way of the $TI_SECURE_DEV_PKG environmental
> > variable during SPL build.
>
> OK. That's odd because it should be the keys that are secure, not the
> tool! If anything, the tool should have as many eyes on it as
> possible.
Yes that's the ideal-world case I was also arguing we should follow...
But as indicated decision processes in the corporate world sometimes are
complex :)
> >> >
> >> > diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk
> >> > index c7bb101..c4514ad 100644
> >> > --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk
> >> > +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk
> >> > @@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
> >> > $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> >> > else
> >> > cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
> >> > - $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
> >> > - $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> >> > + $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
> >> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> >> > endif
> >> > else
> >> > cmd_mkomapsecimg = echo "WARNING:" \
> >> > @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
> >> > "variable must be defined for TI secure devices. $@ was NOT created!"
> >> > endif
> >> >
> >> > +ifdef CONFIG_SPL_LOAD_FIT
> >> > +quiet_cmd_omapsecureimg = SECURE $@
> >> > +ifneq ($(TI_SECURE_DEV_PKG),)
> >> > +ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),)
> >> > +cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \
> >> > + $< $@ \
> >> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> >> > +else
> >> > +cmd_omapsecureimg = echo "WARNING:" \
> >> > + "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
> >> > + "$@ was NOT created!"; cp $< $@
> >> > +endif
> >> > +else
> >> > +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
> >> > + "variable must be defined for TI secure devices." \
> >> > + "$@ was NOT created!"; cp $< $@
> >> > +endif
> >> > +endif
> >> > +
> >> > +
> >> > # Standard X-LOADER target (QPSI, NOR flash)
> >> > u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin
> >> > $(call if_changed,mkomapsecimg)
> >> > @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin
> >> > # the mkomapsecimg command looks for a u-boot-HS_* prefix
> >> > u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin
> >> > $(call if_changed,mkomapsecimg)
> >> > +
> >> > +# For supporting the SPL loading and interpreting
> >> > +# of FIT images whose components are pre-processed
> >> > +# before being integrated into the FIT image in order
> >> > +# to secure them in some way
> >> > +ifdef CONFIG_SPL_LOAD_FIT
> >> > +
> >> > +MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
> >> > + -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> >> > + -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
> >> > + $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> >> > +
> >> > +OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> >> > +$(OF_LIST_TARGETS): dtbs
> >> > +
> >> > +%_HS.dtb: %.dtb
> >> > + $(call if_changed,omapsecureimg)
> >> > + $(Q)if [ -f $@ ]; then \
> >> > + cp -f $@ $<; \
> >> > + fi
> >> > +
> >> > +u-boot-nodtb_HS.bin: u-boot-nodtb.bin
> >> > + $(call if_changed,omapsecureimg)
> >> > +
> >> > +u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))
> >> > + $(call if_changed,mkimage)
> >> > + $(Q)if [ -f $@ ]; then \
> >> > + cp -f $@ u-boot.img; \
> >> > + fi
> >> > +
> >> > +.NOTPARALLEL: dtbs
> >>
> >> Why is that needed?
> >
> > Good catch! This issue actually caused me a fair amount of grief when
> > running into it. During development we found the build process works
> > with make -j1 but would fail for parallel builds (like make -j8) with
> > the following error....
> >
> > ----------------------->8---------------------------
> > $ make mrproper
> > $ make dra7xx_hs_evm_defconfig
> > $ make -j8
> > ....
> > LD u-boot
> > OBJCOPY u-boot-nodtb.bin
> > OBJCOPY u-boot.srec
> > SYM u-boot.sym
> > DTC arch/arm/dts/dra72-evm.dtb
> > DTC arch/arm/dts/dra7-evm.dtb
> > DTC arch/arm/dts/dra72-evm.dtb
> > DTC arch/arm/dts/dra7-evm.dtb
> > Error: arch/arm/dts/dra7-evm.dts:492.15-16 syntax error
> > FATAL ERROR: Unable to parse input tree
> > make[2]: *** [arch/arm/dts/dra7-evm.dtb] Error 1
> > make[2]: *** Waiting for unfinished jobs....
> > Error: arch/arm/dts/dra72-evm.dts:149.26-150.2 syntax error
> > FATAL ERROR: Unable to parse input tree
> > make[2]: *** [arch/arm/dts/dra72-evm.dtb] Error 1
> > make[1]: *** [arch-dtbs] Error 2
> > make: *** [dtbs] Error 2
> > make: *** Waiting for unfinished jobs....
> > SHIPPED dts/dt.dtb
> > ----------------------->8---------------------------
>
> Probably you are missing a dependency somewhere - but the failure to
> parse is odd. What is it missing?
This failure in parsing is not the real issue. What I found (at least
this is how interpreted it) two concurrent DTC builds on the same file
are stomping over each other in terms of temp files. Besides this I'd
also have a very hard time explaining this error in any other way since
the dts file itself is perfectly fine.
> >
> > However with the .NOTPARALLEL: dtbs line included it works. Also when
> > doing make a second time it works as well (clearly that's not a
> > solution:)
> >
> > On my quest trying to digest/root cause this I concluded that this may
> > be caused by the DTB files somehow getting compiled twice (see snippet).
> > This in combination with the fact that we post-process files like
> > dta7-evm.dtb (signing them) and creating new DTB files with the same
> > name (like dra7-evm.dtb) replacing the old ones causes issues during the
> > DTB compile step.
>
> Yes I think it is better to create new files, since otherwise the
> build system can't determine which version of the file it should
> target...it assumes a single version as I understand it.
Yes this is how I had it initially. But as indicated the issue was that
the mkimage step that packages this newly-named DTB files would put them
into the u-boot.img FIT blob also with this odd new name. And then the
board match would not work anymore... And that was when I decided to
stop hacking stuff to get it to work.
> >
> > Now I'm not a make guru but I tried to clean up the dependency chain as
> > good as I can but could not get it to the point where each DTB file is
> > only build once which most likely would allow us getting rid of
> > NOPARALLEL.
> >
> > On a related note we wanted to keep the same names for the DTB files
> > (rather than turning dra7-evm.dtb into dra7-evm-sec.dtb for example, as
> > I had it initially) since it's important as they get bundled into the
> > u-boot.img FIT blob so that they can be later matched by the SPL to a
> > board. But it appears like that trying to keep the same names despite
> > the additional signing step seems to complicate things from a make
> > perspective.
>
> Yes - best to create a new u-boot-sec.img I think.
U-Boot itself is not really the issue here - in fact we are already
creating an u-boot_HS.img output artifact. The issue is with the DTBs...
> > If you or anybody else has any smart suggestion how to address this in
> > a better way I'd love to hear about it.
>
> Masahiro is the expert, but my ideas are above.
Awesome. Now that the kids are asleep I'm actually in the process
preparing/testing an optimized patch series based on your, Lokesh's, and
Tom's feedback. I should be able to send it out soon hopefully tonight.
It won't have 100% of feedback implemented (Make stuff is the same) but
it'll be a better base for continued discussion.
Best Regards,
--
Andreas Dannenberg
Texas Instruments Inc
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT
2016-06-21 2:35 ` Andreas Dannenberg
@ 2016-06-23 4:59 ` Masahiro Yamada
2016-06-23 13:23 ` Andreas Dannenberg
0 siblings, 1 reply; 29+ messages in thread
From: Masahiro Yamada @ 2016-06-23 4:59 UTC (permalink / raw)
To: u-boot
2016-06-21 11:35 GMT+09:00 Andreas Dannenberg <dannenberg@ti.com>:
> Hi Simon,
> thanks for the continued feedback. Comments below...
>
> On Mon, Jun 20, 2016 at 04:40:10PM -0600, Simon Glass wrote:
>> +Masahiro for the Makefile question
>>
>> Hi Andreas,
>>
>> On 17 June 2016 at 10:13, Andreas Dannenberg <dannenberg@ti.com> wrote:
>> > Hi Simon,
>> > many thanks for your feedback on the series... I know it takes a lot of
>> > effort to digest all that stuff. We'll see how we can tackle the
>> > feedback one by one and send out an updated series.
>> >
>> > Regarding this patch, please see below...
>> >
>> > On Thu, Jun 16, 2016 at 09:52:52PM -0600, Simon Glass wrote:
>> >> Hi Andreas,
>> >>
>> >> On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
>> >> > From: Daniel Allred <d-allred@ti.com>
>> >> >
>> >> > Adds commands so that when a secure device is in use and the SPL is
>> >> > built to load a FIT image (with combined u-boot binary and various
>> >> > DTBs), these components that get fed into the FIT are all processed to
>> >> > be signed/encrypted/etc. as per the operations performed by the
>> >> > secure-binary-image script of the TI SECDEV package.
>> >> >
>> >> > Signed-off-by: Daniel Allred <d-allred@ti.com>
>> >> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>> >> > ---
>> >> > arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++-
>> >> > arch/arm/cpu/armv7/omap5/config.mk | 3 ++
>> >> > 2 files changed, 58 insertions(+), 2 deletions(-)
>> >>
>> >> Reviewed-by: Simon Glass <sjg@chromium.org>
>> >>
>> >> But please can you add a README for this somewhere?
>> >>
>> >> Also, can this tool be added to U-Boot instead of being external?
>> >
>> > Yes we will definitely enhance the readme in the PATCH version, this
>> > wasn't quite ready yet by the time we submitted the RFC.
>> >
>> > Also regarding the external singing/encryption tool this is only
>> > available from TI under NDA after customers engage with TI just like the
>> > high-secure (HS) devices themselves (you can't just buy them on
>> > Digi-Key). Personally I hope that we eventually lower this barrier of
>> > entry (and this has been brought up more than once) but as many things
>> > in the corporate world there is a complex decision process behind it. So
>> > until this strategy changes (if ever) our poor developer's hands are
>> > tied. All we can do for now is trying to allow this external tool to get
>> > hooked into the U-Boot build process in the easiest way possible which
>> > is already done today by way of the $TI_SECURE_DEV_PKG environmental
>> > variable during SPL build.
>>
>> OK. That's odd because it should be the keys that are secure, not the
>> tool! If anything, the tool should have as many eyes on it as
>> possible.
>
> Yes that's the ideal-world case I was also arguing we should follow...
> But as indicated decision processes in the corporate world sometimes are
> complex :)
>
>> >> >
>> >> > diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk
>> >> > index c7bb101..c4514ad 100644
>> >> > --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk
>> >> > +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk
>> >> > @@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
>> >> > $(if $(KBUILD_VERBOSE:1=), >/dev/null)
>> >> > else
>> >> > cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
>> >> > - $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
>> >> > - $(if $(KBUILD_VERBOSE:1=), >/dev/null)
>> >> > + $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
>> >> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
>> >> > endif
>> >> > else
>> >> > cmd_mkomapsecimg = echo "WARNING:" \
>> >> > @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
>> >> > "variable must be defined for TI secure devices. $@ was NOT created!"
>> >> > endif
>> >> >
>> >> > +ifdef CONFIG_SPL_LOAD_FIT
>> >> > +quiet_cmd_omapsecureimg = SECURE $@
>> >> > +ifneq ($(TI_SECURE_DEV_PKG),)
>> >> > +ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),)
>> >> > +cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \
>> >> > + $< $@ \
>> >> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
>> >> > +else
>> >> > +cmd_omapsecureimg = echo "WARNING:" \
>> >> > + "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
>> >> > + "$@ was NOT created!"; cp $< $@
>> >> > +endif
>> >> > +else
>> >> > +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
>> >> > + "variable must be defined for TI secure devices." \
>> >> > + "$@ was NOT created!"; cp $< $@
>> >> > +endif
>> >> > +endif
>> >> > +
>> >> > +
>> >> > # Standard X-LOADER target (QPSI, NOR flash)
>> >> > u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin
>> >> > $(call if_changed,mkomapsecimg)
>> >> > @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin
>> >> > # the mkomapsecimg command looks for a u-boot-HS_* prefix
>> >> > u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin
>> >> > $(call if_changed,mkomapsecimg)
>> >> > +
>> >> > +# For supporting the SPL loading and interpreting
>> >> > +# of FIT images whose components are pre-processed
>> >> > +# before being integrated into the FIT image in order
>> >> > +# to secure them in some way
>> >> > +ifdef CONFIG_SPL_LOAD_FIT
>> >> > +
>> >> > +MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
>> >> > + -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
>> >> > + -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
>> >> > + $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
>> >> > +
>> >> > +OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
>> >> > +$(OF_LIST_TARGETS): dtbs
>> >> > +
>> >> > +%_HS.dtb: %.dtb
>> >> > + $(call if_changed,omapsecureimg)
>> >> > + $(Q)if [ -f $@ ]; then \
>> >> > + cp -f $@ $<; \
>> >> > + fi
>> >> > +
>> >> > +u-boot-nodtb_HS.bin: u-boot-nodtb.bin
>> >> > + $(call if_changed,omapsecureimg)
>> >> > +
>> >> > +u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))
>> >> > + $(call if_changed,mkimage)
>> >> > + $(Q)if [ -f $@ ]; then \
>> >> > + cp -f $@ u-boot.img; \
>> >> > + fi
>> >> > +
>> >> > +.NOTPARALLEL: dtbs
>> >>
>> >> Why is that needed?
>> >
>> > Good catch! This issue actually caused me a fair amount of grief when
>> > running into it. During development we found the build process works
>> > with make -j1 but would fail for parallel builds (like make -j8) with
>> > the following error....
>> >
>> > ----------------------->8---------------------------
>> > $ make mrproper
>> > $ make dra7xx_hs_evm_defconfig
>> > $ make -j8
>> > ....
>> > LD u-boot
>> > OBJCOPY u-boot-nodtb.bin
>> > OBJCOPY u-boot.srec
>> > SYM u-boot.sym
>> > DTC arch/arm/dts/dra72-evm.dtb
>> > DTC arch/arm/dts/dra7-evm.dtb
>> > DTC arch/arm/dts/dra72-evm.dtb
>> > DTC arch/arm/dts/dra7-evm.dtb
>> > Error: arch/arm/dts/dra7-evm.dts:492.15-16 syntax error
>> > FATAL ERROR: Unable to parse input tree
>> > make[2]: *** [arch/arm/dts/dra7-evm.dtb] Error 1
>> > make[2]: *** Waiting for unfinished jobs....
>> > Error: arch/arm/dts/dra72-evm.dts:149.26-150.2 syntax error
>> > FATAL ERROR: Unable to parse input tree
>> > make[2]: *** [arch/arm/dts/dra72-evm.dtb] Error 1
>> > make[1]: *** [arch-dtbs] Error 2
>> > make: *** [dtbs] Error 2
>> > make: *** Waiting for unfinished jobs....
>> > SHIPPED dts/dt.dtb
>> > ----------------------->8---------------------------
>>
>> Probably you are missing a dependency somewhere - but the failure to
>> parse is odd. What is it missing?
>
> This failure in parsing is not the real issue. What I found (at least
> this is how interpreted it) two concurrent DTC builds on the same file
> are stomping over each other in terms of temp files. Besides this I'd
> also have a very hard time explaining this error in any other way since
> the dts file itself is perfectly fine.
>
>> >
>> > However with the .NOTPARALLEL: dtbs line included it works. Also when
>> > doing make a second time it works as well (clearly that's not a
>> > solution:)
>> >
>> > On my quest trying to digest/root cause this I concluded that this may
>> > be caused by the DTB files somehow getting compiled twice (see snippet).
>> > This in combination with the fact that we post-process files like
>> > dta7-evm.dtb (signing them) and creating new DTB files with the same
>> > name (like dra7-evm.dtb) replacing the old ones causes issues during the
>> > DTB compile step.
>>
>> Yes I think it is better to create new files, since otherwise the
>> build system can't determine which version of the file it should
>> target...it assumes a single version as I understand it.
>
> Yes this is how I had it initially. But as indicated the issue was that
> the mkimage step that packages this newly-named DTB files would put them
> into the u-boot.img FIT blob also with this odd new name. And then the
> board match would not work anymore... And that was when I decided to
> stop hacking stuff to get it to work.
>
>> >
>> > Now I'm not a make guru but I tried to clean up the dependency chain as
>> > good as I can but could not get it to the point where each DTB file is
>> > only build once which most likely would allow us getting rid of
>> > NOPARALLEL.
>> >
>> > On a related note we wanted to keep the same names for the DTB files
>> > (rather than turning dra7-evm.dtb into dra7-evm-sec.dtb for example, as
>> > I had it initially) since it's important as they get bundled into the
>> > u-boot.img FIT blob so that they can be later matched by the SPL to a
>> > board. But it appears like that trying to keep the same names despite
>> > the additional signing step seems to complicate things from a make
>> > perspective.
>>
>> Yes - best to create a new u-boot-sec.img I think.
>
> U-Boot itself is not really the issue here - in fact we are already
> creating an u-boot_HS.img output artifact. The issue is with the DTBs...
>
>> > If you or anybody else has any smart suggestion how to address this in
>> > a better way I'd love to hear about it.
>>
>> Masahiro is the expert, but my ideas are above.
>
> Awesome. Now that the kids are asleep I'm actually in the process
> preparing/testing an optimized patch series based on your, Lokesh's, and
> Tom's feedback. I should be able to send it out soon hopefully tonight.
> It won't have 100% of feedback implemented (Make stuff is the same) but
> it'll be a better base for continued discussion.
>
> Best Regards,
>
Hi.
Could you try this?
http://patchwork.ozlabs.org/patch/639478/
I think you can remove the ".NOTPARALLEL" line.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT
2016-06-23 4:59 ` Masahiro Yamada
@ 2016-06-23 13:23 ` Andreas Dannenberg
0 siblings, 0 replies; 29+ messages in thread
From: Andreas Dannenberg @ 2016-06-23 13:23 UTC (permalink / raw)
To: u-boot
On Thu, Jun 23, 2016 at 01:59:40PM +0900, Masahiro Yamada wrote:
> 2016-06-21 11:35 GMT+09:00 Andreas Dannenberg <dannenberg@ti.com>:
> > Hi Simon,
> > thanks for the continued feedback. Comments below...
> >
> > On Mon, Jun 20, 2016 at 04:40:10PM -0600, Simon Glass wrote:
> >> +Masahiro for the Makefile question
> >>
> >> Hi Andreas,
> >>
> >> On 17 June 2016 at 10:13, Andreas Dannenberg <dannenberg@ti.com> wrote:
> >> > Hi Simon,
> >> > many thanks for your feedback on the series... I know it takes a lot of
> >> > effort to digest all that stuff. We'll see how we can tackle the
> >> > feedback one by one and send out an updated series.
> >> >
> >> > Regarding this patch, please see below...
> >> >
> >> > On Thu, Jun 16, 2016 at 09:52:52PM -0600, Simon Glass wrote:
> >> >> Hi Andreas,
> >> >>
> >> >> On 15 June 2016 at 13:26, Andreas Dannenberg <dannenberg@ti.com> wrote:
> >> >> > From: Daniel Allred <d-allred@ti.com>
> >> >> >
> >> >> > Adds commands so that when a secure device is in use and the SPL is
> >> >> > built to load a FIT image (with combined u-boot binary and various
> >> >> > DTBs), these components that get fed into the FIT are all processed to
> >> >> > be signed/encrypted/etc. as per the operations performed by the
> >> >> > secure-binary-image script of the TI SECDEV package.
> >> >> >
> >> >> > Signed-off-by: Daniel Allred <d-allred@ti.com>
> >> >> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >> >> > ---
> >> >> > arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++-
> >> >> > arch/arm/cpu/armv7/omap5/config.mk | 3 ++
> >> >> > 2 files changed, 58 insertions(+), 2 deletions(-)
> >> >>
> >> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >> >>
> >> >> But please can you add a README for this somewhere?
> >> >>
> >> >> Also, can this tool be added to U-Boot instead of being external?
> >> >
> >> > Yes we will definitely enhance the readme in the PATCH version, this
> >> > wasn't quite ready yet by the time we submitted the RFC.
> >> >
> >> > Also regarding the external singing/encryption tool this is only
> >> > available from TI under NDA after customers engage with TI just like the
> >> > high-secure (HS) devices themselves (you can't just buy them on
> >> > Digi-Key). Personally I hope that we eventually lower this barrier of
> >> > entry (and this has been brought up more than once) but as many things
> >> > in the corporate world there is a complex decision process behind it. So
> >> > until this strategy changes (if ever) our poor developer's hands are
> >> > tied. All we can do for now is trying to allow this external tool to get
> >> > hooked into the U-Boot build process in the easiest way possible which
> >> > is already done today by way of the $TI_SECURE_DEV_PKG environmental
> >> > variable during SPL build.
> >>
> >> OK. That's odd because it should be the keys that are secure, not the
> >> tool! If anything, the tool should have as many eyes on it as
> >> possible.
> >
> > Yes that's the ideal-world case I was also arguing we should follow...
> > But as indicated decision processes in the corporate world sometimes are
> > complex :)
> >
> >> >> >
> >> >> > diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk
> >> >> > index c7bb101..c4514ad 100644
> >> >> > --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk
> >> >> > +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk
> >> >> > @@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
> >> >> > $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> >> >> > else
> >> >> > cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \
> >> >> > - $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
> >> >> > - $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> >> >> > + $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
> >> >> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> >> >> > endif
> >> >> > else
> >> >> > cmd_mkomapsecimg = echo "WARNING:" \
> >> >> > @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
> >> >> > "variable must be defined for TI secure devices. $@ was NOT created!"
> >> >> > endif
> >> >> >
> >> >> > +ifdef CONFIG_SPL_LOAD_FIT
> >> >> > +quiet_cmd_omapsecureimg = SECURE $@
> >> >> > +ifneq ($(TI_SECURE_DEV_PKG),)
> >> >> > +ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),)
> >> >> > +cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \
> >> >> > + $< $@ \
> >> >> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null)
> >> >> > +else
> >> >> > +cmd_omapsecureimg = echo "WARNING:" \
> >> >> > + "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
> >> >> > + "$@ was NOT created!"; cp $< $@
> >> >> > +endif
> >> >> > +else
> >> >> > +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \
> >> >> > + "variable must be defined for TI secure devices." \
> >> >> > + "$@ was NOT created!"; cp $< $@
> >> >> > +endif
> >> >> > +endif
> >> >> > +
> >> >> > +
> >> >> > # Standard X-LOADER target (QPSI, NOR flash)
> >> >> > u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin
> >> >> > $(call if_changed,mkomapsecimg)
> >> >> > @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin
> >> >> > # the mkomapsecimg command looks for a u-boot-HS_* prefix
> >> >> > u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin
> >> >> > $(call if_changed,mkomapsecimg)
> >> >> > +
> >> >> > +# For supporting the SPL loading and interpreting
> >> >> > +# of FIT images whose components are pre-processed
> >> >> > +# before being integrated into the FIT image in order
> >> >> > +# to secure them in some way
> >> >> > +ifdef CONFIG_SPL_LOAD_FIT
> >> >> > +
> >> >> > +MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
> >> >> > + -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> >> >> > + -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
> >> >> > + $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> >> >> > +
> >> >> > +OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> >> >> > +$(OF_LIST_TARGETS): dtbs
> >> >> > +
> >> >> > +%_HS.dtb: %.dtb
> >> >> > + $(call if_changed,omapsecureimg)
> >> >> > + $(Q)if [ -f $@ ]; then \
> >> >> > + cp -f $@ $<; \
> >> >> > + fi
> >> >> > +
> >> >> > +u-boot-nodtb_HS.bin: u-boot-nodtb.bin
> >> >> > + $(call if_changed,omapsecureimg)
> >> >> > +
> >> >> > +u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))
> >> >> > + $(call if_changed,mkimage)
> >> >> > + $(Q)if [ -f $@ ]; then \
> >> >> > + cp -f $@ u-boot.img; \
> >> >> > + fi
> >> >> > +
> >> >> > +.NOTPARALLEL: dtbs
> >> >>
> >> >> Why is that needed?
> >> >
> >> > Good catch! This issue actually caused me a fair amount of grief when
> >> > running into it. During development we found the build process works
> >> > with make -j1 but would fail for parallel builds (like make -j8) with
> >> > the following error....
> >> >
> >> > ----------------------->8---------------------------
> >> > $ make mrproper
> >> > $ make dra7xx_hs_evm_defconfig
> >> > $ make -j8
> >> > ....
> >> > LD u-boot
> >> > OBJCOPY u-boot-nodtb.bin
> >> > OBJCOPY u-boot.srec
> >> > SYM u-boot.sym
> >> > DTC arch/arm/dts/dra72-evm.dtb
> >> > DTC arch/arm/dts/dra7-evm.dtb
> >> > DTC arch/arm/dts/dra72-evm.dtb
> >> > DTC arch/arm/dts/dra7-evm.dtb
> >> > Error: arch/arm/dts/dra7-evm.dts:492.15-16 syntax error
> >> > FATAL ERROR: Unable to parse input tree
> >> > make[2]: *** [arch/arm/dts/dra7-evm.dtb] Error 1
> >> > make[2]: *** Waiting for unfinished jobs....
> >> > Error: arch/arm/dts/dra72-evm.dts:149.26-150.2 syntax error
> >> > FATAL ERROR: Unable to parse input tree
> >> > make[2]: *** [arch/arm/dts/dra72-evm.dtb] Error 1
> >> > make[1]: *** [arch-dtbs] Error 2
> >> > make: *** [dtbs] Error 2
> >> > make: *** Waiting for unfinished jobs....
> >> > SHIPPED dts/dt.dtb
> >> > ----------------------->8---------------------------
> >>
> >> Probably you are missing a dependency somewhere - but the failure to
> >> parse is odd. What is it missing?
> >
> > This failure in parsing is not the real issue. What I found (at least
> > this is how interpreted it) two concurrent DTC builds on the same file
> > are stomping over each other in terms of temp files. Besides this I'd
> > also have a very hard time explaining this error in any other way since
> > the dts file itself is perfectly fine.
> >
> >> >
> >> > However with the .NOTPARALLEL: dtbs line included it works. Also when
> >> > doing make a second time it works as well (clearly that's not a
> >> > solution:)
> >> >
> >> > On my quest trying to digest/root cause this I concluded that this may
> >> > be caused by the DTB files somehow getting compiled twice (see snippet).
> >> > This in combination with the fact that we post-process files like
> >> > dta7-evm.dtb (signing them) and creating new DTB files with the same
> >> > name (like dra7-evm.dtb) replacing the old ones causes issues during the
> >> > DTB compile step.
> >>
> >> Yes I think it is better to create new files, since otherwise the
> >> build system can't determine which version of the file it should
> >> target...it assumes a single version as I understand it.
> >
> > Yes this is how I had it initially. But as indicated the issue was that
> > the mkimage step that packages this newly-named DTB files would put them
> > into the u-boot.img FIT blob also with this odd new name. And then the
> > board match would not work anymore... And that was when I decided to
> > stop hacking stuff to get it to work.
> >
> >> >
> >> > Now I'm not a make guru but I tried to clean up the dependency chain as
> >> > good as I can but could not get it to the point where each DTB file is
> >> > only build once which most likely would allow us getting rid of
> >> > NOPARALLEL.
> >> >
> >> > On a related note we wanted to keep the same names for the DTB files
> >> > (rather than turning dra7-evm.dtb into dra7-evm-sec.dtb for example, as
> >> > I had it initially) since it's important as they get bundled into the
> >> > u-boot.img FIT blob so that they can be later matched by the SPL to a
> >> > board. But it appears like that trying to keep the same names despite
> >> > the additional signing step seems to complicate things from a make
> >> > perspective.
> >>
> >> Yes - best to create a new u-boot-sec.img I think.
> >
> > U-Boot itself is not really the issue here - in fact we are already
> > creating an u-boot_HS.img output artifact. The issue is with the DTBs...
> >
> >> > If you or anybody else has any smart suggestion how to address this in
> >> > a better way I'd love to hear about it.
> >>
> >> Masahiro is the expert, but my ideas are above.
> >
> > Awesome. Now that the kids are asleep I'm actually in the process
> > preparing/testing an optimized patch series based on your, Lokesh's, and
> > Tom's feedback. I should be able to send it out soon hopefully tonight.
> > It won't have 100% of feedback implemented (Make stuff is the same) but
> > it'll be a better base for continued discussion.
> >
> > Best Regards,
> >
>
>
> Hi.
>
> Could you try this?
> http://patchwork.ozlabs.org/patch/639478/
>
> I think you can remove the ".NOTPARALLEL" line.
Yamada-san, yes your patch fixes this. Thanks for spending time thinking
about this issue. Will remove the .NOTPARALLEL hack in the next revision
of this patch series.
Regards,
--
Andreas Dannenberg
Texas Instruments Inc
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-06-23 13:23 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-15 19:26 [U-Boot] [RFC 0/9] Secure Boot by Authenticating/Decrypting SPL FIT blobs Andreas Dannenberg
2016-06-15 19:26 ` [U-Boot] [RFC 1/9] spl: fit: add support for post-processing of images Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-15 19:26 ` [U-Boot] [RFC 2/9] arm: cache: add missing dummy functions for when dcache disabled Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-20 2:13 ` Tom Rini
2016-06-15 19:26 ` [U-Boot] [RFC 3/9] arm: omap-common: add secure smc entry Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-15 19:26 ` [U-Boot] [RFC 4/9] arm: omap-common: add secure rom call API for secure devices Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-17 4:18 ` Lokesh Vutla
2016-06-15 19:26 ` [U-Boot] [RFC 5/9] arm: omap5: add secure ROM signature verify API Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-20 2:13 ` Tom Rini
2016-06-15 19:26 ` [U-Boot] [RFC 6/9] arm: omap5: add FIT image post process function Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-17 4:26 ` Lokesh Vutla
2016-06-15 19:26 ` [U-Boot] [RFC 7/9] arm: am4x: add secure ROM signature verify API Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-15 19:26 ` [U-Boot] [RFC 8/9] arm: am4x: add FIT image post process function Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-17 4:27 ` Lokesh Vutla
2016-06-15 19:26 ` [U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT Andreas Dannenberg
2016-06-17 3:52 ` Simon Glass
2016-06-17 16:13 ` Andreas Dannenberg
2016-06-20 22:40 ` Simon Glass
2016-06-21 2:35 ` Andreas Dannenberg
2016-06-23 4:59 ` Masahiro Yamada
2016-06-23 13:23 ` Andreas Dannenberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.