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

[Simon Glass]

Hi Mark,

On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > From: Simon Glass <sjg at chromium.org>
> > Date: Wed, 3 Nov 2021 20:51:25 -0600
> >
> > Hi Mark,
> >
> > On Tue, 2 Nov 2021 at 09:13, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> > >
> > > > From: Simon Glass <sjg at chromium.org>
> > > > Date: Tue, 2 Nov 2021 08:56:50 -0600
> > > >
> > > > Hi Takahiro,
> > > >
> > > > > > - 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.
> > >
> > > Well, unless this patchset fixes things, we can't, because
> > > mkeficapsule doesn't build on OpenBSD.  I tried looking into it, but I
> > > can't figure out how this is even supposed to compile as a host tool:
> > >
> > >
> > > In file included from tools/mkeficapsule.c:8:
> > > In file included from include/malloc.h:369:
> > > include/linux/string.h:15:24: error: conflicting types for 'strspn'
> > > extern __kernel_size_t strspn(const char *,const char *);
> > >                        ^
> > > /usr/include/string.h:88:9: note: previous declaration is here
> > > size_t   strspn(const char *, const char *);
> >
> > My guess is that linux/string.h should not be included, or perhaps
> > __kernel_size_t should be defined to size_t.
> >
> > I doubt it would take an age to figure out, with a bit of fiddling.
>
> Well, I think the problem is quite fundamental.  Indeed I agree that
> linux/string.h shouldn't be included.  It gets pulled in because the
> tools include <malloc.h>.  Modern software really shouldn't include
> that header anymore, and we removed it in OpenBSD some time ago.  But
> even with that fixed, things break since the same header gets pulled
> in from <efi.h>.
>
> Redefining __kernel_size_t doesn't provide a way out:
>
> tools/mkeficapsule.c:23:16: error: typedef redefinition with different types ('size_t' (aka 'unsigned long') vs 'unsigned int')
> typedef size_t __kernel_size_t;
>                ^
> ./arch/arm/include/asm/posix_types.h:37:23: note: previous definition is here
> typedef unsigned int            __kernel_size_t;
>                                                ^
>
> This is on an amd64 host, so "unsigned int" clearly is the wrong type
> for size_t.
>
> The fundamental problem seems to be that <efi.h> isn't safe to include
> in a "host" tool because it includes "target" headers that
> accidentally resolve to "system" headers on Linux systems.
>
> Maybe Takahiro or Heinrich have an idea how to fix that?  But in the
> meantime it would be good if building this tool would remain optional.

Yes let's ask them to fix that as I agree this sounds wrong. We have
several efi headers so perhaps just need to have the right stuff in
each.

It is OK to have it optional with a CONFIG, but it should be enabled
by default, otherwise no one will know it is there.

Can we get the OpenBSD environment into CI or is that just too hard?

Regards,
Simon