diff mbox series

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

Message ID 20211028062356.98224-3-takahiro.akashi@linaro.org
State Superseded
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: capsule: improve capsule authentication support | expand

Commit Message

AKASHI Takahiro Oct. 28, 2021, 6:23 a.m. UTC 
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(-)

Comments

Simon Glass Oct. 29, 2021, 3:17 a.m. UTC | #1 
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>

This looks OK but I have some suggestions

- I don't think you should return -1 from main
- could you split up your create_fwbin() to return the number of gotos?
- could we have a man page for the tool?
- should the files be opened in binary mode?
- can we just build the tool always?

Regards,
Simon
AKASHI Takahiro Oct. 29, 2021, 4:56 a.m. UTC | #2 
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
Simon Glass Nov. 2, 2021, 2:56 p.m. UTC | #3 
Hi Takahiro,

On Thu, 28 Oct 2021 at 22:56, AKASHI Takahiro
<takahiro.akashi@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@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.

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
Mark Kettenis Nov. 2, 2021, 3:13 p.m. UTC | #4 
> From: Simon Glass <sjg@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 *);
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
In file included from include/linux/string.h:21:
./arch/arm/include/asm/string.h:20:15: error: conflicting types for 'memcpy'
extern void * memcpy(void *, const void *, __kernel_size_t);
              ^
/usr/include/string.h:65:7: note: previous declaration is here
void    *memcpy(void *__restrict, const void *__restrict, size_t)
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
In file included from include/linux/string.h:21:
./arch/arm/include/asm/string.h:27:15: error: conflicting types for 'memmove'
extern void * memmove(void *, const void *, __kernel_size_t);
              ^
/usr/include/string.h:68:7: note: previous declaration is here
void    *memmove(void *, const void *, size_t)
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
In file included from include/linux/string.h:21:
./arch/arm/include/asm/string.h:30:15: error: conflicting types for 'memchr'
extern void * memchr(const void *, int, __kernel_size_t);
              ^
/usr/include/string.h:63:7: note: previous declaration is here
void    *memchr(const void *, int, size_t);
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
In file included from include/linux/string.h:21:
./arch/arm/include/asm/string.h:36:15: error: conflicting types for 'memset'
extern void * memset(void *, int, __kernel_size_t);
              ^
/usr/include/string.h:71:7: note: previous declaration is here
void    *memset(void *, int, size_t)
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:27:15: error: conflicting types for 'strncpy'
extern char * strncpy(char *,const char *, __kernel_size_t);
              ^
/usr/include/string.h:84:7: note: previous declaration is here
char    *strncpy(char *__restrict, const char *__restrict, size_t)
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:36:15: error: conflicting types for 'strncat'
extern char * strncat(char *, const char *, __kernel_size_t);
              ^
/usr/include/string.h:81:7: note: previous declaration is here
char    *strncat(char *__restrict, const char *__restrict, size_t)
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:45:12: error: conflicting types for 'strncmp'
extern int strncmp(const char *,const char *,__kernel_size_t);
           ^
/usr/include/string.h:83:6: note: previous declaration is here
int      strncmp(const char *, const char *, size_t);
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:51:12: error: conflicting types for 'strncasecmp'
extern int strncasecmp(const char *s1, const char *s2, __kernel_size_t len);
           ^
/usr/include/strings.h:75:6: note: previous declaration is here
int      strncasecmp(const char *, const char *, size_t);
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:77:24: error: conflicting types for 'strlen'
extern __kernel_size_t strlen(const char *);
                       ^
/usr/include/string.h:80:9: note: previous declaration is here
size_t   strlen(const char *);
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:80:24: error: conflicting types for 'strnlen'
extern __kernel_size_t strnlen(const char *,__kernel_size_t);
                       ^
/usr/include/string.h:115:9: note: previous declaration is here
size_t   strnlen(const char *, size_t);
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:111:15: error: conflicting types for 'memset'
extern void * memset(void *,int,__kernel_size_t);
              ^
/usr/include/string.h:71:7: note: previous declaration is here
void    *memset(void *, int, size_t)
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:114:15: error: conflicting types for 'memcpy'
extern void * memcpy(void *,const void *,__kernel_size_t);
              ^
/usr/include/string.h:65:7: note: previous declaration is here
void    *memcpy(void *__restrict, const void *__restrict, size_t)
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:117:15: error: conflicting types for 'memmove'
extern void * memmove(void *,const void *,__kernel_size_t);
              ^
/usr/include/string.h:68:7: note: previous declaration is here
void    *memmove(void *, const void *, size_t)
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:123:12: error: conflicting types for 'memcmp'
extern int memcmp(const void *,const void *,__kernel_size_t);
           ^
/usr/include/string.h:64:6: note: previous declaration is here
int      memcmp(const void *, const void *, size_t);
         ^
In file included from tools/mkeficapsule.c:8:
In file included from include/malloc.h:369:
include/linux/string.h:126:15: error: conflicting types for 'memchr'
extern void * memchr(const void *,int,__kernel_size_t);
              ^
/usr/include/string.h:63:7: note: previous declaration is here
void    *memchr(const void *, int, size_t);
         ^
