[U-Boot] [PATCH v3 5/6] binman: add ROM image signing for Bay Trail SoC

[Anatolij Gustschin]

Hi Simon,

On Mon, 20 Nov 2017 08:40:32 -0700
Simon Glass sjg at chromium.org wrote:

> Hi Anatolij,
> 
> On 16 November 2017 at 18:16, Anatolij Gustschin <agust at denx.de> wrote:
> > Generate u-boot-verified.rom image containing Secure Boot Manifest
> > when secure boot option is enabled.
> >
> > Signed-off-by: Anatolij Gustschin <agust at denx.de>
> > ---
> > NOTE: This patch applies on top of binman changes in binman-working
> > branch in git://git.denx.de/u-boot-dm.git
> >
> > Changes in v3:
> >  - New patch. Moved signing script functionality (secure_boot_helper.py
> >    in first series) to binman. The signing is enabled automatically
> >    via u-boot.dtsi when secure boot option is enabled
> >  - Clean up all temporary files generated by signing script
> >
> >  arch/x86/dts/u-boot.dtsi         |   7 +
> >  tools/binman/signing/baytrail.py | 313 +++++++++++++++++++++++++++++++++++++++
> >  tools/binman/signing/signer.py   |   3 +
> >  3 files changed, 323 insertions(+)
> >  create mode 100644 tools/binman/signing/baytrail.py
> >  
> 
> This is a really nice use of binman, integrating various things to
> make it work. It makes me wish we had this for FIT verified boot,
> since at present you need manual steps.
> 
> To finish this, please add a test and info in the binman README about
> the signing feature (x86-specific stuff can stay where it is).

OK.

...
> > +FSP_FILE_NAME = "fsp-sb.bin"  
> 
> Please use ' throughout if you can

OK.

...
> > +SIGNED_MANIFEST_FILE_NAME = 'signed_manifest.bin'
> > +UNSIGNED_MANIFEST_FILE_NAME = 'un'+SIGNED_MANIFEST_FILE_NAME  
> 
> space around +

OK.

...
> > +
> > +oem_data_hash_files = []  
> 
> comment?

OK.

...
> > +
> > +def append_binary_files(first_file, second_file, new_file):  
> 
> function comment. Please fix globally. There is a standard format for
> these, describing args and return value.

OK, done in v4.

...
> > +# This function creates the final U-Boot ROM image from
> > +# the original u-boot.rom and the signed Initial Boot Block
> > +# which contains the Secure Boot Manifest
> > +def assemble_secure_boot_image(u_boot_rom, signed_ibb):
> > +    data = bytearray(open(u_boot_rom, 'rb').read())
> > +    ibb = bytearray(open(signed_ibb, 'rb').read())
> > +    data[-(MANIFEST_SIZE+IBB_SIZE):] = ibb
> > +    open(OUTPUT_FILE_NAME, 'wb').write(data)  
> 
> Should probably use
> 
> with open(OUTPUT_FILE_NAME, 'wb') as fd:
>    fd.write(data)
> 
> so that the file gets closed here.

OK, will fix.

...
> > +    assemble_secure_boot_image(u_boot_rom, signed_ibb)
> > +
> > +    # Cleanup temporary files  
> 
> Instead of this, can you create a tmpdir and remove the whole directory?

OK, will rework for v4.

Thanks,
Anatolij