Hi Thirupathaiah,
On Tue, 1 Sep 2020 at 14:48, Thirupathaiah Annapureddy
<thir...@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 <thir...@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
