[U-Boot] [PATCH v2 8/8] tools, fit_check_sign: verify a signed fit image

Simon Glass sjg at chromium.org
Sun Feb 16 00:22:14 CET 2014


Hi Heiko,

On 8 February 2014 22:34, Heiko Schocher <hs at denx.de> wrote:
> add host tool "fit_check_sign" which verifies, if a fit image is
> signed correct.
>
> Signed-off-by: Heiko Schocher <hs at denx.de>
> Cc: Simon Glass <sjg at chromium.org>

Overall this seems reasonable, and very useful. I'm just a little
nervous about some of the work-arounds to make the code run on the
host. So a few minor comments.

>
> ---
> - changes for v2:
>   - fixed compile error for sandbox
>   - add fit_check_sign test to test/vboot/vboot_test.sh
>
>  Makefile                     |   1 +
>  common/image-sig.c           |  22 +++++---
>  doc/uImage.FIT/signature.txt |   8 +++
>  include/fdt_support.h        |   5 ++
>  include/image.h              |  16 +++---
>  lib/libfdt/fdt_wip.c         |  17 +++++++
>  lib/rsa/rsa-checksum.c       |  10 ++--
>  lib/rsa/rsa-sign.c           |   2 +-
>  lib/rsa/rsa-verify.c         |  18 +++++--
>  test/vboot/vboot_test.sh     |  36 ++++++++++++-
>  tools/Makefile               |  19 ++++++-
>  tools/fdt_host.h             |   2 +
>  tools/fit_check_sign.c       | 119 +++++++++++++++++++++++++++++++++++++++++++
>  tools/image-host.c           |  13 +++++
>  14 files changed, 265 insertions(+), 23 deletions(-)
>  create mode 100644 tools/fit_check_sign.c
>
> diff --git a/Makefile b/Makefile
> index a2e424d..900d8f7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -794,6 +794,7 @@ clean:
>         @rm -f $(obj)tools/bmp_logo        $(obj)tools/easylogo/easylogo  \
>                $(obj)tools/env/fw_printenv                                \
>                $(obj)tools/envcrc                                         \
> +              $(obj)tools/fit_check_sign                                 \
>                $(obj)tools/fit_info                                       \
>                $(obj)tools/gdb/{gdbcont,gdbsend}                          \
>                $(obj)tools/gen_eth_addr    $(obj)tools/img2srec           \
> diff --git a/common/image-sig.c b/common/image-sig.c
> index 199e634..5b19ea8 100644
> --- a/common/image-sig.c
> +++ b/common/image-sig.c
> @@ -19,6 +19,14 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define IMAGE_MAX_HASHED_NODES         100
>
>  #if defined(CONFIG_FIT_SIGNATURE)
> +
> +#ifdef USE_HOSTCC
> +__attribute__((weak)) void *get_blob(void)
> +{
> +       return NULL;
> +}
> +#endif
> +

Why make this weak? A better name would be something like
image_get_host_blob(). I think we should get a compile error if it is
not defined.


