diff mbox series

[2/3] mkeficapsule: Remove dtb related options

Message ID 20210715170030.97758-2-ilias.apalodimas@linaro.org
State Superseded
Headers show
Series [1/3] efi_capsule: Move signature from DTB to .rodata | expand

Commit Message

Ilias Apalodimas July 15, 2021, 5 p.m. UTC
commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb")
added a bunch of options enabling the addition of the capsule public key
in a dtb.  Since now we embeded the key in U-Boot's .rodata we don't this
this functionality anymore

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

---
 tools/mkeficapsule.c | 226 ++-----------------------------------------
 1 file changed, 7 insertions(+), 219 deletions(-)

-- 
2.32.0.rc0

Comments

Masami Hiramatsu July 16, 2021, 5:57 a.m. UTC | #1
2021年7月16日(金) 2:00 Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>

> commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb")

> added a bunch of options enabling the addition of the capsule public key

> in a dtb.  Since now we embeded the key in U-Boot's .rodata we don't this

> this functionality anymore


This looks good to me.

Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>


Thanks,

>

> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> ---

>  tools/mkeficapsule.c | 226 ++-----------------------------------------

>  1 file changed, 7 insertions(+), 219 deletions(-)

>

> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c

> index de0a62898886..214dc38e46e3 100644

> --- a/tools/mkeficapsule.c

> +++ b/tools/mkeficapsule.c

> @@ -4,14 +4,12 @@

>   *             Author: AKASHI Takahiro

>   */

>

> -#include <errno.h>

>  #include <getopt.h>

>  #include <malloc.h>

>  #include <stdbool.h>

>  #include <stdio.h>

>  #include <stdlib.h>

>  #include <string.h>

> -#include <unistd.h>

>  #include <linux/types.h>

>

>  #include <sys/mman.h>

> @@ -29,9 +27,6 @@ typedef __s32 s32;

>

>  #define aligned_u64 __aligned_u64

>

> -#define SIGNATURE_NODENAME     "signature"

> -#define OVERLAY_NODENAME       "__overlay__"

> -

>  #ifndef __packed

>  #define __packed __attribute__((packed))

>  #endif

> @@ -52,9 +47,6 @@ static struct option options[] = {

>         {"raw", required_argument, NULL, 'r'},

>         {"index", required_argument, NULL, 'i'},

>         {"instance", required_argument, NULL, 'I'},

> -       {"dtb", required_argument, NULL, 'D'},

> -       {"public key", required_argument, NULL, 'K'},

> -       {"overlay", no_argument, NULL, 'O'},

>         {"help", no_argument, NULL, 'h'},

>         {NULL, 0, NULL, 0},

>  };

> @@ -68,187 +60,10 @@ 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"

> -              "\t-K, --public-key <key file> public key esl file\n"

> -              "\t-D, --dtb <dtb file>        dtb file\n"

> -              "\t-O, --overlay               the dtb file is an overlay\n"

>                "\t-h, --help                  print a help message\n",

>                tool_name);

>  }

>

> -static int fdt_add_pub_key_data(void *sptr, void *dptr, size_t key_size,

> -                               bool overlay)

> -{

> -       int parent;

> -       int ov_node;

> -       int frag_node;

> -       int ret = 0;

> -

> -       if (overlay) {

> -               /*

> -                * The signature would be stored in the

> -                * first fragment node of the overlay

> -                */

> -               frag_node = fdt_first_subnode(dptr, 0);

> -               if (frag_node == -FDT_ERR_NOTFOUND) {

> -                       fprintf(stderr,

> -                               "Couldn't find the fragment node: %s\n",

> -                               fdt_strerror(frag_node));

> -                       goto done;

> -               }

> -

> -               ov_node = fdt_subnode_offset(dptr, frag_node, OVERLAY_NODENAME);

> -               if (ov_node == -FDT_ERR_NOTFOUND) {

> -                       fprintf(stderr,

> -                               "Couldn't find the __overlay__ node: %s\n",

> -                               fdt_strerror(ov_node));

> -                       goto done;

> -               }

> -       } else {

> -               ov_node = 0;

> -       }

> -

> -       parent = fdt_subnode_offset(dptr, ov_node, SIGNATURE_NODENAME);

> -       if (parent == -FDT_ERR_NOTFOUND) {

> -               parent = fdt_add_subnode(dptr, ov_node, SIGNATURE_NODENAME);

> -               if (parent < 0) {

> -                       ret = parent;

> -                       if (ret != -FDT_ERR_NOSPACE) {

> -                               fprintf(stderr,

> -                                       "Couldn't create signature node: %s\n",

> -                                       fdt_strerror(parent));

> -                       }

> -               }

> -       }

> -       if (ret)

> -               goto done;

> -

> -       /* Write the key to the FDT node */

> -       ret = fdt_setprop(dptr, parent, "capsule-key",

> -                         sptr, key_size);

> -

> -done:

> -       if (ret)

> -               ret = ret == -FDT_ERR_NOSPACE ? -ENOSPC : -EIO;

> -

> -       return ret;

> -}

> -

> -static int add_public_key(const char *pkey_file, const char *dtb_file,

> -                         bool overlay)

