[PATCH u-boot v2019.04-aspeed-openbmc 2/6] image: Control FIT uImage signature verification at runtime

[Joel Stanley]

On Mon, 31 Jan 2022 at 01:26, Andrew Jeffery <andrew at aj.id.au> wrote:
>
> Some platform designs include support for disabling secure-boot via a
> jumper on the board. Sometimes this control can be separate from the
> mechanism enabling the root-of-trust for the platform. Add support for
> this latter scenario by allowing boards to implement
> board_fit_image_require_verfied(), which is then invoked in the usual
> FIT verification paths.
>
> Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> ---
>  Kconfig            |  9 +++++++++
>  common/image-fit.c | 17 +++++++++++++++--
>  include/image.h    |  9 +++++++++
>  3 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/Kconfig b/Kconfig
> index c3dfa8de47c8..11f796035ae4 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -322,6 +322,15 @@ config FIT_SIGNATURE
>           format support in this case, enable it using
>           CONFIG_IMAGE_FORMAT_LEGACY.
>
> +if FIT_SIGNATURE
> +config FIT_RUNTIME_SIGNATURE
> +       bool "Control verification of FIT uImages at runtime"

Can you follow the pattern of the other FIT_ options by making this
depends on FIT_SIGNATURE?

> +       help
> +         This option allows board support to disable verification of
> +         signatures at runtime, for example through the state of a GPIO.
> +endif # FIT_SIGNATURE
> +
> +
>  config FIT_SIGNATURE_MAX_SIZE
>         hex "Max size of signed FIT structures"
>         depends on FIT_SIGNATURE
> diff --git a/common/image-fit.c b/common/image-fit.c
> index 3c8667f93de2..eb1e66b02b68 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -1199,6 +1199,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data,
>         return 0;
>  }
>
> +#ifndef __weak
> +#define __weak
> +#endif

Shouldn't we always get this from linux/compiler.h?

> +__weak int board_fit_image_require_verified(void)
> +{
> +       return 1;
> +}
> +
>  int fit_image_verify_with_data(const void *fit, int image_noffset,
>                                const void *data, size_t size)
>  {
> @@ -1209,6 +1217,7 @@ int fit_image_verify_with_data(const void *fit, int image_noffset,
>
>         /* Verify all required signatures */
>         if (IMAGE_ENABLE_VERIFY &&
> +           fit_image_require_verified() &&
>             fit_image_verify_required_sigs(fit, image_noffset, data, size,
>                                            gd_fdt_blob(), &verify_all)) {
>                 err_msg = "Unable to verify required signature";
> @@ -1230,7 +1239,9 @@ int fit_image_verify_with_data(const void *fit, int image_noffset,
>                                                  &err_msg))
>                                 goto error;
>                         puts("+ ");
> -               } else if (IMAGE_ENABLE_VERIFY && verify_all &&
> +               } else if (IMAGE_ENABLE_VERIFY &&
> +                               fit_image_require_verified() &&
> +                               verify_all &&

reading through this it's quite confusing.

We have IMAGE_ENABLE_VERIFY, a compile time constant that will be true
if CONFIG_FIT_SIGNATURE is enabled.

We're adding a function that will override this.

So we could have a function:

__weak bool fit_enable_verification(void)
{
   return IMAGE_ENABLE_VERIFY;
}

The downside of this would be if a board were to implement this but
not have FIT_SIGNATURE enabled then they could return true when they
shouldn't. You could go back to this:

static bool fit_enable_verification(void)
{
   return IMAGE_ENABLE_VERIFY && board_fit_image_require_verified();
}

And drop the ifdefs from image.h

>                                 !strncmp(name, FIT_SIG_NODENAME,
>                                         strlen(FIT_SIG_NODENAME))) {
>                         ret = fit_image_check_sig(fit, noffset, data,
> @@ -1849,7 +1860,9 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>                 if (image_type == IH_TYPE_KERNEL)
>                         images->fit_uname_cfg = fit_base_uname_config;
>
> -               if (IMAGE_ENABLE_VERIFY && images->verify) {
> +               if (IMAGE_ENABLE_VERIFY &&
> +                               fit_image_require_verified() &&
> +                               images->verify) {
>                         puts("   Verifying Hash Integrity ... ");
>                         if (fit_config_verify(fit, cfg_noffset)) {
>                                 puts("Bad Data Hash\n");
> diff --git a/include/image.h b/include/image.h
> index 937c7eee8ffb..19ea743af08f 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1103,6 +1103,15 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>  # define IMAGE_ENABLE_VERIFY   0
>  #endif
>
> +/*
> + * Further, allow run-time control of verification, e.g. via a jumper
> + */
> +#if defined(CONFIG_FIT_RUNTIME_SIGNATURE)
> +# define fit_image_require_verified()  board_fit_image_require_verified()
> +#else
> +# define fit_image_require_verified()  IMAGE_ENABLE_VERIFY
> +#endif
> +
>  #ifdef USE_HOSTCC
>  void *image_get_host_blob(void);
>  void image_set_host_blob(void *host_blob);
> --
> 2.32.0
>