[U-Boot] [RFC 9/9] ti: omap-common: Update to generate secure FIT
Andreas Dannenberg
dannenberg at ti.com
Thu Jun 23 15:23:46 CEST 2016
On Thu, Jun 23, 2016 at 01:59:40PM +0900, Masahiro Yamada wrote:
> 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.
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
More information about the U-Boot
mailing list