> -{

> -       int ret;

> -       int srcfd = -1;

> -       int destfd = -1;

> -       void *sptr = NULL;

> -       void *dptr = NULL;

> -       off_t src_size;

> -       struct stat pub_key;

> -       struct stat dtb;

> -

> -       /* Find out the size of the public key */

> -       srcfd = open(pkey_file, O_RDONLY);

> -       if (srcfd == -1) {

> -               fprintf(stderr, "%s: Can't open %s: %s\n",

> -                       __func__, pkey_file, strerror(errno));

> -               ret = -1;

> -               goto err;

> -       }

> -

> -       ret = fstat(srcfd, &pub_key);

> -       if (ret == -1) {

> -               fprintf(stderr, "%s: Can't stat %s: %s\n",

> -                       __func__, pkey_file, strerror(errno));

> -               ret = -1;

> -               goto err;

> -       }

> -

> -       src_size = pub_key.st_size;

> -

> -       /* mmap the public key esl file */

> -       sptr = mmap(0, src_size, PROT_READ, MAP_SHARED, srcfd, 0);

> -       if (sptr == MAP_FAILED) {

> -               fprintf(stderr, "%s: Failed to mmap %s:%s\n",

> -                       __func__, pkey_file, strerror(errno));

> -               ret = -1;

> -               goto err;

> -       }

> -

> -       /* Open the dest FDT */

> -       destfd = open(dtb_file, O_RDWR);

> -       if (destfd == -1) {

> -               fprintf(stderr, "%s: Can't open %s: %s\n",

> -                       __func__, dtb_file, strerror(errno));

> -               ret = -1;

> -               goto err;

> -       }

> -

> -       ret = fstat(destfd, &dtb);

> -       if (ret == -1) {

> -               fprintf(stderr, "%s: Can't stat %s: %s\n",

> -                       __func__, dtb_file, strerror(errno));

> -               goto err;

> -       }

> -

> -       dtb.st_size += src_size + 0x30;

> -       if (ftruncate(destfd, dtb.st_size)) {

> -               fprintf(stderr, "%s: Can't expand %s: %s\n",

> -                       __func__, dtb_file, strerror(errno));

> -               ret = -1;

> -               goto err;

> -       }

> -

> -       errno = 0;

> -       /* mmap the dtb file */

> -       dptr = mmap(0, dtb.st_size, PROT_READ | PROT_WRITE, MAP_SHARED,

> -                   destfd, 0);

> -       if (dptr == MAP_FAILED) {

> -               fprintf(stderr, "%s: Failed to mmap %s:%s\n",

> -                       __func__, dtb_file, strerror(errno));

> -               ret = -1;

> -               goto err;

> -       }

> -

> -       if (fdt_check_header(dptr)) {

> -               fprintf(stderr, "%s: Invalid FDT header\n", __func__);

> -               ret = -1;

> -               goto err;

> -       }

> -

> -       ret = fdt_open_into(dptr, dptr, dtb.st_size);

> -       if (ret) {

> -               fprintf(stderr, "%s: Cannot expand FDT: %s\n",

> -                       __func__, fdt_strerror(ret));

> -               ret = -1;

> -               goto err;

> -       }

> -

> -       /* Copy the esl file to the expanded FDT */

> -       ret = fdt_add_pub_key_data(sptr, dptr, src_size, overlay);

> -       if (ret < 0) {

> -               fprintf(stderr, "%s: Unable to add public key to the FDT\n",

> -                       __func__);

> -               ret = -1;

> -               goto err;

> -       }

> -

> -       ret = 0;

> -

> -err:

> -       if (sptr)

> -               munmap(sptr, src_size);

> -

> -       if (dptr)

> -               munmap(dptr, dtb.st_size);

> -

> -       if (srcfd != -1)

> -               close(srcfd);

> -

> -       if (destfd != -1)

> -               close(destfd);

> -

> -       return ret;

> -}

> -

>  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,

>                         unsigned long index, unsigned long instance)

>  {

> @@ -366,22 +181,16 @@ err_1:

>  int main(int argc, char **argv)

>  {

>         char *file;

> -       char *pkey_file;

> -       char *dtb_file;

>         efi_guid_t *guid;

>         unsigned long index, instance;

>         int c, idx;

> -       int ret;

> -       bool overlay = false;

>

>         file = NULL;

> -       pkey_file = NULL;

> -       dtb_file = NULL;

>         guid = NULL;

>         index = 0;

>         instance = 0;

>         for (;;) {

> -               c = getopt_long(argc, argv, "f:r:i:I:v:D:K:Oh", options, &idx);

> +               c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx);

>                 if (c == -1)

>                         break;

>

> @@ -408,43 +217,22 @@ int main(int argc, char **argv)

>                 case 'I':

>                         instance = strtoul(optarg, NULL, 0);

>                         break;

> -               case 'K':

> -                       if (pkey_file) {

> -                               printf("Public Key already specified\n");

> -                               return -1;

> -                       }

> -                       pkey_file = optarg;

> -                       break;

> -               case 'D':

> -                       if (dtb_file) {

> -                               printf("DTB file already specified\n");

> -                               return -1;

> -                       }

> -                       dtb_file = optarg;

> -                       break;

> -               case 'O':

> -                       overlay = true;

> -                       break;

>                 case 'h':

>                         print_usage();

>                         return 0;

>                 }

>         }

