Re: [PATCH v5 02/11] tools: mkeficapsule: add firmwware image signing

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Thu, Oct 28, 2021 at 09:17:45PM -0600, Simon Glass wrote:
> Hi Takahiro,
> 
> On Thu, 28 Oct 2021 at 00:25, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > With this enhancement, mkeficapsule will be able to sign a capsule
> > file when it is created. A signature added will be used later
> > in the verification at FMP's SetImage() call.
> >
> > To do that, We need specify additional command parameters:
> >   -monotonic-cout <count> : monotonic count
> >   -private-key <private key file> : private key file
> >   -certificate <certificate file> : certificate file
> > Only when all of those parameters are given, a signature will be added
> > to a capsule file.
> >
> > Users are expected to maintain and increment the monotonic count at
> > every time of the update for each firmware image.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  tools/Kconfig        |   8 +
> >  tools/Makefile       |   8 +-
> >  tools/mkeficapsule.c | 435 +++++++++++++++++++++++++++++++++++++++----
> >  3 files changed, 417 insertions(+), 34 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thank you for your reviewing.

> This looks OK but I have some suggestions
> 
> - I don't think you should return -1 from main

exit(EXIT_FAILURE)?
Yeah, but when I first wrote this tool (without authentication support),
'return -1' was used everywhere. So I didn't want to have mixed styles
in this patch.
I will make a change with the tweak below.

> - could you split up your create_fwbin() to return the number of gotos?

Yeah, lots of gotos are messy.

> - could we have a man page for the tool?

Patch#3

> - should the files be opened in binary mode?

Well, the man page of fopen() says,
   This is strictly for compatibility with C89 and has no effect;
   the 'b' is ignored on all POSIX conforming  sys- tems,  including Linux.

U-Boot now requires C11, and so no need? 

> - can we just build the tool always?

This is one of my questions.
Why do you want to do so while there are bunch of tools that are
not always built.

# I saw some discussion in another topic thread, and some distro guy said
# that they used sandbox_defconfig for tool packaging.

-Takahiro Akashi



> Regards,
> Simon