Re: [RFC PATCH v3 4/8] boot: image: add a stage pre-load

[Simon Glass <sjg@chromium.org>]

Hi Philippe,

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This commit adds a stage pre-load that could
> check or modify an image.
>
> For the moment, only a header with a signature is
> supported. This header has this format:
> - magic : 4 bytes
> - image size : 4 bytes
> - signature : n bytes
> - padding : up to header size
>
> The stage use a node /image/pre-load/sig to
> get some information:
> - header-size (mandatory) : size of the header
> - algo-name (mandatory) : name of the algo used to sign
> - padding-name : name of padding used to sign
> - signature-size : size of the signature (in the header)
> - mandatory : set to yes if this sig is mandatory
> - public-key (madatory) : value of the public key
>
> Before running the image, the stage pre-load check
> the signature provided in the header.
>
> This is an initial support, later we could add the
> support of:
> - ciphering
> - uncompressing
> - ...
>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  boot/Kconfig          |  33 +++++
>  boot/Makefile         |   1 +
>  boot/image-pre-load.c | 291 ++++++++++++++++++++++++++++++++++++++++++
>  include/image.h       |   9 ++
>  4 files changed, 334 insertions(+)
>  create mode 100644 boot/image-pre-load.c

Reviewed-by: Simon Glass <sjg@chromium.org>

Please see below.

>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index d3a12be228..3856580af6 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -958,6 +958,39 @@ config AUTOBOOT_MENU_SHOW
>
>  endmenu
>
> +menu "Image support"
> +
> +config IMAGE_PRE_LOAD
> +       bool "Image pre-load support"
> +       help
> +         Enable image pre-load support
> +
> +config SPL_IMAGE_PRE_LOAD
> +       bool "Image pre-load support within SPL"
> +       depends on SPL && IMAGE_PRE_LOAD
> +       help
> +         Enable image pre-load support in SPL

Please expand all your help

