| Message ID | 20210916130958.306964-2-oleksandr.suvorov@foundries.io |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Enable strict signature verification for FIT | expand |
Hi Oleksandr On 9/16/21 8:09 AM, Oleksandr Suvorov wrote: > From: Henry Beberman <hebeberm@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@microsoft.com> > Signed-off-by: Ricardo Salveti <ricardo@foundries.io> > Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@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; >
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(); + } 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;