[PATCH 1/2] tools: add fdt_add_pubkey
Roman Kopytin
Roman.Kopytin at kaspersky.com
Wed Nov 10 08:03:59 CET 2021
Hi, Simon
I have question about:
Some of these are not optional so should not have [] around them.
As I see we have defaults for:
static const char *algo_name = "sha1,rsa2048"; /* -a <algo> */
static const char *keydir = "."; /* -k <keydir> */
static const char *keyname = "key"; /* -n <keyname> */
It means that we can skip it in command line. Should I need to remove [] from code for those parameters?
-----Original Message-----
From: Simon Glass <sjg at chromium.org>
Sent: Wednesday, November 10, 2021 3:58 AM
To: Roman Kopytin <Roman.Kopytin at kaspersky.com>
Cc: u-boot at lists.denx.de; Rasmus Villemoes <rasmus.villemoes at prevas.dk>
Subject: Re: [PATCH 1/2] tools: add fdt_add_pubkey
Hi Roman,
On Mon, 8 Nov 2021 at 08:29, Roman Kopytin <Roman.Kopytin at kaspersky.com> wrote:
>
> Having to use the -K option to mkimage to populate U-Boot's .dtb with
> the public key while signing the kernel FIT image is often a little
> awkward. In particular, when using a meta-build system such as
> bitbake/Yocto, having the tasks of the kernel and U-Boot recipes
> intertwined, modifying deployed artifacts and rebuilding U-Boot with
> an updated .dtb is quite cumbersome. Also, in some scenarios one may
> wish to build U-Boot complete with the public key(s) embedded in the
> .dtb without the corresponding private keys being present on the same
> build host.
>
> So this adds a simple tool that allows one to disentangle the kernel
> and U-Boot builds, by simply copy-pasting just enough of the mkimage
> code to allow one to add a public key to a .dtb. When using mkimage,
> some of the information is taken from the .its used to build the
> kernel (algorithm and key name), so that of course needs to be
> supplied on the command line.
>
> Signed-off-by: Roman Kopytin <Roman.Kopytin at kaspersky.com>
> Cc: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> ---
> tools/.gitignore | 1 +
> tools/Makefile | 3 ++
> tools/fdt_add_pubkey.c | 97
> ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 101 insertions(+)
> create mode 100755 tools/fdt_add_pubkey.c
>
> diff --git a/tools/.gitignore b/tools/.gitignore index
> a88453f64d..f312b760e4 100644
> --- a/tools/.gitignore
> +++ b/tools/.gitignore
> @@ -6,6 +6,7 @@
> /dumpimage
> /easylogo/easylogo
> /envcrc
> +/fdt_add_pubkey
> /fdtgrep
> /file2include
> /fit_check_sign
> diff --git a/tools/Makefile b/tools/Makefile index
> 4a86321f64..44f25dda18 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -73,6 +73,7 @@ mkenvimage-objs := mkenvimage.o os_support.o
> lib/crc32.o
>
> hostprogs-y += dumpimage mkimage
> hostprogs-$(CONFIG_TOOLS_LIBCRYPTO) += fit_info fit_check_sign
> +hostprogs-$(CONFIG_TOOLS_LIBCRYPTO) += fdt_add_pubkey
>
> hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include
>
> @@ -153,6 +154,7 @@ dumpimage-objs := $(dumpimage-mkimage-objs) dumpimage.o
> mkimage-objs := $(dumpimage-mkimage-objs) mkimage.o
> fit_info-objs := $(dumpimage-mkimage-objs) fit_info.o
> fit_check_sign-objs := $(dumpimage-mkimage-objs) fit_check_sign.o
> +fdt_add_pubkey-objs := $(dumpimage-mkimage-objs) fdt_add_pubkey.o
> file2include-objs := file2include.o
>
> ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),)
> @@ -190,6 +192,7 @@ HOSTCFLAGS_fit_image.o += -DMKIMAGE_DTC=\"$(CONFIG_MKIMAGE_DTC_PATH)\"
> HOSTLDLIBS_dumpimage := $(HOSTLDLIBS_mkimage) HOSTLDLIBS_fit_info :=
> $(HOSTLDLIBS_mkimage) HOSTLDLIBS_fit_check_sign :=
> $(HOSTLDLIBS_mkimage)
> +HOSTLDLIBS_fdt_add_pubkey := $(HOSTLDLIBS_mkimage)
>
> hostprogs-$(CONFIG_EXYNOS5250) += mkexynosspl
> hostprogs-$(CONFIG_EXYNOS5420) += mkexynosspl diff --git
> a/tools/fdt_add_pubkey.c b/tools/fdt_add_pubkey.c new file mode 100755
> index 0000000000..9306ecedd1
> --- /dev/null
> +++ b/tools/fdt_add_pubkey.c
> @@ -0,0 +1,97 @@
> +#include <image.h>
> +#include "fit_common.h"
> +
> +static const char *cmdname;
> +
> +static const char *algo_name = "sha1,rsa2048"; /* -a <algo> */ static
> +const char *keydir = "."; /* -k <keydir> */ static const char
> +*keyname = "key"; /* -n <keyname> */ static const char *require_keys;
> +/* -r <conf|image> */ static const char *keydest; /* argv[n] */
> +
> +static void usage(const char *msg)
> +{
> + fprintf(stderr, "Error: %s\n", msg);
> + fprintf(stderr, "Usage: %s [-a <algo>] [-k <keydir>] [-n
> +<keyname>] [-r <conf|image>] <fdt blob>\n",
Some of these are not optional so should not have [] around them.
> + cmdname);
> + exit(EXIT_FAILURE);
> +}
> +
> +static void process_args(int argc, char *argv[]) {
> + int opt;
> +
> + while((opt = getopt(argc, argv, "a:k:n:r:")) != -1) {
> + switch (opt) {
> + case 'k':
> + keydir = optarg;
> + break;
> + case 'a':
> + algo_name = optarg;
> + break;
> + case 'n':
> + keyname = optarg;
> + break;
> + case 'r':
> + require_keys = optarg;
> + break;
> + default:
> + usage("Invalid option");
> + }
> + }
> + /* The last parameter is expected to be the .dtb to add the public key to */
> + if (optind < argc)
> + keydest = argv[optind];
> +
> + if (!keydest)
> + usage("Missing dtb file to update"); }
> +
> +int main(int argc, char *argv[])
> +{
> + struct image_sign_info info;
> + int destfd, ret;
> + void *dest_blob = NULL;
> + struct stat dest_sbuf;
> + size_t size_inc = 0;
> +
> + cmdname = argv[0];
> +
> + process_args(argc, argv);
> +
> + memset(&info, 0, sizeof(info));
'\0'
> +
> + info.keydir = keydir;
> + info.keyname = keyname;
> + info.name = algo_name;
> + info.require_keys = require_keys;
> + info.crypto = image_get_crypto_algo(algo_name);
> + if (!info.crypto) {
> + fprintf(stderr, "Unsupported signature algorithm '%s'\n", algo_name);
> + exit(EXIT_FAILURE);
> + }
Can you please put the block above and the loop below into a separate function that returns an error code? Then you can print that out at the bottom, with a single EXIT_FAILURE.
> +
> + while (1) {
> + destfd = mmap_fdt(cmdname, keydest, size_inc, &dest_blob, &dest_sbuf, false, false);
> + if (destfd < 0)
> + exit(EXIT_FAILURE);
> +
> + ret = info.crypto->add_verify_data(&info, dest_blob);
> +
> + munmap(dest_blob, dest_sbuf.st_size);
> + close(destfd);
> + if (!ret || ret != -ENOSPC)
> + break;
> + fprintf(stderr, ".dtb too small, increasing size by
> + 1024 bytes\n");
debug() I think. This isn't very important. BTW I found that 512 bytes is enough, if you want to use that, but 1024 is fine too.
> + size_inc = 1024;
> + }
> +
> + if (ret) {
> + fprintf(stderr, "%s: Cannot add public key to FIT blob: %s\n",
> + cmdname, strerror(-ret));
> + exit(EXIT_FAILURE);
> + }
> +
> + exit(EXIT_SUCCESS);
> +}
> +
> --
> 2.25.1
>
Regards,
Simon
More information about the U-Boot
mailing list