[RFC PATCH 1/1] image: add anti rollback protection for FIT Images

Simon Glass sjg at chromium.org
Mon Sep 7 03:43:44 CEST 2020


Hi Thirupathaiah,

On Tue, 1 Sep 2020 at 14:48, Thirupathaiah Annapureddy
<thiruan at linux.microsoft.com> wrote:
>
> Anti rollback protection is required when there is a need to retire
> previous versions of FIT images due to security flaws in them.
> Currently U-Boot Verified boot does not have rollback protection to
> protect against known security flaws.
>
> This change adds anti-rollback protection for FIT images.
>
> Signed-off-by: Thirupathaiah Annapureddy <thiruan at linux.microsoft.com>
> ---
>  Kconfig                |  9 +++++
>  common/image-fit-sig.c | 79 ++++++++++++++++++++++++++++++++++++++++++
>  common/image-fit.c     | 24 +++++++++++++
>  include/image.h        | 23 ++++++++++++
>  4 files changed, 135 insertions(+)

Good to see this. I have a few comments though. This needs to be done
very carefully as it affects security.

>
> diff --git a/Kconfig b/Kconfig
> index 883e3f71d0..3959a6592c 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -533,6 +533,15 @@ config FIT_CIPHER
>           Enable the feature of data ciphering/unciphering in the tool mkimage
>           and in the u-boot support of the FIT image.
>
> +config FIT_ARBP

How about using ROLLBACK instead of ARBP. It is easier to understand.

> +       bool "Enable Anti rollback version check for FIT images"

anti-rollback (add hyphen)

> +       depends on FIT_SIGNATURE
> +       default n
> +       help
> +         Enable this to activate anti-rollback protection. This is required
> +         when a platform needs to retire previous versions of FIT images due to
> +         security flaws in in them.

in

> +
>  config FIT_VERBOSE
>         bool "Show verbose messages when FIT images fail"
>         help
> diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
> index cc1967109e..5103508894 100644
> --- a/common/image-fit-sig.c
> +++ b/common/image-fit-sig.c
> @@ -78,6 +78,35 @@ struct image_region *fit_region_make_list(const void *fit,
>         return region;
>  }
>
> +#if !defined(USE_HOSTCC)
> +static int fit_image_verify_arbvn(const void *fit, int image_noffset)

Please add a comment as to what this does and what it checks