>  struct checksum_algo checksum_algos[] = {
>         {
>                 "sha1",
> @@ -26,10 +34,9 @@ struct checksum_algo checksum_algos[] = {
>                 RSA2048_BYTES,
>  #if IMAGE_ENABLE_SIGN
>                 EVP_sha1,
> -#else
> +#endif
>                 sha1_calculate,
>                 padding_sha1_rsa2048,
> -#endif
>         },
>         {
>                 "sha256",
> @@ -37,10 +44,9 @@ struct checksum_algo checksum_algos[] = {
>                 RSA2048_BYTES,
>  #if IMAGE_ENABLE_SIGN
>                 EVP_sha256,
> -#else
> +#endif
>                 sha256_calculate,
>                 padding_sha256_rsa2048,
> -#endif
>         },
>         {
>                 "sha256",
> @@ -48,10 +54,9 @@ struct checksum_algo checksum_algos[] = {
>                 RSA4096_BYTES,
>  #if IMAGE_ENABLE_SIGN
>                 EVP_sha256,
> -#else
> +#endif
>                 sha256_calculate,
>                 padding_sha256_rsa4096,
> -#endif
>         }
>
>  };
> @@ -462,4 +467,9 @@ int fit_config_verify(const void *fit, int conf_noffset)
>         return !fit_config_verify_required_sigs(fit, conf_noffset,
>                                                 gd_fdt_blob());
>  }
> +#else
> +int fit_config_verify(const void *fit, int conf_noffset)
> +{
> +       return 0;
> +}
>  #endif
> diff --git a/doc/uImage.FIT/signature.txt b/doc/uImage.FIT/signature.txt
> index 5bd229f..171af2a 100644
> --- a/doc/uImage.FIT/signature.txt
> +++ b/doc/uImage.FIT/signature.txt
> @@ -355,7 +355,11 @@ Test Verified Boot Run: signed images: OK
>  Build FIT with signed configuration
>  Test Verified Boot Run: unsigned config: OK
>  Sign images
> +check signed config on the host
> +OK
>  Test Verified Boot Run: signed config: OK
> +check bad signed config on the host
> +OK
>  Test Verified Boot Run: signed config with bad hash: OK
>
>  Test sha1 passed
> @@ -377,7 +381,11 @@ Test Verified Boot Run: signed images: OK
>  Build FIT with signed configuration
>  Test Verified Boot Run: unsigned config: OK
>  Sign images
> +check signed config on the host
> +OK
>  Test Verified Boot Run: signed config: OK
> +check bad signed config on the host
> +OK
>  Test Verified Boot Run: signed config with bad hash: OK
>
>  Test sha256 passed
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 9871e2f..76c9b2e 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -115,4 +115,9 @@ static inline int fdt_status_disabled_by_alias(void *fdt, const char* alias)
>  }
>
>  #endif /* ifdef CONFIG_OF_LIBFDT */
> +
> +#ifdef USE_HOSTCC
> +int fdtdec_get_int(const void *blob, int node, const char *prop_name,
> +               int default_val);
> +#endif
>  #endif /* ifndef __FDT_SUPPORT_H */
> diff --git a/include/image.h b/include/image.h
> index 6e4745a..260a396 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -831,7 +831,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>  #if defined(CONFIG_FIT_SIGNATURE)
>  # ifdef USE_HOSTCC
>  #  define IMAGE_ENABLE_SIGN    1
> -#  define IMAGE_ENABLE_VERIFY  0
> +#  define IMAGE_ENABLE_VERIFY  1
>  # include  <openssl/evp.h>
>  #else
>  #  define IMAGE_ENABLE_SIGN    0
> @@ -843,7 +843,8 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>  #endif
>
>  #ifdef USE_HOSTCC
> -# define gd_fdt_blob()         NULL
> +void *get_blob(void);
> +# define gd_fdt_blob()         get_blob()

image_get_host_blob()

>  #else
>  # define gd_fdt_blob()         (gd->fdt_blob)
>  #endif
> @@ -880,14 +881,11 @@ struct checksum_algo {
>         const int checksum_len;
>         const int pad_len;
>  #if IMAGE_ENABLE_SIGN
> -       const EVP_MD *(*calculate)(void);
> -#else
> -#if IMAGE_ENABLE_VERIFY
> +       const EVP_MD *(*calculate_sign)(void);
> +#endif
>         void (*calculate)(const struct image_region region[],
>                           int region_count, uint8_t *checksum);
>         const uint8_t *rsa_padding;
> -#endif
> -#endif
>  };
>
>  struct image_sig_algo {
> @@ -1008,7 +1006,11 @@ struct image_region *fit_region_make_list(const void *fit,
>
>  static inline int fit_image_check_target_arch(const void *fdt, int node)
>  {
> +#ifndef USE_HOSTCC
>         return fit_image_check_arch(fdt, node, IH_ARCH_DEFAULT);
> +#else
> +       return 0;
> +#endif
>  }
>
>  #ifdef CONFIG_FIT_VERBOSE
> diff --git a/lib/libfdt/fdt_wip.c b/lib/libfdt/fdt_wip.c
> index 3f2dfa5..4babdbc 100644
> --- a/lib/libfdt/fdt_wip.c
> +++ b/lib/libfdt/fdt_wip.c
> @@ -31,6 +31,23 @@ int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
>         return 0;
>  }
>
> +#ifdef USE_HOSTCC
> +int fdtdec_get_int(const void *blob, int node, const char *prop_name,
> +               int default_val)
> +{
> +       const int *cell;
> +       int len;
> +
> +       cell = fdt_getprop_w((void *)blob, node, prop_name, &len);
> +       if (cell && len >= sizeof(int)) {
> +               int val = fdt32_to_cpu(cell[0]);
> +
> +               return val;
> +       }
> +       return default_val;
> +}
> +#endif

I don't think you can put this function here - it is an upstream
library. If you don't want to include fdtdec.c into the hostcc
compilation, then perhaps add this function into a host file
somewhere, with a comment as to why it is here.

> +
>  static void _fdt_nop_region(void *start, int len)
>  {
>         fdt32_t *p;
> diff --git a/lib/rsa/rsa-checksum.c b/lib/rsa/rsa-checksum.c
> index a9d096d..32d6602 100644
> --- a/lib/rsa/rsa-checksum.c
> +++ b/lib/rsa/rsa-checksum.c
> @@ -4,14 +4,18 @@
>   * SPDX-License-Identifier:    GPL-2.0+
>   */
>
> +#ifndef USE_HOSTCC
>  #include <common.h>
>  #include <fdtdec.h>
> -#include <rsa.h>
> -#include <sha1.h>
> -#include <sha256.h>
>  #include <asm/byteorder.h>
>  #include <asm/errno.h>
>  #include <asm/unaligned.h>
> +#else
> +#include "fdt_host.h"
> +#endif
> +#include <rsa.h>
> +#include <sha1.h>
> +#include <sha256.h>
>
>  /* PKCS 1.5 paddings as described in the RSA PKCS#1 v2.1 standard. */
>
> diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
> index 0fe6e9f..ca8c120 100644
> --- a/lib/rsa/rsa-sign.c
> +++ b/lib/rsa/rsa-sign.c
> @@ -193,7 +193,7 @@ static int rsa_sign_with_key(RSA *rsa, struct checksum_algo *checksum_algo,
>                 goto err_create;
>         }
>         EVP_MD_CTX_init(context);
> -       if (!EVP_SignInit(context, checksum_algo->calculate())) {
> +       if (!EVP_SignInit(context, checksum_algo->calculate_sign())) {
>                 ret = rsa_err("Signer setup failed");
>                 goto err_sign;
>         }
> diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
> index 09268ca..587da5b 100644
> --- a/lib/rsa/rsa-verify.c
> +++ b/lib/rsa/rsa-verify.c
> @@ -4,17 +4,28 @@
>   * SPDX-License-Identifier:    GPL-2.0+
>   */
>
> +#ifndef USE_HOSTCC
>  #include <common.h>
>  #include <fdtdec.h>
> -#include <rsa.h>
> -#include <sha1.h>
> -#include <sha256.h>
> +#include <asm/types.h>
>  #include <asm/byteorder.h>
>  #include <asm/errno.h>
> +#include <asm/types.h>
>  #include <asm/unaligned.h>
> +#else
> +#include "fdt_host.h"
> +#include "mkimage.h"
> +#include <fdt_support.h>
> +#endif
> +#include <rsa.h>
> +#include <sha1.h>
> +#include <sha256.h>
>
>  #define UINT64_MULT32(v, multby)  (((uint64_t)(v)) * ((uint32_t)(multby)))
>
> +#define get_unaligned_be32(a) fdt32_to_cpu(*(uint32_t *)a)
> +#define put_unaligned_be32(a, b) (*(uint32_t *)(b) = cpu_to_fdt32(a))
> +
>  /**
>   * subtract_modulus() - subtract modulus from the given value
>   *
> @@ -150,7 +161,6 @@ static int pow_mod(const struct rsa_public_key *key, uint32_t *inout)
>         /* Convert to bigendian byte array */
>         for (i = key->len - 1, ptr = inout; (int)i >= 0; i--, ptr++)
>                 put_unaligned_be32(result[i], ptr);
> -
>         return 0;
>  }
>
> diff --git a/test/vboot/vboot_test.sh b/test/vboot/vboot_test.sh
> index 27b221a..fc33f2d 100755
> --- a/test/vboot/vboot_test.sh
> +++ b/test/vboot/vboot_test.sh
> @@ -55,6 +55,7 @@ O=$(readlink -f ${O})
>  dtc="-I dts -O dtb -p 2000"
>  uboot="${O}/u-boot"
>  mkimage="${O}/tools/mkimage"
> +fit_check_sign="${O}/tools/fit_check_sign"
>  keys="${dir}/dev-keys"
>  echo ${mkimage} -D "${dtc}"
>
> @@ -88,7 +89,6 @@ ${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb -r test.fit >${tmp}
>
>  run_uboot "signed images" "dev+"
>
> -
>  # Create a fresh .dtb without the public keys
>  dtc -p 0x1000 sandbox-u-boot.dts -O dtb -o sandbox-u-boot.dtb
>
> @@ -101,6 +101,23 @@ run_uboot "unsigned config" $sha"+ OK"
>  echo Sign images
>  ${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb -r test.fit >${tmp}
>
> +echo check signed config on the host
> +if ! ${fit_check_sign} -f test.fit -k sandbox-u-boot.dtb >${tmp}; then
> +       echo
> +       echo "Verified boot key check on host failed, output follows:"
> +       cat ${tmp}
> +       false
> +else
> +       if ! grep -q "dev+" ${tmp}; then
> +               echo
> +               echo "Verified boot key check failed, output follows:"
> +               cat ${tmp}
> +               false
> +       else
> +               echo "OK"
> +       fi
> +fi
> +
>  run_uboot "signed config" "dev+"
>
>  # Increment the first byte of the signature, which should cause failure
> @@ -109,6 +126,23 @@ newbyte=$(printf %x $((0x${sig:0:2} + 1)))
>  sig="${newbyte} ${sig:2}"
>  fdtput -t bx test.fit /configurations/conf at 1/signature at 1 value ${sig}
>
> +echo check bad signed config on the host
> +if ${fit_check_sign} -f test.fit -k sandbox-u-boot.dtb >${tmp}; then
> +       echo
> +       echo "Verified boot key check on host failed, output follows:"
> +       cat ${tmp}
> +       false
> +else
> +       if ! grep -q "failed" ${tmp}; then
> +               echo
> +               echo "Verified boot key check failed, output follows:"
> +               cat ${tmp}
> +               false
> +       else
> +               echo "OK"
> +       fi
> +fi
> +
>  run_uboot "signed config with bad hash" "Bad Data Hash"
>
>  popd >/dev/null
> diff --git a/tools/Makefile b/tools/Makefile
> index d079bc9..874705a 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -53,6 +53,7 @@ BIN_FILES-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes$(SFX)
>  BIN_FILES-y += dumpimage$(SFX)
>  BIN_FILES-y += mkenvimage$(SFX)
>  BIN_FILES-y += mkimage$(SFX)
> +BIN_FILES-y += fit_check_sign$(SFX)
>  BIN_FILES-y += fit_info$(SFX)
>  BIN_FILES-$(CONFIG_EXYNOS5250) += mk$(BOARD)spl$(SFX)
>  BIN_FILES-$(CONFIG_EXYNOS5420) += mk$(BOARD)spl$(SFX)
> @@ -86,6 +87,7 @@ NOPED_OBJ_FILES-y += kwbimage.o
>  NOPED_OBJ_FILES-y += imagetool.o
>  NOPED_OBJ_FILES-y += mkenvimage.o
>  NOPED_OBJ_FILES-y += mkimage.o
> +NOPED_OBJ_FILES-y += fit_check_sign.o
>  NOPED_OBJ_FILES-y += fit_info.o
>  NOPED_OBJ_FILES-y += mxsimage.o
>  NOPED_OBJ_FILES-y += omapimage.o
> @@ -122,7 +124,7 @@ LIBFDT_OBJ_FILES-y += fdt_strerror.o
>  LIBFDT_OBJ_FILES-y += fdt_wip.o
>
>  # RSA objects
> -RSA_OBJ_FILES-$(CONFIG_FIT_SIGNATURE) += rsa-sign.o
> +RSA_OBJ_FILES-$(CONFIG_FIT_SIGNATURE) += rsa-sign.o rsa-verify.o rsa-checksum.o
>
>  # Generated LCD/video logo
>  LOGO_H = $(OBJTREE)/include/bmp_logo.h
> @@ -266,6 +268,21 @@ $(obj)mkimage$(SFX):       $(obj)aisimage.o \
>         $(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^ $(HOSTLIBS)
>         $(HOSTSTRIP) $@
>
> +$(obj)fit_check_sign$(SFX):    $(obj)fit_check_sign.o \
> +                       $(FIT_SIG_OBJS) \
> +                       $(obj)crc32.o \
> +                       $(obj)fit_common.o \
> +                       $(obj)image-fit.o \
> +                       $(obj)image-host.o \
> +                       $(obj)image.o \
> +                       $(obj)md5.o \
> +                       $(obj)sha1.o \
> +                       $(obj)sha256.o \
> +                       $(LIBFDT_OBJS) \
> +                       $(RSA_OBJS)
> +       $(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^ $(HOSTLIBS)
> +       $(HOSTSTRIP) $@
> +
>  $(obj)fit_info$(SFX):  $(obj)fit_info.o \
>                         $(FIT_SIG_OBJS) \
>                         $(obj)crc32.o \
> diff --git a/tools/fdt_host.h b/tools/fdt_host.h
> index c2b23c6..134d965 100644
> --- a/tools/fdt_host.h
> +++ b/tools/fdt_host.h
> @@ -11,4 +11,6 @@
>  #include "../include/libfdt.h"
>  #include "../include/fdt_support.h"
>
> +int fit_check_sign(const void *working_fdt, const void *key);
> +
>  #endif /* __FDT_HOST_H__ */
> diff --git a/tools/fit_check_sign.c b/tools/fit_check_sign.c
> new file mode 100644
> index 0000000..63c7295
> --- /dev/null
> +++ b/tools/fit_check_sign.c
> @@ -0,0 +1,119 @@
> +/*
> + * (C) Copyright 2014
> + * DENX Software Engineering
> + * Heiko Schocher <hs at denx.de>
> + *
> + * Based on:
> + * (C) Copyright 2008 Semihalf
> + *
> + * (C) Copyright 2000-2004
> + * DENX Software Engineering
> + * Wolfgang Denk, wd at denx.de
> + *
> + * Updated-by: Prafulla Wadaskar <prafulla at marvell.com>
> + *             FIT image specific code abstracted from mkimage.c
> + *             some functions added to address abstraction
> + *
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include "mkimage.h"
> +#include "fit_common.h"
> +#include <image.h>
> +#include <u-boot/crc.h>
> +
> +void *key_blob;
> +
> +void usage(char *cmdname)
> +{
> +       fprintf(stderr, "Usage: %s -f fit file -k key file\n"
> +                        "          -f ==> set fit file which should be checked'\n"
> +                        "          -k ==> set key file which contains the key'\n",
> +               cmdname);
> +       exit(EXIT_FAILURE);
> +}
> +
> +void *get_blob(void)
> +{
> +       return key_blob;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +       int ffd = -1;
> +       int kfd = -1;
> +       struct stat fsbuf;
> +       struct stat ksbuf;
> +       void *fit_blob;
> +       char *fdtfile = NULL;
> +       char *keyfile = NULL;
> +       char cmdname[50];
> +       int ret;
> +
> +       strcpy(cmdname, *argv);
> +
> +       while (--argc > 0 && **++argv == '-') {
> +               while (*++*argv) {

If this is a new tool, shouldn't we use getopt()?

> +                       switch (**argv) {
> +                       case 'f':
> +                               if (--argc <= 0)
> +                                       usage(cmdname);
> +                               fdtfile = *++argv;
> +                               goto NXTARG;
> +
> +                       case 'k':
> +                               if (--argc <= 0)
> +                                       usage(cmdname);
> +                               keyfile = *++argv;
> +                               goto NXTARG;
> +
> +                       default:
> +                               usage(cmdname);
> +                       }
> +               }
> +NXTARG:;
> +       }
> +
> +       if (argc != 0)
> +               usage(cmdname);
> +
> +       ffd = mmap_fdt(cmdname, fdtfile, &fit_blob, &fsbuf);
> +       kfd = mmap_fdt(cmdname, keyfile, &key_blob, &ksbuf);
> +
> +       ret = fit_check_sign(fit_blob, key_blob);
> +
> +       if (ret)
> +               ret = EXIT_SUCCESS;
> +       else
> +               ret = EXIT_FAILURE;
> +
> +       (void) munmap((void *)fit_blob, fsbuf.st_size);
> +       (void) munmap((void *)key_blob, ksbuf.st_size);
> +

from here:

> +       /* We're a bit of paranoid */
> +#if defined(_POSIX_SYNCHRONIZED_IO) && \
> +       !defined(__sun__) && \
> +       !defined(__FreeBSD__) && \
> +       !defined(__APPLE__)
> +       (void) fdatasync(ffd);
> +       (void) fdatasync(kfd);
> +#else
> +       (void) fsync(ffd);
> +       (void) fsync(kfd);
> +#endif

I don't think we need this since we are not changing the file.

> +
> +       if (close(ffd)) {
> +               fprintf(stderr, "%s: Write error on %s: %s\n",
> +                       cmdname, fdtfile, strerror(errno));
> +               exit(EXIT_FAILURE);
> +       }
> +       if (close(kfd)) {
> +               fprintf(stderr, "%s: Write error on %s: %s\n",
> +                       cmdname, keyfile, strerror(errno));
> +               exit(EXIT_FAILURE);
> +       }

We are not writing, I think we can ignore errors on close().

> +
> +       exit(ret);
> +}
> diff --git a/tools/image-host.c b/tools/image-host.c
> index 8e185ec..65e3932 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -695,3 +695,16 @@ int fit_add_verification_data(const char *keydir, void *keydest, void *fit,
>
>         return 0;
>  }
> +
> +int fit_check_sign(const void *working_fdt, const void *key)
> +{
> +       int cfg_noffset;
> +       int ret;
> +
> +       cfg_noffset = fit_conf_get_node(working_fdt, NULL);
> +       if (!cfg_noffset)
> +               return -1;
> +
> +       ret = fit_config_verify(working_fdt, cfg_noffset);
> +       return ret;
> +}
> --
> 1.8.3.1
>

Regards,
Simon


More information about the U-Boot mailing list