> +
> +config IMAGE_PRE_LOAD_SIG
> +       bool "Image pre-load signature support"
> +       depends on IMAGE_PRE_LOAD
> +       select FIT_SIGNATURE
> +       select RSA
> +       select RSA_VERIFY_WITH_PKEY
> +       help
> +         Enable image pre-load signature support
> +
> +config SPL_IMAGE_PRE_LOAD_SIG
> +       bool "Image pre-load signature support witin SPL"
> +       depends on SPL_IMAGE_PRE_LOAD && IMAGE_PRE_LOAD_SIG
> +       select SPL_FIT_SIGNATURE
> +       select SPL_RSA
> +       select SPL_RSA_VERIFY_WITH_PKEY
> +       help
> +         Enable image pre-load signature support in SPL
> +
> +endmenu
> +
>  config USE_BOOTARGS
>         bool "Enable boot arguments"
>         help
> diff --git a/boot/Makefile b/boot/Makefile
> index 2938c3f145..59752c65ca 100644
> --- a/boot/Makefile
> +++ b/boot/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
>  obj-$(CONFIG_$(SPL_)MULTI_DTB_FIT) += boot_fit.o common_fit.o
> +obj-$(CONFIG_$(SPL_TPL_)IMAGE_PRE_LOAD) += image-pre-load.o
>  obj-$(CONFIG_$(SPL_TPL_)IMAGE_SIGN_INFO) += image-sig.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += image-fit-sig.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT_CIPHER) += image-cipher.o
> diff --git a/boot/image-pre-load.c b/boot/image-pre-load.c
> new file mode 100644
> index 0000000000..6ed21c3f51
> --- /dev/null
> +++ b/boot/image-pre-load.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Philippe Reynes <philippe.reynes@softathome.com>
> + */
> +
> +#include <common.h>
> +#include <asm/global_data.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +#include <image.h>
> +#include <mapmem.h>
> +
> +#define IMAGE_PRE_LOAD_SIG_MAGIC               0x55425348
> +#define IMAGE_PRE_LOAD_SIG_OFFSET_MAGIC                0
> +#define IMAGE_PRE_LOAD_SIG_OFFSET_IMG_LEN      4
> +#define IMAGE_PRE_LOAD_SIG_OFFSET_SIG          8
> +
> +#define IMAGE_PRE_LOAD_PATH                    "/image/pre-load/sig"
> +#define IMAGE_PRE_LOAD_PROP_HEADER_SIZE                "header-size"
> +#define IMAGE_PRE_LOAD_PROP_ALGO_NAME          "algo-name"
> +#define IMAGE_PRE_LOAD_PROP_PADDING_NAME       "padding-name"
> +#define IMAGE_PRE_LOAD_PROP_SIG_SIZE           "signature-size"
> +#define IMAGE_PRE_LOAD_PROP_PUBLIC_KEY         "public-key"
> +#define IMAGE_PRE_LOAD_PROP_MANDATORY          "mandatory"
> +
> +#ifndef CONFIG_SYS_BOOTM_LEN
> +/* use 8MByte as default max gunzip size */
> +#define CONFIG_SYS_BOOTM_LEN   0x800000
> +#endif
> +
> +struct image_sig_header {
> +       u32 magic;
> +       u32 size;
> +       u8 *sig;
> +};
> +
> +struct image_sig_info {
> +       ulong header_size;
> +       char *algo_name;
> +       char *padding_name;
> +       u8 *key;
> +       int key_len;
> +       u32 sig_size;
> +       int mandatory;

comments

> +};
> +
> +ulong image_load_offset;
> +
> +/*
> + * This function gathers information about the signature check
> + * that could be done before launching the image.
> + *
> + * return:
> + * -1 => an error has occurred

Can you use -Exxx instead?

> + *  0 => OK
> + *  1 => no setup
> + */
> +static int image_pre_load_sig_setup(struct image_sig_info *info)
> +{
> +       const void *algo_name, *padding_name, *key, *mandatory;
> +       const u32 *header_size, *sig_size;
> +       int key_len;
> +       int node, ret = 0;
> +
> +       if (!info) {
> +               printf("ERROR: info is NULL for image pre-load sig check\n");

log_err() ?

> +               ret = -1;
> +               goto out;
> +       }
> +
> +       memset(info, 0, sizeof(*info));
> +
> +       node = fdt_path_offset(gd_fdt_blob(), IMAGE_PRE_LOAD_PATH);
> +       if (node < 0) {
> +               printf("INFO: no info for image pre-load sig check\n");
> +               ret = 1;
> +               goto out;
> +       }
> +
> +       header_size = fdt_getprop(gd_fdt_blob(), node,
> +                                 IMAGE_PRE_LOAD_PROP_HEADER_SIZE, NULL);
> +       if (!header_size) {
> +               printf("ERROR: no header-size for image pre-load sig check\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       algo_name = fdt_getprop(gd_fdt_blob(), node,
> +                               IMAGE_PRE_LOAD_PROP_ALGO_NAME, NULL);
> +       if (!algo_name) {
> +               printf("ERROR: no algo_name for image pre-load sig check\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       padding_name = fdt_getprop(gd_fdt_blob(), node,
> +                                  IMAGE_PRE_LOAD_PROP_PADDING_NAME, NULL);
> +       if (!padding_name) {
> +               printf("INFO: no padding_name provided, so using pkcs-1.5\n");
> +               padding_name = "pkcs-1.5";
> +       }
> +
> +       sig_size = fdt_getprop(gd_fdt_blob(), node,
> +                              IMAGE_PRE_LOAD_PROP_SIG_SIZE, NULL);
> +       if (!sig_size) {
> +               printf("ERROR: no signature-size for image pre-load sig check\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       key = fdt_getprop(gd_fdt_blob(), node,
> +                         IMAGE_PRE_LOAD_PROP_PUBLIC_KEY, &key_len);
> +       if (!key) {
> +               printf("ERROR: no key for image pre-load sig check\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       info->header_size       = fdt32_to_cpu(*header_size);
> +       info->algo_name         = (char *)algo_name;
> +       info->padding_name      = (char *)padding_name;
> +       info->key               = (uint8_t *)key;
> +       info->key_len           = key_len;
> +       info->sig_size          = fdt32_to_cpu(*sig_size);
> +
> +       mandatory = fdt_getprop(gd_fdt_blob(), node,
> +                               IMAGE_PRE_LOAD_PROP_MANDATORY, NULL);
> +       if (mandatory && !strcmp((char *)mandatory, "yes"))
> +               info->mandatory = 1;
> +
> + out:
> +       return ret;
> +}
> +
> +static int image_pre_load_sig_get_header_u32(struct image_sig_info *info,
> +                                            ulong addr,  u32 offset,
> +                                            u32 *value)
> +{
> +       void *header;
> +       u32 *tmp;
> +       int ret = 0;
> +
> +       header = map_sysmem(addr, info->header_size);
> +       if (!header) {
> +               printf("ERROR: can't map header image pre-load sig\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       tmp = header + offset;
> +       *value = be32_to_cpu(*tmp);
> +
> +       unmap_sysmem(header);
> +
> + out:
> +       return ret;
> +}
> +
> +static int image_pre_load_sig_get_magic(struct image_sig_info *info,
> +                                       ulong addr, u32 *magic)
> +{
> +       int ret;
> +
> +       ret = image_pre_load_sig_get_header_u32(info, addr,
> +                                               IMAGE_PRE_LOAD_SIG_OFFSET_MAGIC, magic);
> +
> +       return ret;
> +}
> +
> +static int image_pre_load_sig_get_img_len(struct image_sig_info *info,
> +                                         ulong addr, u32 *len)
> +{
> +       int ret;
> +
> +       ret = image_pre_load_sig_get_header_u32(info, addr,
> +                                               IMAGE_PRE_LOAD_SIG_OFFSET_IMG_LEN, len);
> +       if (ret < 0)
> +               goto out;
> +
> +       if (*len > CONFIG_SYS_BOOTM_LEN) {
> +               printf("ERROR: size of image (%u) bigger than CONFIG_SYS_BOOTM_LEN (%u)\n",
> +                      *len, CONFIG_SYS_BOOTM_LEN);
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       if (*len == 0) {
> +               printf("ERROR: size of image (%u) is zero\n", *len);
> +               ret = -1;
> +               goto out;
> +       }
> +
> + out:
> +       return ret;
> +}
> +
> +static int image_pre_load_sig_check(struct image_sig_info *info, ulong addr, int img_len)
> +{
> +       void *image;
> +       struct image_sign_info sig_info;
> +       struct image_region reg;
> +       u32 sig_len;
> +       u8 *sig;
> +       int ret = 0;
> +
> +       image = (void *)map_sysmem(addr, info->header_size + img_len);
> +       if (!image) {
> +               printf("ERROR: can't map full image\n");
> +               ret = -1;

Please use real error codes throughout.

> +               goto out;
> +       }
> +
> +       memset(&sig_info, 0, sizeof(sig_info));

'\0'

> +       sig_info.name = info->algo_name;
> +       sig_info.padding = image_get_padding_algo(info->padding_name);
> +       sig_info.checksum = image_get_checksum_algo(sig_info.name);
> +       sig_info.crypto = image_get_crypto_algo(sig_info.name);
> +       sig_info.key = info->key;
> +       sig_info.keylen = info->key_len;
> +
> +       reg.data = image + info->header_size;
> +       reg.size = img_len;
> +
> +       sig = (uint8_t *)image + IMAGE_PRE_LOAD_SIG_OFFSET_SIG;
> +       sig_len = info->sig_size;
> +
> +       ret = sig_info.crypto->verify(&sig_info, &reg, 1, sig, sig_len);
> +       if (ret) {
> +               printf("ERROR: signature check has failed (err=%d)\n", ret);
> +               ret = -1;
> +               goto out_unmap;
> +       }
> +
> +       printf("INFO: signature check has succeed\n");
> +
> +out_unmap:
> +       unmap_sysmem(image);
> +
> + out:
> +       return ret;
> +}
> +
> +int image_pre_load_sig(ulong addr)
> +{
> +       struct image_sig_info info;
> +       u32 magic, img_len;
> +       int ret;
> +
> +       ret = image_pre_load_sig_setup(&info);
> +       if (ret < 0)
> +               goto out;
> +       if (ret > 0) {
> +               ret = 0;
> +               goto out;
> +       }
> +
> +       ret = image_pre_load_sig_get_magic(&info, addr, &magic);
> +       if (ret < 0)
> +               goto out;
> +
> +       if (magic != IMAGE_PRE_LOAD_SIG_MAGIC) {
> +               if (info.mandatory) {
> +                       printf("ERROR: signature is mandatory\n");
> +                       ret = -1;
> +               }
> +               goto out;
> +       }
> +
> +       ret = image_pre_load_sig_get_img_len(&info, addr, &img_len);
> +       if (ret < 0)
> +               goto out;
> +
> +       ret = image_pre_load_sig_check(&info, addr, img_len);
> +
> +       if (!ret)
> +               image_load_offset += info.header_size;
> +
> + out:
> +       return ret;
> +}
> +
> +int image_pre_load(ulong addr)
> +{
> +       int ret = 0;
> +
> +       image_load_offset = 0;
> +
> +       if (CONFIG_IS_ENABLED(IMAGE_PRE_LOAD_SIG))
> +               ret = image_pre_load_sig(addr);
> +
> +       return ret;
> +}
> diff --git a/include/image.h b/include/image.h
> index fd662e74b4..5f83e4c747 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -48,6 +48,7 @@ struct fdt_region;
>  extern ulong image_load_addr;          /* Default Load Address */
>  extern ulong image_save_addr;          /* Default Save Address */
>  extern ulong image_save_size;          /* Default Save Size */
> +extern ulong image_load_offset;        /* Default Load Address Offset */
>
>  /* An invalid size, meaning that the image size is not known */
>  #define IMAGE_SIZE_INVAL       (-1UL)
> @@ -1289,6 +1290,14 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name);
>   */
>  struct padding_algo *image_get_padding_algo(const char *name);
>
> +/**
> + * image_pre_load() - Manage pre load header
> + *

??

Please add a description here.

> + * @param addr         Address of the image
> + * @return: 0 on success, -ve on error
> + */
> +int image_pre_load(ulong addr);
> +
>  /**
>   * fit_image_verify_required_sigs() - Verify signatures marked as 'required'
>   *
> --
> 2.17.1
>

Regards,
Simon