>

> -       /* need a fit image file or raw image file */

> -       if (!file && !pkey_file && !dtb_file) {

> +       /* need a output file */

> +       if (argc != optind + 1) {

>                 print_usage();

>                 exit(EXIT_FAILURE);

>         }

>

> -       if (pkey_file && dtb_file) {

> -               ret = add_public_key(pkey_file, dtb_file, overlay);

> -               if (ret == -1) {

> -                       printf("Adding public key to the dtb failed\n");

> -                       exit(EXIT_FAILURE);

> -               } else {

> -                       exit(EXIT_SUCCESS);

> -               }

> +       /* need a fit image file or raw image file */

> +       if (!file) {

> +               print_usage();

> +               exit(EXIT_SUCCESS);

>         }

>

>         if (create_fwbin(argv[optind], file, guid, index, instance)

> --

> 2.32.0.rc0

>



-- 
Masami Hiramatsu
AKASHI Takahiro July 16, 2021, 1:53 p.m. UTC | #2
On Fri, Jul 16, 2021 at 02:57:54PM +0900, Masami Hiramatsu wrote:
> 2021年7月16日(金) 2:00 Ilias Apalodimas <ilias.apalodimas@linaro.org>:

> >

> > commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb")

> > added a bunch of options enabling the addition of the capsule public key

> > in a dtb.  Since now we embeded the key in U-Boot's .rodata we don't this

> > this functionality anymore

> 

> This looks good to me.

> 

> Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>

> 

> Thanks,

> 

> >

> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> > ---

> >  tools/mkeficapsule.c | 226 ++-----------------------------------------

> >  1 file changed, 7 insertions(+), 219 deletions(-)

> >

> > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c

> > index de0a62898886..214dc38e46e3 100644

> > --- a/tools/mkeficapsule.c

> > +++ b/tools/mkeficapsule.c

> > @@ -4,14 +4,12 @@

> >   *             Author: AKASHI Takahiro

> >   */

> >

> > -#include <errno.h>

> >  #include <getopt.h>

> >  #include <malloc.h>

> >  #include <stdbool.h>

> >  #include <stdio.h>

> >  #include <stdlib.h>

> >  #include <string.h>

> > -#include <unistd.h>

> >  #include <linux/types.h>

> >

> >  #include <sys/mman.h>


I didn't try the compilation, but I don't think
we need neither <sys/mman.h> nor "fdt_host.h".

-Takahiro Akashi

> > @@ -29,9 +27,6 @@ typedef __s32 s32;

> >

> >  #define aligned_u64 __aligned_u64

> >

> > -#define SIGNATURE_NODENAME     "signature"

> > -#define OVERLAY_NODENAME       "__overlay__"

> > -

> >  #ifndef __packed

> >  #define __packed __attribute__((packed))

> >  #endif

> > @@ -52,9 +47,6 @@ static struct option options[] = {

> >         {"raw", required_argument, NULL, 'r'},

> >         {"index", required_argument, NULL, 'i'},

> >         {"instance", required_argument, NULL, 'I'},

> > -       {"dtb", required_argument, NULL, 'D'},

> > -       {"public key", required_argument, NULL, 'K'},

> > -       {"overlay", no_argument, NULL, 'O'},

> >         {"help", no_argument, NULL, 'h'},

> >         {NULL, 0, NULL, 0},

> >  };

> > @@ -68,187 +60,10 @@ 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"

> > -              "\t-K, --public-key <key file> public key esl file\n"

> > -              "\t-D, --dtb <dtb file>        dtb file\n"

> > -              "\t-O, --overlay               the dtb file is an overlay\n"

> >                "\t-h, --help                  print a help message\n",

> >                tool_name);

> >  }

> >

> > -static int fdt_add_pub_key_data(void *sptr, void *dptr, size_t key_size,

> > -                               bool overlay)

> > -{

> > -       int parent;

> > -       int ov_node;

> > -       int frag_node;

> > -       int ret = 0;

> > -

> > -       if (overlay) {

> > -               /*

> > -                * The signature would be stored in the

> > -                * first fragment node of the overlay

> > -                */

> > -               frag_node = fdt_first_subnode(dptr, 0);

> > -               if (frag_node == -FDT_ERR_NOTFOUND) {

> > -                       fprintf(stderr,

> > -                               "Couldn't find the fragment node: %s\n",

> > -                               fdt_strerror(frag_node));

> > -                       goto done;

> > -               }

> > -

> > -               ov_node = fdt_subnode_offset(dptr, frag_node, OVERLAY_NODENAME);

> > -               if (ov_node == -FDT_ERR_NOTFOUND) {

> > -                       fprintf(stderr,

> > -                               "Couldn't find the __overlay__ node: %s\n",

> > -                               fdt_strerror(ov_node));

> > -                       goto done;

> > -               }

> > -       } else {

> > -               ov_node = 0;

> > -       }

> > -

> > -       parent = fdt_subnode_offset(dptr, ov_node, SIGNATURE_NODENAME);

> > -       if (parent == -FDT_ERR_NOTFOUND) {

> > -               parent = fdt_add_subnode(dptr, ov_node, SIGNATURE_NODENAME);

> > -               if (parent < 0) {

> > -                       ret = parent;

> > -                       if (ret != -FDT_ERR_NOSPACE) {

> > -                               fprintf(stderr,

> > -                                       "Couldn't create signature node: %s\n",

> > -                                       fdt_strerror(parent));

> > -                       }

> > -               }

> > -       }

> > -       if (ret)

> > -               goto done;

> > -

> > -       /* Write the key to the FDT node */

> > -       ret = fdt_setprop(dptr, parent, "capsule-key",

> > -                         sptr, key_size);

> > -

> > -done:

> > -       if (ret)

> > -               ret = ret == -FDT_ERR_NOSPACE ? -ENOSPC : -EIO;

> > -

> > -       return ret;

> > -}

> > -

> > -static int add_public_key(const char *pkey_file, const char *dtb_file,

> > -                         bool overlay)

> > -{

> > -       int ret;

> > -       int srcfd = -1;

> > -       int destfd = -1;

> > -       void *sptr = NULL;

> > -       void *dptr = NULL;

> > -       off_t src_size;

> > -       struct stat pub_key;

> > -       struct stat dtb;

> > -

> > -       /* Find out the size of the public key */

> > -       srcfd = open(pkey_file, O_RDONLY);

> > -       if (srcfd == -1) {

> > -               fprintf(stderr, "%s: Can't open %s: %s\n",

> > -                       __func__, pkey_file, strerror(errno));

> > -               ret = -1;

> > -               goto err;

> > -       }

> > -

> > -       ret = fstat(srcfd, &pub_key);

> > -       if (ret == -1) {

> > -               fprintf(stderr, "%s: Can't stat %s: %s\n",

> > -                       __func__, pkey_file, strerror(errno));

> > -               ret = -1;

> > -               goto err;

> > -       }

> > -

> > -       src_size = pub_key.st_size;

> > -

> > -       /* mmap the public key esl file */

> > -       sptr = mmap(0, src_size, PROT_READ, MAP_SHARED, srcfd, 0);

> > -       if (sptr == MAP_FAILED) {

> > -               fprintf(stderr, "%s: Failed to mmap %s:%s\n",

> > -                       __func__, pkey_file, strerror(errno));

> > -               ret = -1;

> > -               goto err;

> > -       }

> > -

> > -       /* Open the dest FDT */

> > -       destfd = open(dtb_file, O_RDWR);

> > -       if (destfd == -1) {

> > -               fprintf(stderr, "%s: Can't open %s: %s\n",

> > -                       __func__, dtb_file, strerror(errno));

> > -               ret = -1;

> > -               goto err;

> > -       }

> > -

> > -       ret = fstat(destfd, &dtb);

> > -       if (ret == -1) {

> > -               fprintf(stderr, "%s: Can't stat %s: %s\n",

> > -                       __func__, dtb_file, strerror(errno));

> > -               goto err;

> > -       }

> > -

> > -       dtb.st_size += src_size + 0x30;

> > -       if (ftruncate(destfd, dtb.st_size)) {

> > -               fprintf(stderr, "%s: Can't expand %s: %s\n",

> > -                       __func__, dtb_file, strerror(errno));

> > -               ret = -1;

> > -               goto err;

> > -       }

> > -

> > -       errno = 0;

> > -       /* mmap the dtb file */

> > -       dptr = mmap(0, dtb.st_size, PROT_READ | PROT_WRITE, MAP_SHARED,

> > -                   destfd, 0);

> > -       if (dptr == MAP_FAILED) {

> > -               fprintf(stderr, "%s: Failed to mmap %s:%s\n",

> > -                       __func__, dtb_file, strerror(errno));

> > -               ret = -1;

> > -               goto err;

> > -       }

> > -

> > -       if (fdt_check_header(dptr)) {

> > -               fprintf(stderr, "%s: Invalid FDT header\n", __func__);

> > -               ret = -1;

> > -               goto err;

> > -       }

> > -

> > -       ret = fdt_open_into(dptr, dptr, dtb.st_size);

> > -       if (ret) {

> > -               fprintf(stderr, "%s: Cannot expand FDT: %s\n",

> > -                       __func__, fdt_strerror(ret));

> > -               ret = -1;

> > -               goto err;

> > -       }

> > -

> > -       /* Copy the esl file to the expanded FDT */

> > -       ret = fdt_add_pub_key_data(sptr, dptr, src_size, overlay);

> > -       if (ret < 0) {

> > -               fprintf(stderr, "%s: Unable to add public key to the FDT\n",

> > -                       __func__);

> > -               ret = -1;

> > -               goto err;

> > -       }

> > -

> > -       ret = 0;

> > -

> > -err:

> > -       if (sptr)

> > -               munmap(sptr, src_size);

> > -

> > -       if (dptr)

> > -               munmap(dptr, dtb.st_size);

> > -

> > -       if (srcfd != -1)

> > -               close(srcfd);

> > -

> > -       if (destfd != -1)

> > -               close(destfd);

> > -

> > -       return ret;

> > -}

> > -

> >  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,

> >                         unsigned long index, unsigned long instance)