tools/mkeficapsule.c:25:9: warning: 'aligned_u64' macro redefined [-Wmacro-redefined]
#define aligned_u64 __aligned_u64
        ^
include/linux/types.h:118:9: note: previous definition is here
#define aligned_u64 __u64 __aligned(8)
        ^
In file included from tools/mkeficapsule.c:32:
In file included from include/efi_api.h:20:
In file included from include/charset.h:11:
In file included from include/linux/kernel.h:5:
In file included from include/linux/printk.h:4:
include/log.h:382:24: error: expression is not an integer constant expression
        LOGRECF_FORCE_DEBUG     = BIT(0),
                                  ^~~~~~
include/log.h:384:18: error: expression is not an integer constant expression
        LOGRECF_CONT            = BIT(1),
                                  ^~~~~~
include/log.h:421:18: error: expression is not an integer constant expression
        LOGDF_ENABLE            = BIT(0),       /* Device is enabled */
                                  ^~~~~~
fatal error: too many errors emitted, stopping now [-ferror-limit=]
1 warning and 20 errors generated.
Simon Glass Nov. 4, 2021, 2:51 a.m. UTC | #5 
Hi Mark,

On Tue, 2 Nov 2021 at 09:13, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Simon Glass <sjg@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.

Regards,
Simon
AKASHI Takahiro Nov. 4, 2021, 2:59 a.m. UTC | #6 
On Tue, Nov 02, 2021 at 08:56:50AM -0600, Simon Glass wrote:
> Hi Takahiro,
> 
> On Thu, 28 Oct 2021 at 22:56, AKASHI Takahiro
> <takahiro.akashi@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@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.
> 
> 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.

I forgot to add CMD_MKEFITOOL in tools-only_defconfig in v6.
If I need to send v7, I will include it, otherwise send it in a separate patch.

-Takahiro Akashi


> Regards,
> Simon
Mark Kettenis Nov. 4, 2021, 2:31 p.m. UTC | #7 
> From: Simon Glass <sjg@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@xs4all.nl> wrote:
> >
> > > From: Simon Glass <sjg@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.
Simon Glass Nov. 4, 2021, 3:11 p.m. UTC | #8 
Hi Mark,

On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > >
> > > > From: Simon Glass <sjg@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
Mark Kettenis Nov. 4, 2021, 4:51 p.m. UTC | #9 
> From: Simon Glass <sjg@chromium.org>
> Date: Thu, 4 Nov 2021 09:11:59 -0600
> 
> Hi Mark,
> 
> On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > > >
> > > > > From: Simon Glass <sjg@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?

Last time that came it was pointed out that Azure pipelines only
support Windows, macOS and Linux.  I would expect that a macOS build
would catch the issues I'm seeing on OpenBSD.
AKASHI Takahiro Nov. 5, 2021, 1:04 a.m. UTC | #10 
Hi, Simon,

On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
> Hi Mark,
> 
> On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > > >
> > > > > From: Simon Glass <sjg@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.

As far as I know, you initially introduced efi.h and efi_api.h.
What is your intent to have the two?

I think that efi_api.h contains definitions and interfaces defined
in UEFI specification for building EFI application/modules, hence
I believe that it should be target-independent. Right?

But it *includes* efi.h which also contains some definitions
defined in UEFI specification, while efi.h is only for U-Boot as
UEFI application.

I suspect that is the root cause.
Or should we thoroughly use linux headers like "efi/efi.h"
in this tool?

-Takahiro Akashi


> 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
Simon Glass Nov. 5, 2021, 2:02 a.m. UTC | #11 
Hi Mark,

On Thu, 4 Nov 2021 at 10:51, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Simon Glass <sjg@chromium.org>
> > Date: Thu, 4 Nov 2021 09:11:59 -0600
> >
> > Hi Mark,
> >
> > On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > >
> > > > From: Simon Glass <sjg@chromium.org>
> > > > Date: Wed, 3 Nov 2021 20:51:25 -0600
> > > >
> > > > Hi Mark,e
> > > >
> > > > On Tue, 2 Nov 2021 at 09:13, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > >
> > > > > > From: Simon Glass <sjg@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?
>
> Last time that came it was pointed out that Azure pipelines only
> support Windows, macOS and Linux.  I would expect that a macOS build
> would catch the issues I'm seeing on OpenBSD.

So should we have something in doc/build/openbsd.rst ? I have
installed openbsd but have no idea how to build the tools or what
prereqs are needed, nor how to install them...

Regards,
SImon
Simon Glass Nov. 5, 2021, 2:02 a.m. UTC | #12 
Hi Takahiro,

On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Hi, Simon,
>
> On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
> > Hi Mark,
> >
> > On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > >
> > > > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > > > >
> > > > > > From: Simon Glass <sjg@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.
>
> As far as I know, you initially introduced efi.h and efi_api.h.
> What is your intent to have the two?
>
> I think that efi_api.h contains definitions and interfaces defined
> in UEFI specification for building EFI application/modules, hence
> I believe that it should be target-independent. Right?
>
> But it *includes* efi.h which also contains some definitions
> defined in UEFI specification, while efi.h is only for U-Boot as
> UEFI application.
>
> I suspect that is the root cause.

Yes I think you are right.

> Or should we thoroughly use linux headers like "efi/efi.h"
> in this tool?

