[U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT
Masahiro Yamada
yamada.masahiro at socionext.com
Thu Jun 23 06:59:40 CEST 2016
2016-06-21 11:35 GMT+09:00 Andreas Dannenberg <dannenberg at 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 at 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 at ti.com> wrote:
>> >> > From: Daniel Allred <d-allred at 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 at ti.com>
>> >> > Signed-off-by: Andreas Dannenberg <dannenberg at 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 at 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
More information about the U-Boot
mailing list