Hi Oleksandr
On 9/16/21 8:09 AM, Oleksandr Suvorov wrote:
From: Henry Beberman <hebeb...@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 <hebeb...@microsoft.com>
Signed-off-by: Ricardo Salveti <rica...@foundries.io>
Co-developed-by: Oleksandr Suvorov <oleksandr.suvo...@foundries.io>
Signed-off-by: Oleksandr Suvorov <oleksandr.suvo...@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;