Well either way, we need host builds to not include U-Boot headers.


- Simon

>
> -Takahiro Akashi
>
>
> > 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
AKASHI Takahiro Nov. 5, 2021, 2:35 a.m. UTC | #13 
On Thu, Nov 04, 2021 at 08:02:40PM -0600, Simon Glass wrote:
> Hi Takahiro,
> 
> On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > Hi, Simon,
> >
> > On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
> > > Hi Mark,
> > >
> > > On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > >
> > > > > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > > > > >
> > > > > > > From: Simon Glass <sjg@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.
> >
> > As far as I know, you initially introduced efi.h and efi_api.h.
> > What is your intent to have the two?
> >
> > I think that efi_api.h contains definitions and interfaces defined
> > in UEFI specification for building EFI application/modules, hence
> > I believe that it should be target-independent. Right?
> >
> > But it *includes* efi.h which also contains some definitions
> > defined in UEFI specification, while efi.h is only for U-Boot as
> > UEFI application.
> >
> > I suspect that is the root cause.
> 
> Yes I think you are right.
> 
> > Or should we thoroughly use linux headers like "efi/efi.h"
> > in this tool?
> 
> Well either way, we need host builds to not include U-Boot headers.

Yeah, but there are still lots of host tools which include U-Boot headers.
In addition, I'm not quite sure whether *generic* efi headers, like
efi/efi.h, are available across different host OSs.

-Takahiro Akashi

> 
> - Simon
> 
> >
> > -Takahiro Akashi
> >
> >
> > > 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
Mark Kettenis Nov. 5, 2021, 8:36 a.m. UTC | #14 
> From: Simon Glass <sjg@chromium.org>
> Date: Thu, 4 Nov 2021 20:02:38 -0600
> 
> Hi Mark,
> 
> On Thu, 4 Nov 2021 at 10:51, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > From: Simon Glass <sjg@chromium.org>
> > > Date: Thu, 4 Nov 2021 09:11:59 -0600
> > >
> > > Hi Mark,
> > >
> > > On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > >
> > > > > From: Simon Glass <sjg@chromium.org>
> > > > > Date: Wed, 3 Nov 2021 20:51:25 -0600
> > > > >
> > > > > Hi Mark,e
> > > > >
> > > > > On Tue, 2 Nov 2021 at 09:13, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > >
> > > > > > > From: Simon Glass <sjg@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?
> >
> > Last time that came it was pointed out that Azure pipelines only
> > support Windows, macOS and Linux.  I would expect that a macOS build
> > would catch the issues I'm seeing on OpenBSD.
> 
> So should we have something in doc/build/openbsd.rst ? I have
> installed openbsd but have no idea how to build the tools or what
> prereqs are needed, nor how to install them...

I suppose I could write something like that.  Seems that the build
requirements for Linux are actually documented in doc/build/gcc.rst?
Could simply add an OpenBSD paragraph to that.
AKASHI Takahiro Nov. 5, 2021, 9:35 a.m. UTC | #15 
On Fri, Nov 05, 2021 at 11:35:00AM +0900, AKASHI Takahiro wrote:
> On Thu, Nov 04, 2021 at 08:02:40PM -0600, Simon Glass wrote:
> > Hi Takahiro,
> > 
> > On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > Hi, Simon,
> > >
> > > On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
> > > > Hi Mark,
> > > >
> > > > On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > >
> > > > > > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > > > > > >
> > > > > > > > From: Simon Glass <sjg@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.
> > >
> > > As far as I know, you initially introduced efi.h and efi_api.h.
> > > What is your intent to have the two?
> > >
> > > I think that efi_api.h contains definitions and interfaces defined
> > > in UEFI specification for building EFI application/modules, hence
> > > I believe that it should be target-independent. Right?
> > >
> > > But it *includes* efi.h which also contains some definitions
> > > defined in UEFI specification, while efi.h is only for U-Boot as
> > > UEFI application.
> > >
> > > I suspect that is the root cause.
> > 
> > Yes I think you are right.
> > 
> > > Or should we thoroughly use linux headers like "efi/efi.h"
> > > in this tool?
> > 
> > Well either way, we need host builds to not include U-Boot headers.
> 
> Yeah, but there are still lots of host tools which include U-Boot headers.
> In addition, I'm not quite sure whether *generic* efi headers, like
> efi/efi.h, are available across different host OSs.

I looked through linux's efi headers under /usr/include/efi,
but they don't provide enough set of definitions to make mkeficapsule
buildable. Particularly, capsule-related structure definitions are missing.

