[PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT

[Alex G.]

Hi Oleksandr

On 9/16/21 8:09 AM, Oleksandr Suvorov wrote:
> From: Henry Beberman <hebeberm at microsoft.com>
> 
> SPL FIT load checks the signature on loadable images but just continues
> in the case of a failure. This is undesirable behavior because the boot
> process depends on the authenticity of each loadable part.
> 
> Adding CONFIG_SPL_FIT_SIGNATURE_STRICT to halt the platform when any
> image fails its signature check, including loadable parts.

This sounds very similar to what FIT config verification already does, 
so I don't understand the motivation behind this patch.

> SPL already supports image signature verification but had no mechanism
> to check that the FIT's configuration block was signed correctly.

This statement is incorrect. This is exactly what required = "conf";
in u-boot's devicetree is intended to do.

NAK

> Add a check near the start of spl_load_simple_fit that verifies the
> FIT's configuration block, and fails if it's not present or the
> signature doesn't match what's stored in the SPL DTB.
> 
> Signed-off-by: Henry Beberman <hebeberm at microsoft.com>
> Signed-off-by: Ricardo Salveti <ricardo at foundries.io>
> Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov at foundries.io>
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov at foundries.io>
> ---
> 
>   common/Kconfig.boot  |  7 +++++++
>   common/spl/spl_fit.c | 21 ++++++++++++++++++++-
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> index 902a5b8fbea..6f95d009dfa 100644
> --- a/common/Kconfig.boot
> +++ b/common/Kconfig.boot
> @@ -166,6 +166,13 @@ config SPL_FIT_SIGNATURE
>   	select SPL_IMAGE_SIGN_INFO
>   	select SPL_FIT_FULL_CHECK
>   
> +config SPL_FIT_SIGNATURE_STRICT
> +	bool "Halt if loadables or firmware don't pass FIT signature verification"
> +	select SPL_FIT_SIGNATURE
> +	help
> +	  Strictly requires each loadable or firmware in a FIT image to be
> +	  passed verification. Halt if any loadable fails to be verified.
> +
>   config SPL_LOAD_FIT
>   	bool "Enable SPL loading U-Boot as a FIT (basic fitImage features)"
>   	select SPL_FIT
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index f41abca0ccb..e7eaaa4cb9e 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -315,7 +315,12 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>   		printf("## Checking hash(es) for Image %s ... ",
>   		       fit_get_name(fit, node, NULL));
>   		if (!fit_image_verify_with_data(fit, node, src, length))
> -			return -EPERM;
> +			if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> +				puts("Invalid FIT signature found in a required image.\n");
> +				hang();

hang() is rarely the appropriate thing to do. Once you get -EPERM you 
could choose to either
   a) drop to a lower privilege level
   b) enter some sort of recovery mode
   c) outright hang.

spl_load_fit_image() isn't the right place to decide (a) or (b), so by 
extension, it's the wrong place to decide (c).

> +			} else {
> +				return -EPERM;
> +			}
>   		puts("OK\n");
>   	}
>   
> @@ -681,6 +686,20 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   	if (ret < 0)
>   		return ret;
>   
> +	if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> +		int cfg_noffset = fit_conf_get_node(fit, NULL);
> +
> +		if (cfg_noffset >= 0) {
> +			if (fit_config_verify(fit, cfg_noffset)) {
> +				puts("Unable to verify the required FIT config.\n");
> +				hang();
> +			}
> +		} else {
> +			puts("SPL_FIT_SIGNATURE_STRICT needs a config node in FIT\n");
> +			hang();
> +		}
> +	}
> +
>   	/* skip further processing if requested to enable load-only use cases */
>   	if (spl_load_simple_fit_skip_processing())
>   		return 0;
>