> +{
> +       uint8_t type;
> +       uint32_t image_arbvn;
> +       uint32_t plat_arbvn = 0;

Those three can be uint.

> +       int ret;
> +
> +       ret = fit_image_get_arbvn(fit, image_noffset, &image_arbvn);
> +       if (ret)
> +               return 0;
> +
> +       ret = fit_image_get_type(fit, image_noffset, &type);
> +       if (ret)
> +               return 0;
> +
> +       ret = board_get_arbvn(type, &plat_arbvn);
> +
> +       if (image_arbvn < plat_arbvn) {
> +               return -EPERM;
> +       } else if (image_arbvn > plat_arbvn) {
> +               ret = board_set_arbvn(type, image_arbvn);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
>  static int fit_image_setup_verify(struct image_sign_info *info,
>                                   const void *fit, int noffset,
>                                   int required_keynode, char **err_msgp)
> @@ -181,6 +210,14 @@ static int fit_image_verify_sig(const void *fit, int image_noffset,
>                 goto error;
>         }
>
> +#if !defined(USE_HOSTCC)
> +       if (IMAGE_ENABLE_ARBP && verified) {
> +               ret = fit_image_verify_arbvn(fit, image_noffset);
> +               if (ret)
> +                       verified = 0;
> +       }
> +#endif
> +
>         return verified ? 0 : -EPERM;
>
>  error:
> @@ -370,6 +407,40 @@ static int fit_config_check_sig(const void *fit, int noffset,
>         return 0;
>  }
>
> +#if !defined(USE_HOSTCC)
> +static int fit_config_verify_arbvn(const void *fit, int conf_noffset,
> +                                  int sig_offset)
> +{
> +       static const char default_list[] = FIT_KERNEL_PROP "\0"
> +                       FIT_FDT_PROP;
> +       int ret, len;
> +       const char *prop, *iname, *end;
> +       int image_noffset;
> +
> +       /* If there is "sign-images" property, use that */
> +       prop = fdt_getprop(fit, sig_offset, "sign-images", &len);
> +       if (!prop) {
> +               prop = default_list;
> +               len = sizeof(default_list);
> +       }
> +
> +       /* Locate the images */
> +       end = prop + len;
> +       for (iname = prop; iname < end; iname += strlen(iname) + 1) {
> +               image_noffset = fit_conf_get_prop_node(fit, conf_noffset,
> +                                                      iname);
> +               if (image_noffset < 0)
> +                       return -ENOENT;
> +
> +               ret = fit_image_verify_arbvn(fit, image_noffset);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
>  static int fit_config_verify_sig(const void *fit, int conf_noffset,
>                                  const void *sig_blob, int sig_offset)
>  {
> @@ -401,6 +472,14 @@ static int fit_config_verify_sig(const void *fit, int conf_noffset,
>                 goto error;
>         }
>
> +#if !defined(USE_HOSTCC)

Do we need this £ifdef, or can we rely on IMAGE_ENABLE_ARBP?

> +       if (IMAGE_ENABLE_ARBP && verified) {
> +               ret = fit_config_verify_arbvn(fit, conf_noffset, noffset);
> +               if (ret)
> +                       verified = 0;
> +       }
> +#endif
> +
>         if (verified)
>                 return 0;
>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index d54eff9033..97029853b9 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -1025,6 +1025,30 @@ int fit_image_get_data_and_size(const void *fit, int noffset,
>         return ret;
>  }
>
> +/**
> + * Get 'arbvn' property from a given image node.
> + *
> + * @fit: pointer to the FIT image header
> + * @noffset: component image node offset
> + * @arbvn: holds the arbvn property value
> + *
> + * returns:
> + *     0, on success
> + *     -ENOENT if the property could not be found
> + */
> +int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn)

fit_image_get_rollback()

s/arbvn/versionp/

> +{
> +       const fdt32_t *val;
> +
> +       val = fdt_getprop(fit, noffset, FIT_ARBVN_PROP, NULL);
> +       if (!val)
> +               return -ENOENT;
> +
> +       *arbvn = fdt32_to_cpu(*val);
> +
> +       return 0;
> +}
> +
>  /**
>   * fit_image_hash_get_algo - get hash algorithm name
>   * @fit: pointer to the FIT format image header
> diff --git a/include/image.h b/include/image.h
> index 9a5a87dbf8..72a963cf27 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1005,6 +1005,7 @@ int booti_setup(ulong image, ulong *relocated_addr, ulong *size,
>  #define FIT_COMP_PROP          "compression"
>  #define FIT_ENTRY_PROP         "entry"
>  #define FIT_LOAD_PROP          "load"
> +#define FIT_ARBVN_PROP         "arbvn"

ROLLBACK / "rollback"

>
>  /* configuration node */
>  #define FIT_KERNEL_PROP                "kernel"
> @@ -1085,6 +1086,7 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
>                                        size_t *data_size);
>  int fit_image_get_data_and_size(const void *fit, int noffset,
>                                 const void **data, size_t *size);
> +int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn);

Please add a full function comment

>
>  int fit_image_hash_get_algo(const void *fit, int noffset, char **algo);
>  int fit_image_hash_get_value(const void *fit, int noffset, uint8_t **value,
> @@ -1186,6 +1188,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>   * device
>   */
>  #if defined(USE_HOSTCC)
> +# define IMAGE_ENABLE_ARBP     0
>  # if defined(CONFIG_FIT_SIGNATURE)
>  #  define IMAGE_ENABLE_SIGN    1
>  #  define IMAGE_ENABLE_VERIFY  1
> @@ -1200,6 +1203,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>  # define IMAGE_ENABLE_SIGN     0
>  # define IMAGE_ENABLE_VERIFY           CONFIG_IS_ENABLED(RSA_VERIFY)
>  # define FIT_IMAGE_ENABLE_VERIFY       CONFIG_IS_ENABLED(FIT_SIGNATURE)
> +# define IMAGE_ENABLE_ARBP             CONFIG_IS_ENABLED(FIT_ARBP)
>  #endif
>
>  #if IMAGE_ENABLE_FIT
> @@ -1544,6 +1548,25 @@ int board_fit_config_name_match(const char *name);
>  void board_fit_image_post_process(void **p_image, size_t *p_size);
>  #endif /* CONFIG_SPL_FIT_IMAGE_POST_PROCESS */
>
> +#if defined(CONFIG_FIT_ARBP)
> +/**
> + * board_get_arbvn() - get the arbvn stored in the platform secure storage.
> + *
> + * @ih_type: image type
> + * @arbvn: pointer to the arbvn
> + * @return 0 if upon successful retrieval, non-zero upon failure.
> + */
> +int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);

This needs a driver since the rollback counter may be implemented by a
TPM or anything. If you want to use the board, add a new
get_rollback() to UCLASS_BOARD (board.h). Or you could create a new
UCLASS_SECURITY which includes these two API calls.

> +/**
> + * board_set_arbvn() - set the arbvn stored in the platform secure storage.
> + *
> + * @ih_type: image type
> + * @arbvn: new arbvn value to store in the platform secure storage.
> + * @return 0 if stored successfully, non-zero upon failure.
> + */
> +int board_set_arbvn(uint8_t ih_type, uint32_t arbvn);
> +#endif /* CONFIG_FIT_ARBP */
> +
>  #define FDT_ERROR      ((ulong)(-1))
>
>  ulong fdt_getprop_u32(const void *fdt, int node, const char *prop);
> --
> 2.25.2
>

Also please update the vboot test to add a check for rollback.

Regards,
Simon


More information about the U-Boot mailing list