So modifying U-Boot headers and removing target-dependent coding
would be more practical.
(I don't know yet whether it is feasible or not.)

Or even adding host-tools-local headers would be more optimal.

-Takahiro Akashi

> -Takahiro Akashi
> 
> > 
> > - Simon
> > 
> > >
> > > -Takahiro Akashi
> > >
> > >
> > > > 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
AKASHI Takahiro Nov. 8, 2021, 4:55 a.m. UTC | #16 
Heinrich,

On Fri, Nov 05, 2021 at 06:35:08PM +0900, AKASHI Takahiro wrote:
> On Fri, Nov 05, 2021 at 11:35:00AM +0900, AKASHI Takahiro wrote:
> > On Thu, Nov 04, 2021 at 08:02:40PM -0600, Simon Glass wrote:
> > > Hi Takahiro,
> > > 
> > > On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > Hi, Simon,
> > > >
> > > > On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
> > > > > Hi Mark,
> > > > >
> > > > > On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > >
> > > > > > > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > > > > > > >
> > > > > > > > > From: Simon Glass <sjg@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.
> > > >
> > > > As far as I know, you initially introduced efi.h and efi_api.h.
> > > > What is your intent to have the two?
> > > >
> > > > I think that efi_api.h contains definitions and interfaces defined
> > > > in UEFI specification for building EFI application/modules, hence
> > > > I believe that it should be target-independent. Right?
> > > >
> > > > But it *includes* efi.h which also contains some definitions
> > > > defined in UEFI specification, while efi.h is only for U-Boot as
> > > > UEFI application.
> > > >
> > > > I suspect that is the root cause.
> > > 
> > > Yes I think you are right.
> > > 
> > > > Or should we thoroughly use linux headers like "efi/efi.h"
> > > > in this tool?
> > > 
> > > Well either way, we need host builds to not include U-Boot headers.
> > 
> > Yeah, but there are still lots of host tools which include U-Boot headers.
> > In addition, I'm not quite sure whether *generic* efi headers, like
> > efi/efi.h, are available across different host OSs.
> 
> I looked through linux's efi headers under /usr/include/efi,
> but they don't provide enough set of definitions to make mkeficapsule
> buildable. Particularly, capsule-related structure definitions are missing.
> 
> So modifying U-Boot headers and removing target-dependent coding
> would be more practical.
> (I don't know yet whether it is feasible or not.)

What's your thought here?

> Or even adding host-tools-local headers would be more optimal.

I prefer this approach, though.

-Takahiro Akashi


> -Takahiro Akashi
> 
> > -Takahiro Akashi
> > 
> > > 
> > > - Simon
> > > 
> > > >
> > > > -Takahiro Akashi
> > > >
> > > >
> > > > > 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
AKASHI Takahiro Nov. 8, 2021, 8:46 a.m. UTC | #17 
Hi Mark,

On Thu, Nov 04, 2021 at 03:31:40PM +0100, Mark Kettenis wrote:
> > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > >
> > > > From: Simon Glass <sjg@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.

Please let me confirm one thing; you can't build this tool even
without my patch applied. Right?
If so, we may want to discuss and figure out a solution after the patch is
merged as long as the tool won't be built neither by Kconfig default nor
by tools-only_defconfig.

-Takahiro Akashi
AKASHI Takahiro Nov. 15, 2021, 7:50 a.m. UTC | #18 
Heinrich,

On Mon, Nov 08, 2021 at 01:55:24PM +0900, AKASHI Takahiro wrote:
> Heinrich,
> 
> On Fri, Nov 05, 2021 at 06:35:08PM +0900, AKASHI Takahiro wrote:
> > On Fri, Nov 05, 2021 at 11:35:00AM +0900, AKASHI Takahiro wrote:
> > > On Thu, Nov 04, 2021 at 08:02:40PM -0600, Simon Glass wrote:
> > > > Hi Takahiro,
> > > > 
> > > > On Thu, 4 Nov 2021 at 19:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > Hi, Simon,
> > > > >
> > > > > On Thu, Nov 04, 2021 at 09:11:59AM -0600, Simon Glass wrote:
> > > > > > Hi Mark,
> > > > > >
> > > > > > On Thu, 4 Nov 2021 at 08:31, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > > >
> > > > > > > > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > > > > > > > >
> > > > > > > > > > From: Simon Glass <sjg@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.
> > > > >
> > > > > As far as I know, you initially introduced efi.h and efi_api.h.
> > > > > What is your intent to have the two?
> > > > >
> > > > > I think that efi_api.h contains definitions and interfaces defined
> > > > > in UEFI specification for building EFI application/modules, hence
> > > > > I believe that it should be target-independent. Right?
> > > > >
> > > > > But it *includes* efi.h which also contains some definitions
> > > > > defined in UEFI specification, while efi.h is only for U-Boot as
> > > > > UEFI application.
> > > > >
> > > > > I suspect that is the root cause.
> > > > 
> > > > Yes I think you are right.
> > > > 
> > > > > Or should we thoroughly use linux headers like "efi/efi.h"
> > > > > in this tool?
> > > > 
> > > > Well either way, we need host builds to not include U-Boot headers.
> > > 
> > > Yeah, but there are still lots of host tools which include U-Boot headers.
> > > In addition, I'm not quite sure whether *generic* efi headers, like
> > > efi/efi.h, are available across different host OSs.
> > 
> > I looked through linux's efi headers under /usr/include/efi,
> > but they don't provide enough set of definitions to make mkeficapsule
> > buildable. Particularly, capsule-related structure definitions are missing.
> > 
> > So modifying U-Boot headers and removing target-dependent coding
> > would be more practical.
> > (I don't know yet whether it is feasible or not.)
> 
> What's your thought here?
> 
> > Or even adding host-tools-local headers would be more optimal.
> 
> I prefer this approach, though.

I need your feedback on fixing this issue.

-Takahiro Akashi


> -Takahiro Akashi
> 
> 
> > -Takahiro Akashi
> > 
> > > -Takahiro Akashi
> > > 
> > > > 
> > > > - Simon
> > > > 
> > > > >
> > > > > -Takahiro Akashi
> > > > >
> > > > >
> > > > > > 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
diff mbox series

Patch

diff --git a/tools/Kconfig b/tools/Kconfig
index 91ce8ae3e516..117c921da3fe 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -90,4 +90,12 @@  config TOOLS_SHA512
 	help
 	  Enable SHA512 support in the tools builds
 
+config TOOLS_MKEFICAPSULE
+	bool "Build efimkcapsule command"
+	default y if EFI_CAPSULE_ON_DISK
+	help
+	  This command allows users to create a UEFI capsule file and,
+	  optionally sign that file. If you want to enable UEFI capsule
+	  update feature on your target, you certainly need this.
+
 endmenu
diff --git a/tools/Makefile b/tools/Makefile
index b45219e2c30c..5a73cc4b363d 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -238,8 +238,12 @@  hostprogs-$(CONFIG_MIPS) += mips-relocs
 hostprogs-$(CONFIG_ASN1_COMPILER)	+= asn1_compiler
 HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
 
-mkeficapsule-objs	:= mkeficapsule.o $(LIBFDT_OBJS)
-hostprogs-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += mkeficapsule
+HOSTLDLIBS_mkeficapsule += -luuid
+ifeq ($(CONFIG_TOOLS_LIBCRYPTO),y)
+HOSTLDLIBS_mkeficapsule += \
+	$(shell pkg-config --libs libssl libcrypto 2> /dev/null || echo "-lssl -lcrypto")
+endif
+hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
 
 # We build some files with extra pedantic flags to try to minimize things
 # that won't build on some weird host compiler -- though there are lots of
diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index 4995ba4e0c2a..5541e4bda894 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -15,6 +15,16 @@ 
 #include <sys/stat.h>
 #include <sys/types.h>
 
+#include <linux/kconfig.h>
+#ifdef CONFIG_TOOLS_LIBCRYPTO
+#include <openssl/asn1.h>
+#include <openssl/bio.h>
+#include <openssl/evp.h>
+#include <openssl/err.h>
+#include <openssl/pem.h>
+#include <openssl/pkcs7.h>
+#endif
+
 typedef __u8 u8;
 typedef __u16 u16;
 typedef __u32 u32;
@@ -38,12 +48,25 @@  efi_guid_t efi_guid_image_type_uboot_fit =
 		EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID;
 efi_guid_t efi_guid_image_type_uboot_raw =
 		EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID;
+efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
+
+#ifdef CONFIG_TOOLS_LIBCRYPTO
+static const char *opts_short = "f:r:i:I:v:p:c:m:dh";
+#else
+static const char *opts_short = "f:r:i:I:v:h";
+#endif
 
 static struct option options[] = {
 	{"fit", required_argument, NULL, 'f'},
 	{"raw", required_argument, NULL, 'r'},
 	{"index", required_argument, NULL, 'i'},
 	{"instance", required_argument, NULL, 'I'},
+#ifdef CONFIG_TOOLS_LIBCRYPTO
+	{"private-key", required_argument, NULL, 'p'},
+	{"certificate", required_argument, NULL, 'c'},
+	{"monotonic-count", required_argument, NULL, 'm'},
+	{"dump-sig", no_argument, NULL, 'd'},
+#endif
 	{"help", no_argument, NULL, 'h'},
 	{NULL, 0, NULL, 0},
 };
@@ -57,16 +80,280 @@  static void print_usage(void)
 	       "\t-r, --raw <raw image>       new raw image file\n"
 	       "\t-i, --index <index>         update image index\n"
 	       "\t-I, --instance <instance>   update hardware instance\n"
+#ifdef CONFIG_TOOLS_LIBCRYPTO
+	       "\t-p, --private-key <privkey file>  private key file\n"
+	       "\t-c, --certificate <cert file>     signer's certificate file\n"
+	       "\t-m, --monotonic-count <count>     monotonic count\n"
+	       "\t-d, --dump_sig              dump signature (*.p7)\n"
+#endif
 	       "\t-h, --help                  print a help message\n",
 	       tool_name);
 }
 
+/**
+ * auth_context - authentication context
+ * @key_file:	Path to a private key file
+ * @cert_file:	Path to a certificate file
+ * @image_data:	Pointer to firmware data
+ * @image_size:	Size of firmware data
+ * @auth:	Authentication header
+ * @sig_data:	Signature data
+ * @sig_size:	Size of signature data
+ *
+ * Data structure used in create_auth_data(). @key_file through
+ * @image_size are input parameters. @auth, @sig_data and @sig_size
+ * are filled in by create_auth_data().
+ */
+struct auth_context {
+	char *key_file;
+	char *cert_file;
+	u8 *image_data;
+	size_t image_size;
+	struct efi_firmware_image_authentication auth;
+	u8 *sig_data;
+	size_t sig_size;
+};
+
+static int dump_sig;
+
+#ifdef CONFIG_TOOLS_LIBCRYPTO
+/**
+ * fileio-read_pkey - read out a private key
+ * @filename:	Path to a private key file
+ *
+ * Read out a private key file and parse it into "EVP_PKEY" structure.
+ *
+ * Return:
+ * * Pointer to private key structure  - on success
+ * * NULL - on failure
+ */
+static EVP_PKEY *fileio_read_pkey(const char *filename)
+{
+	EVP_PKEY *key = NULL;
+	BIO *bio;
+
+	bio = BIO_new_file(filename, "r");
+	if (!bio)
+		goto out;
+
+	key = PEM_read_bio_PrivateKey(bio, NULL, NULL, NULL);
+
+out:
+	BIO_free_all(bio);
+	if (!key) {
+		printf("Can't load key from file '%s'\n", filename);
+		ERR_print_errors_fp(stderr);
+	}
+
+	return key;
+}
+
+/**
+ * fileio-read_cert - read out a certificate
+ * @filename:	Path to a certificate file
+ *
+ * Read out a certificate file and parse it into "X509" structure.
+ *
+ * Return:
+ * * Pointer to certificate structure  - on success
+ * * NULL - on failure
+ */
+static X509 *fileio_read_cert(const char *filename)
+{
+	X509 *cert = NULL;
+	BIO *bio;
+
+	bio = BIO_new_file(filename, "r");
+	if (!bio)
+		goto out;
+
+	cert = PEM_read_bio_X509(bio, NULL, NULL, NULL);
+
+out:
+	BIO_free_all(bio);
+	if (!cert) {
+		printf("Can't load certificate from file '%s'\n", filename);
+		ERR_print_errors_fp(stderr);
+	}
+
+	return cert;
+}
+
+/**
+ * create_auth_data - compose authentication data in capsule
+ * @auth_context:	Pointer to authentication context
+ *
+ * Fill up an authentication header (.auth) and signature data (.sig_data)
+ * in @auth_context, using library functions from openssl.
+ * All the parameters in @auth_context must be filled in by a caller.
+ *
+ * Return:
+ * * 0  - on success
+ * * -1 - on failure
+ */
+static int create_auth_data(struct auth_context *ctx)
+{
+	EVP_PKEY *key = NULL;
+	X509 *cert = NULL;
+	BIO *data_bio = NULL;
+	const EVP_MD *md;
+	PKCS7 *p7;
+	int flags, ret = -1;
+
+	OpenSSL_add_all_digests();
+	OpenSSL_add_all_ciphers();
+	ERR_load_crypto_strings();
+
+	key = fileio_read_pkey(ctx->key_file);
+	if (!key)
+		goto err;
+	cert = fileio_read_cert(ctx->cert_file);
+	if (!cert)
+		goto err;
+
+	/*
+	 * create a BIO, containing:
+	 *  * firmware image
+	 *  * monotonic count
+	 * in this order!
+	 * See EDK2's FmpAuthenticatedHandlerRsa2048Sha256()
+	 */
+	data_bio = BIO_new(BIO_s_mem());
+	BIO_write(data_bio, ctx->image_data, ctx->image_size);
+	BIO_write(data_bio, &ctx->auth.monotonic_count,
+		  sizeof(ctx->auth.monotonic_count));
+
+	md = EVP_get_digestbyname("SHA256");
+	if (!md)
+		goto err;
+
+	/* create signature */
+	/* TODO: maybe add PKCS7_NOATTR and PKCS7_NOSMIMECAP */
+	flags = PKCS7_BINARY | PKCS7_DETACHED;
+	p7 = PKCS7_sign(NULL, NULL, NULL, data_bio, flags | PKCS7_PARTIAL);
+	if (!p7)
+		goto err;
+	if (!PKCS7_sign_add_signer(p7, cert, key, md, flags))
+		goto err;
+	if (!PKCS7_final(p7, data_bio, flags))
+		goto err;
+
+	/* convert pkcs7 into DER */
+	ctx->sig_data = NULL;
+	ctx->sig_size = ASN1_item_i2d((ASN1_VALUE *)p7, &ctx->sig_data,
+				      ASN1_ITEM_rptr(PKCS7));
+	if (!ctx->sig_size)
+		goto err;
+
+	/* fill auth_info */
+	ctx->auth.auth_info.hdr.dwLength = sizeof(ctx->auth.auth_info)
+						+ ctx->sig_size;
+	ctx->auth.auth_info.hdr.wRevision = WIN_CERT_REVISION_2_0;
+	ctx->auth.auth_info.hdr.wCertificateType = WIN_CERT_TYPE_EFI_GUID;
+	memcpy(&ctx->auth.auth_info.cert_type, &efi_guid_cert_type_pkcs7,
+	       sizeof(efi_guid_cert_type_pkcs7));
+
+	ret = 0;
+err:
+	BIO_free_all(data_bio);
+	EVP_PKEY_free(key);
+	X509_free(cert);
+
+	return ret;
+}
+
+/**
+ * dump_signature - dump out a signature
+ * @path:	Path to a capsule file
+ * @signature:	Signature data
+ * @sig_size:	Size of signature data
+ *
+ * Signature data pointed to by @signature will be saved into
+ * a file whose file name is @path with ".p7" suffix.
+ *
+ * Return:
+ * * 0  - on success
+ * * -1 - on failure
+ */
+static int dump_signature(const char *path, u8 *signature, size_t sig_size)
+{
+	char *sig_path;
+	FILE *f;
+	size_t size;
+	int ret = -1;
+
+	sig_path = malloc(strlen(path) + 3 + 1);
+	if (!sig_path)
+		return ret;
+
+	sprintf(sig_path, "%s.p7", path);
+	f = fopen(sig_path, "w");
+	if (!f)
+		goto err;
+
+	size = fwrite(signature, 1, sig_size, f);
+	if (size == sig_size)
+		ret = 0;
+
+	fclose(f);
+err:
+	free(sig_path);
+	return ret;
+}
+
+/**
+ * free_sig_data - free out signature data
+ * @ctx:	Pointer to authentication context
+ *
+ * Free signature data allocated in create_auth_data().
+ */
+static void free_sig_data(struct auth_context *ctx)
+{
+	if (ctx->sig_size)
+		OPENSSL_free(ctx->sig_data);
+}
+#else
+static int create_auth_data(struct auth_context *ctx)
+{
+	return 0;
+}
+
+static int dump_signature(const char *path, u8 *signature, size_t sig_size)
+{
+	return 0;
+}
+
+static void free_sig_data(struct auth_context *ctx) {}
+#endif
+
+/**
+ * create_fwbin - create an uefi capsule file
+ * @path:	Path to a created capsule file
+ * @bin:	Path to a firmware binary to encapsulate
+ * @guid:	GUID of related FMP driver
+ * @index:	Index number in capsule
+ * @instance:	Instance number in capsule
+ * @mcount:	Monotonic count in authentication information
+ * @private_file:	Path to a private key file
+ * @cert_file:	Path to a certificate file
+ *
+ * This function actually does the job of creating an uefi capsule file.
+ * All the arguments must be supplied.
+ * If either @private_file ror @cert_file is NULL, the capsule file
+ * won't be signed.
+ *
+ * Return:
+ * * 0  - on success
+ * * -1 - on failure
+ */
 static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
-			unsigned long index, unsigned long instance)
+			unsigned long index, unsigned long instance,
+			uint64_t mcount, char *privkey_file, char *cert_file)
 {
 	struct efi_capsule_header header;
 	struct efi_firmware_management_capsule_header capsule;
 	struct efi_firmware_management_capsule_image_header image;
+	struct auth_context auth_context;
 	FILE *f, *g;
 	struct stat bin_stat;
 	u8 *data;
@@ -76,8 +363,9 @@  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 #ifdef DEBUG
 	printf("For output: %s\n", path);
 	printf("\tbin: %s\n\ttype: %pUl\n", bin, guid);
-	printf("\tindex: %ld\n\tinstance: %ld\n", index, instance);
+	printf("\tindex: %lu\n\tinstance: %lu\n", index, instance);
 #endif
+	auth_context.sig_size = 0;
 
 	g = fopen(bin, "r");
 	if (!g) {
@@ -93,11 +381,34 @@  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 		printf("cannot allocate memory: %zx\n", (size_t)bin_stat.st_size);
 		goto err_1;
 	}
-	f = fopen(path, "w");
-	if (!f) {
-		printf("cannot open %s\n", path);
+
+	size = fread(data, 1, bin_stat.st_size, g);
+	if (size < bin_stat.st_size) {
+		printf("read failed (%zx)\n", size);
 		goto err_2;
 	}
+
+	/* first, calculate signature to determine its size */
+	if (privkey_file && cert_file) {
+		auth_context.key_file = privkey_file;
+		auth_context.cert_file = cert_file;
+		auth_context.auth.monotonic_count = mcount;
+		auth_context.image_data = data;
+		auth_context.image_size = bin_stat.st_size;
+
+		if (create_auth_data(&auth_context)) {
+			printf("Signing firmware image failed\n");
+			goto err_3;
+		}
+
+		if (dump_sig &&
+		    dump_signature(path, auth_context.sig_data,
+				   auth_context.sig_size)) {
+			printf("Creating signature file failed\n");
+			goto err_3;
+		}
+	}
+
 	header.capsule_guid = efi_guid_fm_capsule;
 	header.header_size = sizeof(header);
 	/* TODO: The current implementation ignores flags */
@@ -106,11 +417,20 @@  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 					+ sizeof(capsule) + sizeof(u64)
 					+ sizeof(image)
 					+ bin_stat.st_size;
+	if (auth_context.sig_size)
+		header.capsule_image_size += sizeof(auth_context.auth)
+				+ auth_context.sig_size;
+
+	f = fopen(path, "w");
+	if (!f) {
+		printf("cannot open %s\n", path);
+		goto err_3;
+	}
 
 	size = fwrite(&header, 1, sizeof(header), f);
 	if (size < sizeof(header)) {
 		printf("write failed (%zx)\n", size);
-		goto err_3;
+		goto err_4;
 	}
 
 	capsule.version = 0x00000001;
@@ -119,13 +439,13 @@  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 	size = fwrite(&capsule, 1, sizeof(capsule), f);
 	if (size < (sizeof(capsule))) {
 		printf("write failed (%zx)\n", size);
-		goto err_3;
+		goto err_4;
 	}
 	offset = sizeof(capsule) + sizeof(u64);
 	size = fwrite(&offset, 1, sizeof(offset), f);
 	if (size < sizeof(offset)) {
 		printf("write failed (%zx)\n", size);
-		goto err_3;
+		goto err_4;
 	}
 
 	image.version = 0x00000003;
@@ -135,34 +455,53 @@  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 	image.reserved[1] = 0;
 	image.reserved[2] = 0;
 	image.update_image_size = bin_stat.st_size;
+	if (auth_context.sig_size)
+		image.update_image_size += sizeof(auth_context.auth)
+				+ auth_context.sig_size;
 	image.update_vendor_code_size = 0; /* none */
 	image.update_hardware_instance = instance;
 	image.image_capsule_support = 0;
+	if (auth_context.sig_size)
+		image.image_capsule_support |= CAPSULE_SUPPORT_AUTHENTICATION;
 
 	size = fwrite(&image, 1, sizeof(image), f);
 	if (size < sizeof(image)) {
 		printf("write failed (%zx)\n", size);
-		goto err_3;
+		goto err_4;
 	}
-	size = fread(data, 1, bin_stat.st_size, g);
-	if (size < bin_stat.st_size) {
-		printf("read failed (%zx)\n", size);
-		goto err_3;
+
+	if (auth_context.sig_size) {
+		size = fwrite(&auth_context.auth, 1,
+			      sizeof(auth_context.auth), f);
+		if (size < sizeof(auth_context.auth)) {
+			printf("write failed (%zx)\n", size);
+			goto err_4;
+		}
+		size = fwrite(auth_context.sig_data, 1,
+			      auth_context.sig_size, f);
+		if (size < auth_context.sig_size) {
+			printf("write failed (%zx)\n", size);
+			goto err_4;
+		}
 	}
+
 	size = fwrite(data, 1, bin_stat.st_size, f);
 	if (size < bin_stat.st_size) {
 		printf("write failed (%zx)\n", size);
-		goto err_3;
+		goto err_4;
 	}
 
 	fclose(f);
 	fclose(g);
 	free(data);
+	free_sig_data(&auth_context);
 
 	return 0;
 
-err_3:
+err_4:
 	fclose(f);
+err_3:
+	free_sig_data(&auth_context);
 err_2:
 	free(data);
 err_1:
@@ -171,23 +510,37 @@  err_1:
 	return -1;
 }
 
-/*
- * Usage:
- *   $ mkeficapsule -f <firmware binary> <output file>
+/**
+ * main - main entry function of mkeficapsule
+ * @argc:	Number of arguments
+ * @argv:	Array of pointers to arguments
+ *
+ * Create an uefi capsule file, optionally signing it.
+ * Parse all the arguments and pass them on to create_fwbin().
+ *
+ * Return:
+ * * 0  - on success
+ * * -1 - on failure
  */
 int main(int argc, char **argv)
 {
 	char *file;
 	efi_guid_t *guid;
 	unsigned long index, instance;
+	uint64_t mcount;
+	char *privkey_file, *cert_file;
 	int c, idx;
 
 	file = NULL;
 	guid = NULL;
 	index = 0;
 	instance = 0;
+	mcount = 0;
+	privkey_file = NULL;
+	cert_file = NULL;
+	dump_sig = 0;
 	for (;;) {
-		c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx);
+		c = getopt_long(argc, argv, opts_short, options, &idx);
 		if (c == -1)
 			break;
 
@@ -214,29 +567,47 @@  int main(int argc, char **argv)
 		case 'I':
 			instance = strtoul(optarg, NULL, 0);
 			break;
+#ifdef CONFIG_TOOLS_LIBCRYPTO
+		case 'p':
+			if (privkey_file) {
+				printf("Private Key already specified\n");
+				return -1;
+			}
+			privkey_file = optarg;
+			break;
+		case 'c':
+			if (cert_file) {
+				printf("Certificate file already specified\n");
+				return -1;
+			}
+			cert_file = optarg;
+			break;
+		case 'm':
+			mcount = strtoul(optarg, NULL, 0);
+			break;
+		case 'd':
+			dump_sig = 1;
+			break;
+#endif /* CONFIG_TOOLS_LIBCRYPTO */
 		case 'h':
 			print_usage();
 			return 0;
 		}
 	}
 
-	/* need an output file */
-	if (argc != optind + 1) {
-		print_usage();
-		exit(EXIT_FAILURE);
-	}
-
-	/* need a fit image file or raw image file */
-	if (!file) {
+	/* check necessary parameters */
+	if ((argc != optind + 1) || !file ||
+	    ((privkey_file && !cert_file) ||
+	     (!privkey_file && cert_file))) {
 		print_usage();
-		exit(EXIT_SUCCESS);
+		return -1;
 	}
 
-	if (create_fwbin(argv[optind], file, guid, index, instance)
-			< 0) {
+	if (create_fwbin(argv[optind], file, guid, index, instance,
+			 mcount, privkey_file, cert_file) < 0) {
 		printf("Creating firmware capsule failed\n");
-		exit(EXIT_FAILURE);
+		return -1;
 	}
 
-	exit(EXIT_SUCCESS);
+	return 0;
 }