[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