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

[Simon Glass]

Hi Takahiro,

On Thu, 28 Oct 2021 at 22:56, AKASHI Takahiro
<takahiro.akashi at linaro.org> wrote:
>
> 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 at 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 at linaro.org>
> > > ---
> > >  tools/Kconfig        |   8 +
> > >  tools/Makefile       |   8 +-
> > >  tools/mkeficapsule.c | 435 +++++++++++++++++++++++++++++++++++++++----
> > >  3 files changed, 417 insertions(+), 34 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg at 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.

OK. I just mean that I think the return code is supposed to be 1 or 2
or maybe 3 on error, not 255.

>
> > - 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

OK

>
> > - 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?

Ah OK. I suppose no one builds this on Windows.

>
> > - 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.

Because I think all tools should be built always. It is fine if that
happens due to CONFIG options but we should try to avoid making it
complicated.

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

What about tools-only ?

So long as the options are enabled it is fine to have options for the
tools. But I think we should try to build all the tools.

Regards,
Simon