> >  {

> > @@ -366,22 +181,16 @@ err_1:

> >  int main(int argc, char **argv)

> >  {

> >         char *file;

> > -       char *pkey_file;

> > -       char *dtb_file;

> >         efi_guid_t *guid;

> >         unsigned long index, instance;

> >         int c, idx;

> > -       int ret;

> > -       bool overlay = false;

> >

> >         file = NULL;

> > -       pkey_file = NULL;

> > -       dtb_file = NULL;

> >         guid = NULL;

> >         index = 0;

> >         instance = 0;

> >         for (;;) {

> > -               c = getopt_long(argc, argv, "f:r:i:I:v:D:K:Oh", options, &idx);

> > +               c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx);

> >                 if (c == -1)

> >                         break;

> >

> > @@ -408,43 +217,22 @@ int main(int argc, char **argv)

> >                 case 'I':

> >                         instance = strtoul(optarg, NULL, 0);

> >                         break;

> > -               case 'K':

> > -                       if (pkey_file) {

> > -                               printf("Public Key already specified\n");

> > -                               return -1;

> > -                       }

> > -                       pkey_file = optarg;

> > -                       break;

> > -               case 'D':

> > -                       if (dtb_file) {

> > -                               printf("DTB file already specified\n");

> > -                               return -1;

> > -                       }

> > -                       dtb_file = optarg;

> > -                       break;

> > -               case 'O':

> > -                       overlay = true;

> > -                       break;

> >                 case 'h':

> >                         print_usage();

> >                         return 0;

> >                 }

> >         }

> >

> > -       /* need a fit image file or raw image file */

> > -       if (!file && !pkey_file && !dtb_file) {

> > +       /* need a output file */

> > +       if (argc != optind + 1) {

> >                 print_usage();

> >                 exit(EXIT_FAILURE);

> >         }

> >

> > -       if (pkey_file && dtb_file) {

> > -               ret = add_public_key(pkey_file, dtb_file, overlay);

> > -               if (ret == -1) {

> > -                       printf("Adding public key to the dtb failed\n");

> > -                       exit(EXIT_FAILURE);

> > -               } else {

> > -                       exit(EXIT_SUCCESS);

> > -               }

> > +       /* need a fit image file or raw image file */

> > +       if (!file) {

> > +               print_usage();

> > +               exit(EXIT_SUCCESS);

> >         }

> >

> >         if (create_fwbin(argv[optind], file, guid, index, instance)

> > --

> > 2.32.0.rc0

> >

> 

> 

> -- 

> Masami Hiramatsu
Simon Glass July 16, 2021, 2:03 p.m. UTC | #3
Hi Ilias,

On Thu, 15 Jul 2021 at 11:00, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>

> commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb")

> added a bunch of options enabling the addition of the capsule public key

> in a dtb.  Since now we embeded the key in U-Boot's .rodata we don't this

> this functionality anymore

>

> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> ---

>  tools/mkeficapsule.c | 226 ++-----------------------------------------

>  1 file changed, 7 insertions(+), 219 deletions(-)


Here again I see EFI diverging from the impl in U-Boot. WIth U-Boot
you can add the public key after the build step, e.g. in a key-signing
server. With EFI and this change you will have to rebuild U-Boot (from
source) every time you sign something. Seems like a pain.

Regards,
Simon
Ilias Apalodimas July 17, 2021, 7:24 a.m. UTC | #4
On Fri, Jul 16, 2021 at 08:03:23AM -0600, Simon Glass wrote:
> Hi Ilias,

> 

> On Thu, 15 Jul 2021 at 11:00, Ilias Apalodimas

> <ilias.apalodimas@linaro.org> wrote:

> >

> > commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb")

> > added a bunch of options enabling the addition of the capsule public key

> > in a dtb.  Since now we embeded the key in U-Boot's .rodata we don't this

> > this functionality anymore

> >

> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> > ---

> >  tools/mkeficapsule.c | 226 ++-----------------------------------------

> >  1 file changed, 7 insertions(+), 219 deletions(-)

> 

> Here again I see EFI diverging from the impl in U-Boot. WIth U-Boot

> you can add the public key after the build step, e.g. in a key-signing

> server. With EFI and this change you will have to rebuild U-Boot (from

> source) every time you sign something. Seems like a pain.


I don't see why either of this is a problem.  You need the public key to
update the binary it self, so rebuilding from source is a prerequisite. 

Apart from a signing server, you can also have special hardware that provides 
the public key you need (which is not implemented yet).  So this is the bare 
minimum functionality you need  for authenticated capsule updates.


Regards
/Ilias

> 

> Regards,

> Simon
Simon Glass July 20, 2021, 6:33 p.m. UTC | #5
Hi Ilias,

On Sat, 17 Jul 2021 at 01:24, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>

> On Fri, Jul 16, 2021 at 08:03:23AM -0600, Simon Glass wrote:

> > Hi Ilias,

> >

> > On Thu, 15 Jul 2021 at 11:00, Ilias Apalodimas

> > <ilias.apalodimas@linaro.org> wrote:

> > >

> > > commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb")

> > > added a bunch of options enabling the addition of the capsule public key

> > > in a dtb.  Since now we embeded the key in U-Boot's .rodata we don't this

> > > this functionality anymore

> > >

> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> > > ---

> > >  tools/mkeficapsule.c | 226 ++-----------------------------------------

> > >  1 file changed, 7 insertions(+), 219 deletions(-)

> >

> > Here again I see EFI diverging from the impl in U-Boot. WIth U-Boot

> > you can add the public key after the build step, e.g. in a key-signing

> > server. With EFI and this change you will have to rebuild U-Boot (from

> > source) every time you sign something. Seems like a pain.

>

> I don't see why either of this is a problem.  You need the public key to

> update the binary it self, so rebuilding from source is a prerequisite.


Please can you have a look at binman and the concept of packaging
separate from building? Rebuilding from source is definitely not
needed to update a binary.

>

> Apart from a signing server, you can also have special hardware that provides

> the public key you need (which is not implemented yet).  So this is the bare

> minimum functionality you need  for authenticated capsule updates.


As discussed on the mailing list you have not included the motivation
for this. Now that I understand the motivation, which is to avoid
someone changing the key at runtime, I believe that this change does
not actually help...I've replied separately on the mailing list.

Regards,
Simon
Ilias Apalodimas July 20, 2021, 6:43 p.m. UTC | #6
On Tue, 20 Jul 2021 at 21:33, Simon Glass <sjg@chromium.org> wrote:
>

> Hi Ilias,

>

> On Sat, 17 Jul 2021 at 01:24, Ilias Apalodimas

> <ilias.apalodimas@linaro.org> wrote:

> >

> > On Fri, Jul 16, 2021 at 08:03:23AM -0600, Simon Glass wrote:

> > > Hi Ilias,

> > >

> > > On Thu, 15 Jul 2021 at 11:00, Ilias Apalodimas

> > > <ilias.apalodimas@linaro.org> wrote:

> > > >

> > > > commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb")

> > > > added a bunch of options enabling the addition of the capsule public key

> > > > in a dtb.  Since now we embeded the key in U-Boot's .rodata we don't this

> > > > this functionality anymore

> > > >

> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> > > > ---

> > > >  tools/mkeficapsule.c | 226 ++-----------------------------------------

> > > >  1 file changed, 7 insertions(+), 219 deletions(-)

> > >

> > > Here again I see EFI diverging from the impl in U-Boot. WIth U-Boot

> > > you can add the public key after the build step, e.g. in a key-signing

> > > server. With EFI and this change you will have to rebuild U-Boot (from

> > > source) every time you sign something. Seems like a pain.

> >

> > I don't see why either of this is a problem.  You need the public key to

> > update the binary it self, so rebuilding from source is a prerequisite.

>

> Please can you have a look at binman and the concept of packaging

> separate from building? Rebuilding from source is definitely not

> needed to update a binary.


Sure I'll take a look. We already have an mkeficapsule.c though, which
in theory could take care of the capsule signing.  The point is that
we don't uses that key to sign anything, we use it to authenticate the
capsule that tries to update the firmware.

>

> >

> > Apart from a signing server, you can also have special hardware that provides

> > the public key you need (which is not implemented yet).  So this is the bare

> > minimum functionality you need  for authenticated capsule updates.

>

> As discussed on the mailing list you have not included the motivation

> for this.


To be fair, I did on patch 1/3.

> Now that I understand the motivation, which is to avoid

> someone changing the key at runtime, I believe that this change does

> not actually help...I've replied separately on the mailing list.


It does help, but you need combined code which doesn't exist in either
case.  Anyway, I'll reply on the other thread

Cheers
/Ilias
>

> Regards,

> Simon
Simon Glass July 20, 2021, 6:50 p.m. UTC | #7
Hi Ilias,

On Tue, 20 Jul 2021 at 12:43, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>

> On Tue, 20 Jul 2021 at 21:33, Simon Glass <sjg@chromium.org> wrote:

> >

> > Hi Ilias,

> >

> > On Sat, 17 Jul 2021 at 01:24, Ilias Apalodimas

> > <ilias.apalodimas@linaro.org> wrote:

> > >

> > > On Fri, Jul 16, 2021 at 08:03:23AM -0600, Simon Glass wrote:

> > > > Hi Ilias,

> > > >

> > > > On Thu, 15 Jul 2021 at 11:00, Ilias Apalodimas

> > > > <ilias.apalodimas@linaro.org> wrote:

> > > > >

> > > > > commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb")

> > > > > added a bunch of options enabling the addition of the capsule public key

> > > > > in a dtb.  Since now we embeded the key in U-Boot's .rodata we don't this

> > > > > this functionality anymore

> > > > >

> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> > > > > ---

> > > > >  tools/mkeficapsule.c | 226 ++-----------------------------------------

> > > > >  1 file changed, 7 insertions(+), 219 deletions(-)

> > > >

> > > > Here again I see EFI diverging from the impl in U-Boot. WIth U-Boot

> > > > you can add the public key after the build step, e.g. in a key-signing

> > > > server. With EFI and this change you will have to rebuild U-Boot (from

> > > > source) every time you sign something. Seems like a pain.

> > >

> > > I don't see why either of this is a problem.  You need the public key to

> > > update the binary it self, so rebuilding from source is a prerequisite.

> >

> > Please can you have a look at binman and the concept of packaging

> > separate from building? Rebuilding from source is definitely not

> > needed to update a binary.

>

> Sure I'll take a look. We already have an mkeficapsule.c though, which

> in theory could take care of the capsule signing.  The point is that

> we don't uses that key to sign anything, we use it to authenticate the

> capsule that tries to update the firmware.


That is not the key point IMO :-)

FIT signing works the same way...it is the public key. So I fully
understand that is how it works.

>

> >

> > >

> > > Apart from a signing server, you can also have special hardware that provides

> > > the public key you need (which is not implemented yet).  So this is the bare

> > > minimum functionality you need  for authenticated capsule updates.

> >

> > As discussed on the mailing list you have not included the motivation

> > for this.

>

> To be fair, I did on patch 1/3.


OK I see. Then I believe the motivation is misplaced / incorrect for
reasons mentioned on IRC...you have bigger problems than just the key
in the DT and you yourself mention the power of the command line if
the user has access.

>

> > Now that I understand the motivation, which is to avoid

> > someone changing the key at runtime, I believe that this change does

> > not actually help...I've replied separately on the mailing list.

>

> It does help, but you need combined code which doesn't exist in either

> case.  Anyway, I'll reply on the other thread


I still don't think this helps at all.

Regards,
Simon
diff mbox series

Patch

diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index de0a62898886..214dc38e46e3 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -4,14 +4,12 @@ 
  *		Author: AKASHI Takahiro
  */
 
-#include <errno.h>
 #include <getopt.h>
 #include <malloc.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <unistd.h>
 #include <linux/types.h>
 
 #include <sys/mman.h>
@@ -29,9 +27,6 @@  typedef __s32 s32;
 
 #define aligned_u64 __aligned_u64
 
-#define SIGNATURE_NODENAME	"signature"
-#define OVERLAY_NODENAME	"__overlay__"
-
 #ifndef __packed
 #define __packed __attribute__((packed))
 #endif
@@ -52,9 +47,6 @@  static struct option options[] = {
 	{"raw", required_argument, NULL, 'r'},
 	{"index", required_argument, NULL, 'i'},
 	{"instance", required_argument, NULL, 'I'},
-	{"dtb", required_argument, NULL, 'D'},
-	{"public key", required_argument, NULL, 'K'},
-	{"overlay", no_argument, NULL, 'O'},
 	{"help", no_argument, NULL, 'h'},
 	{NULL, 0, NULL, 0},
 };
@@ -68,187 +60,10 @@  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"
-	       "\t-K, --public-key <key file> public key esl file\n"
-	       "\t-D, --dtb <dtb file>        dtb file\n"
-	       "\t-O, --overlay               the dtb file is an overlay\n"
 	       "\t-h, --help                  print a help message\n",
 	       tool_name);
 }
 
-static int fdt_add_pub_key_data(void *sptr, void *dptr, size_t key_size,
-				bool overlay)
-{
-	int parent;
-	int ov_node;
-	int frag_node;
-	int ret = 0;
-
-	if (overlay) {
-		/*
-		 * The signature would be stored in the
-		 * first fragment node of the overlay
-		 */
-		frag_node = fdt_first_subnode(dptr, 0);
-		if (frag_node == -FDT_ERR_NOTFOUND) {
-			fprintf(stderr,
-				"Couldn't find the fragment node: %s\n",
-				fdt_strerror(frag_node));
-			goto done;
-		}
-
-		ov_node = fdt_subnode_offset(dptr, frag_node, OVERLAY_NODENAME);
-		if (ov_node == -FDT_ERR_NOTFOUND) {
-			fprintf(stderr,
-				"Couldn't find the __overlay__ node: %s\n",
-				fdt_strerror(ov_node));
-			goto done;
-		}
-	} else {
-		ov_node = 0;
-	}
-
-	parent = fdt_subnode_offset(dptr, ov_node, SIGNATURE_NODENAME);
-	if (parent == -FDT_ERR_NOTFOUND) {
-		parent = fdt_add_subnode(dptr, ov_node, SIGNATURE_NODENAME);
-		if (parent < 0) {
-			ret = parent;
-			if (ret != -FDT_ERR_NOSPACE) {
-				fprintf(stderr,
-					"Couldn't create signature node: %s\n",
-					fdt_strerror(parent));
-			}
-		}
-	}
-	if (ret)
-		goto done;
-
-	/* Write the key to the FDT node */
-	ret = fdt_setprop(dptr, parent, "capsule-key",
-			  sptr, key_size);
-
-done:
-	if (ret)
-		ret = ret == -FDT_ERR_NOSPACE ? -ENOSPC : -EIO;
-
-	return ret;
-}
-
-static int add_public_key(const char *pkey_file, const char *dtb_file,
-			  bool overlay)
-{
-	int ret;
-	int srcfd = -1;
-	int destfd = -1;
-	void *sptr = NULL;
-	void *dptr = NULL;
-	off_t src_size;
-	struct stat pub_key;
-	struct stat dtb;
-
-	/* Find out the size of the public key */
-	srcfd = open(pkey_file, O_RDONLY);
-	if (srcfd == -1) {
-		fprintf(stderr, "%s: Can't open %s: %s\n",
-			__func__, pkey_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	ret = fstat(srcfd, &pub_key);
-	if (ret == -1) {
-		fprintf(stderr, "%s: Can't stat %s: %s\n",
-			__func__, pkey_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	src_size = pub_key.st_size;
-
-	/* mmap the public key esl file */
-	sptr = mmap(0, src_size, PROT_READ, MAP_SHARED, srcfd, 0);
-	if (sptr == MAP_FAILED) {
-		fprintf(stderr, "%s: Failed to mmap %s:%s\n",
-			__func__, pkey_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	/* Open the dest FDT */
-	destfd = open(dtb_file, O_RDWR);
-	if (destfd == -1) {
-		fprintf(stderr, "%s: Can't open %s: %s\n",
-			__func__, dtb_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	ret = fstat(destfd, &dtb);
-	if (ret == -1) {
-		fprintf(stderr, "%s: Can't stat %s: %s\n",
-			__func__, dtb_file, strerror(errno));
-		goto err;
-	}
-
-	dtb.st_size += src_size + 0x30;
-	if (ftruncate(destfd, dtb.st_size)) {
-		fprintf(stderr, "%s: Can't expand %s: %s\n",
-			__func__, dtb_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	errno = 0;
-	/* mmap the dtb file */
-	dptr = mmap(0, dtb.st_size, PROT_READ | PROT_WRITE, MAP_SHARED,
-		    destfd, 0);
-	if (dptr == MAP_FAILED) {
-		fprintf(stderr, "%s: Failed to mmap %s:%s\n",
-			__func__, dtb_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	if (fdt_check_header(dptr)) {
-		fprintf(stderr, "%s: Invalid FDT header\n", __func__);
-		ret = -1;
-		goto err;
-	}
-
-	ret = fdt_open_into(dptr, dptr, dtb.st_size);
-	if (ret) {
-		fprintf(stderr, "%s: Cannot expand FDT: %s\n",
-			__func__, fdt_strerror(ret));
-		ret = -1;
-		goto err;
-	}
-
-	/* Copy the esl file to the expanded FDT */
-	ret = fdt_add_pub_key_data(sptr, dptr, src_size, overlay);
-	if (ret < 0) {
-		fprintf(stderr, "%s: Unable to add public key to the FDT\n",
-			__func__);
-		ret = -1;
-		goto err;
-	}
-
-	ret = 0;
-
-err:
-	if (sptr)
-		munmap(sptr, src_size);
-
-	if (dptr)
-		munmap(dptr, dtb.st_size);
-
-	if (srcfd != -1)
-		close(srcfd);
-
-	if (destfd != -1)
-		close(destfd);
-
-	return ret;
-}
-
 static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 			unsigned long index, unsigned long instance)
 {
@@ -366,22 +181,16 @@  err_1:
 int main(int argc, char **argv)
 {
 	char *file;
-	char *pkey_file;
-	char *dtb_file;
 	efi_guid_t *guid;
 	unsigned long index, instance;
 	int c, idx;
-	int ret;
-	bool overlay = false;
 
 	file = NULL;
-	pkey_file = NULL;
-	dtb_file = NULL;
 	guid = NULL;
 	index = 0;
 	instance = 0;
 	for (;;) {
-		c = getopt_long(argc, argv, "f:r:i:I:v:D:K:Oh", options, &idx);
+		c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx);
 		if (c == -1)
 			break;
 
@@ -408,43 +217,22 @@  int main(int argc, char **argv)
 		case 'I':
 			instance = strtoul(optarg, NULL, 0);
 			break;
-		case 'K':
-			if (pkey_file) {
-				printf("Public Key already specified\n");
-				return -1;
-			}
-			pkey_file = optarg;
-			break;
-		case 'D':
-			if (dtb_file) {
-				printf("DTB file already specified\n");
-				return -1;
-			}
-			dtb_file = optarg;
-			break;
-		case 'O':
-			overlay = true;
-			break;
 		case 'h':
 			print_usage();
 			return 0;
 		}
 	}
 
-	/* need a fit image file or raw image file */
-	if (!file && !pkey_file && !dtb_file) {
+	/* need a output file */
+	if (argc != optind + 1) {
 		print_usage();
 		exit(EXIT_FAILURE);
 	}
 
-	if (pkey_file && dtb_file) {
-		ret = add_public_key(pkey_file, dtb_file, overlay);
-		if (ret == -1) {
-			printf("Adding public key to the dtb failed\n");
-			exit(EXIT_FAILURE);
-		} else {
-			exit(EXIT_SUCCESS);
-		}
+	/* need a fit image file or raw image file */
+	if (!file) {
+		print_usage();
+		exit(EXIT_SUCCESS);
 	}
 
 	if (create_fwbin(argv[optind], file, guid, index, instance)