| Message ID | 20220131012538.73021-3-andrew@aj.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | Runtime control of vboot via GPIO | expand |
On 1/30/22 19:25, Andrew Jeffery wrote: > Some platform designs include support for disabling secure-boot via a > jumper on the board. Sometimes this control can be separate from the > mechanism enabling the root-of-trust for the platform. Add support for > this latter scenario by allowing boards to implement > board_fit_image_require_verfied(), which is then invoked in the usual > FIT verification paths. Reviewed-by: Eddie James <eajames@linux.ibm.com > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > Kconfig | 9 +++++++++ > common/image-fit.c | 17 +++++++++++++++-- > include/image.h | 9 +++++++++ > 3 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/Kconfig b/Kconfig > index c3dfa8de47c8..11f796035ae4 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -322,6 +322,15 @@ config FIT_SIGNATURE > format support in this case, enable it using > CONFIG_IMAGE_FORMAT_LEGACY. > > +if FIT_SIGNATURE > +config FIT_RUNTIME_SIGNATURE > + bool "Control verification of FIT uImages at runtime" > + help > + This option allows board support to disable verification of > + signatures at runtime, for example through the state of a GPIO. > +endif # FIT_SIGNATURE > + > + > config FIT_SIGNATURE_MAX_SIZE > hex "Max size of signed FIT structures" > depends on FIT_SIGNATURE > diff --git a/common/image-fit.c b/common/image-fit.c > index 3c8667f93de2..eb1e66b02b68 100644 > --- a/common/image-fit.c > +++ b/common/image-fit.c > @@ -1199,6 +1199,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data, > return 0; > } > > +#ifndef __weak > +#define __weak > +#endif > +__weak int board_fit_image_require_verified(void) > +{ > + return 1; > +} > + > int fit_image_verify_with_data(const void *fit, int image_noffset, > const void *data, size_t size) > { > @@ -1209,6 +1217,7 @@ int fit_image_verify_with_data(const void *fit, int image_noffset, > > /* Verify all required signatures */ > if (IMAGE_ENABLE_VERIFY && > + fit_image_require_verified() && > fit_image_verify_required_sigs(fit, image_noffset, data, size, > gd_fdt_blob(), &verify_all)) { > err_msg = "Unable to verify required signature"; > @@ -1230,7 +1239,9 @@ int fit_image_verify_with_data(const void *fit, int image_noffset, > &err_msg)) > goto error; > puts("+ "); > - } else if (IMAGE_ENABLE_VERIFY && verify_all && > + } else if (IMAGE_ENABLE_VERIFY && > + fit_image_require_verified() && > + verify_all && > !strncmp(name, FIT_SIG_NODENAME, > strlen(FIT_SIG_NODENAME))) { > ret = fit_image_check_sig(fit, noffset, data, > @@ -1849,7 +1860,9 @@ int fit_image_load(bootm_headers_t *images, ulong addr, > if (image_type == IH_TYPE_KERNEL) > images->fit_uname_cfg = fit_base_uname_config; > > - if (IMAGE_ENABLE_VERIFY && images->verify) { > + if (IMAGE_ENABLE_VERIFY && > + fit_image_require_verified() && > + images->verify) { > puts(" Verifying Hash Integrity ... "); > if (fit_config_verify(fit, cfg_noffset)) { > puts("Bad Data Hash\n"); > diff --git a/include/image.h b/include/image.h > index 937c7eee8ffb..19ea743af08f 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -1103,6 +1103,15 @@ int calculate_hash(const void *data, int data_len, const char *algo, > # define IMAGE_ENABLE_VERIFY 0 > #endif > > +/* > + * Further, allow run-time control of verification, e.g. via a jumper > + */ > +#if defined(CONFIG_FIT_RUNTIME_SIGNATURE) > +# define fit_image_require_verified() board_fit_image_require_verified() > +#else > +# define fit_image_require_verified() IMAGE_ENABLE_VERIFY > +#endif > + > #ifdef USE_HOSTCC > void *image_get_host_blob(void); > void image_set_host_blob(void *host_blob);
On Mon, 31 Jan 2022 at 01:26, Andrew Jeffery <andrew@aj.id.au> wrote: > > Some platform designs include support for disabling secure-boot via a > jumper on the board. Sometimes this control can be separate from the > mechanism enabling the root-of-trust for the platform. Add support for > this latter scenario by allowing boards to implement > board_fit_image_require_verfied(), which is then invoked in the usual > FIT verification paths. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > Kconfig | 9 +++++++++ > common/image-fit.c | 17 +++++++++++++++-- > include/image.h | 9 +++++++++ > 3 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/Kconfig b/Kconfig > index c3dfa8de47c8..11f796035ae4 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -322,6 +322,15 @@ config FIT_SIGNATURE > format support in this case, enable it using > CONFIG_IMAGE_FORMAT_LEGACY. > > +if FIT_SIGNATURE > +config FIT_RUNTIME_SIGNATURE > + bool "Control verification of FIT uImages at runtime" Can you follow the pattern of the other FIT_ options by making this depends on FIT_SIGNATURE? > + help > + This option allows board support to disable verification of > + signatures at runtime, for example through the state of a GPIO. > +endif # FIT_SIGNATURE > + > + > config FIT_SIGNATURE_MAX_SIZE > hex "Max size of signed FIT structures" > depends on FIT_SIGNATURE > diff --git a/common/image-fit.c b/common/image-fit.c > index 3c8667f93de2..eb1e66b02b68 100644 > --- a/common/image-fit.c > +++ b/common/image-fit.c > @@ -1199,6 +1199,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data, > return 0; > } > > +#ifndef __weak > +#define __weak > +#endif Shouldn't we always get this from linux/compiler.h? > +__weak int board_fit_image_require_verified(void) > +{ > + return 1; > +} > + > int fit_image_verify_with_data(const void *fit, int image_noffset, > const void *data, size_t size) > { > @@ -1209,6 +1217,7 @@ int fit_image_verify_with_data(const void *fit, int image_noffset, > > /* Verify all required signatures */ > if (IMAGE_ENABLE_VERIFY && > + fit_image_require_verified() && > fit_image_verify_required_sigs(fit, image_noffset, data, size, > gd_fdt_blob(), &verify_all)) { > err_msg = "Unable to verify required signature"; > @@ -1230,7 +1239,9 @@ int fit_image_verify_with_data(const void *fit, int image_noffset, > &err_msg)) > goto error; > puts("+ "); > - } else if (IMAGE_ENABLE_VERIFY && verify_all && > + } else if (IMAGE_ENABLE_VERIFY && > + fit_image_require_verified() && > + verify_all && reading through this it's quite confusing. We have IMAGE_ENABLE_VERIFY, a compile time constant that will be true if CONFIG_FIT_SIGNATURE is enabled. We're adding a function that will override this. So we could have a function: __weak bool fit_enable_verification(void) { return IMAGE_ENABLE_VERIFY; } The downside of this would be if a board were to implement this but not have FIT_SIGNATURE enabled then they could return true when they shouldn't. You could go back to this: static bool fit_enable_verification(void) { return IMAGE_ENABLE_VERIFY && board_fit_image_require_verified(); } And drop the ifdefs from image.h > !strncmp(name, FIT_SIG_NODENAME, > strlen(FIT_SIG_NODENAME))) { > ret = fit_image_check_sig(fit, noffset, data, > @@ -1849,7 +1860,9 @@ int fit_image_load(bootm_headers_t *images, ulong addr, > if (image_type == IH_TYPE_KERNEL) > images->fit_uname_cfg = fit_base_uname_config; > > - if (IMAGE_ENABLE_VERIFY && images->verify) { > + if (IMAGE_ENABLE_VERIFY && > + fit_image_require_verified() && > + images->verify) { > puts(" Verifying Hash Integrity ... "); > if (fit_config_verify(fit, cfg_noffset)) { > puts("Bad Data Hash\n"); > diff --git a/include/image.h b/include/image.h > index 937c7eee8ffb..19ea743af08f 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -1103,6 +1103,15 @@ int calculate_hash(const void *data, int data_len, const char *algo, > # define IMAGE_ENABLE_VERIFY 0 > #endif > > +/* > + * Further, allow run-time control of verification, e.g. via a jumper > + */ > +#if defined(CONFIG_FIT_RUNTIME_SIGNATURE) > +# define fit_image_require_verified() board_fit_image_require_verified() > +#else > +# define fit_image_require_verified() IMAGE_ENABLE_VERIFY > +#endif > + > #ifdef USE_HOSTCC > void *image_get_host_blob(void); > void image_set_host_blob(void *host_blob); > -- > 2.32.0 >
On Tue, 8 Feb 2022, at 16:33, Joel Stanley wrote: > On Mon, 31 Jan 2022 at 01:26, Andrew Jeffery <andrew@aj.id.au> wrote: >> >> Some platform designs include support for disabling secure-boot via a >> jumper on the board. Sometimes this control can be separate from the >> mechanism enabling the root-of-trust for the platform. Add support for >> this latter scenario by allowing boards to implement >> board_fit_image_require_verfied(), which is then invoked in the usual >> FIT verification paths. >> >> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >> --- >> Kconfig | 9 +++++++++ >> common/image-fit.c | 17 +++++++++++++++-- >> include/image.h | 9 +++++++++ >> 3 files changed, 33 insertions(+), 2 deletions(-) >> >> diff --git a/Kconfig b/Kconfig >> index c3dfa8de47c8..11f796035ae4 100644 >> --- a/Kconfig >> +++ b/Kconfig >> @@ -322,6 +322,15 @@ config FIT_SIGNATURE >> format support in this case, enable it using >> CONFIG_IMAGE_FORMAT_LEGACY. >> >> +if FIT_SIGNATURE >> +config FIT_RUNTIME_SIGNATURE >> + bool "Control verification of FIT uImages at runtime" > > Can you follow the pattern of the other FIT_ options by making this > depends on FIT_SIGNATURE? Yeah, I didn't think about this enough :) > >> + help >> + This option allows board support to disable verification of >> + signatures at runtime, for example through the state of a GPIO. >> +endif # FIT_SIGNATURE >> + >> + >> config FIT_SIGNATURE_MAX_SIZE >> hex "Max size of signed FIT structures" >> depends on FIT_SIGNATURE >> diff --git a/common/image-fit.c b/common/image-fit.c >> index 3c8667f93de2..eb1e66b02b68 100644 >> --- a/common/image-fit.c >> +++ b/common/image-fit.c >> @@ -1199,6 +1199,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data, >> return 0; >> } >> >> +#ifndef __weak >> +#define __weak >> +#endif > > Shouldn't we always get this from linux/compiler.h? I'll think about this some more as this file gets linked into the tools as well as the firmware. But probably. > >> +__weak int board_fit_image_require_verified(void) >> +{ >> + return 1; >> +} >> + >> int fit_image_verify_with_data(const void *fit, int image_noffset, >> const void *data, size_t size) >> { >> @@ -1209,6 +1217,7 @@ int fit_image_verify_with_data(const void *fit, int image_noffset, >> >> /* Verify all required signatures */ >> if (IMAGE_ENABLE_VERIFY && >> + fit_image_require_verified() && >> fit_image_verify_required_sigs(fit, image_noffset, data, size, >> gd_fdt_blob(), &verify_all)) { >> err_msg = "Unable to verify required signature"; >> @@ -1230,7 +1239,9 @@ int fit_image_verify_with_data(const void *fit, int image_noffset, >> &err_msg)) >> goto error; >> puts("+ "); >> - } else if (IMAGE_ENABLE_VERIFY && verify_all && >> + } else if (IMAGE_ENABLE_VERIFY && >> + fit_image_require_verified() && >> + verify_all && > > reading through this it's quite confusing. > > We have IMAGE_ENABLE_VERIFY, a compile time constant that will be true > if CONFIG_FIT_SIGNATURE is enabled. > > We're adding a function that will override this. > > So we could have a function: > > __weak bool fit_enable_verification(void) > { > return IMAGE_ENABLE_VERIFY; > } > > The downside of this would be if a board were to implement this but > not have FIT_SIGNATURE enabled then they could return true when they > shouldn't. You could go back to this: > > static bool fit_enable_verification(void) > { > return IMAGE_ENABLE_VERIFY && board_fit_image_require_verified(); > } > > And drop the ifdefs from image.h This sounds attractive, let me poke at it. Thanks for thinking about it. Andrew
diff --git a/Kconfig b/Kconfig index c3dfa8de47c8..11f796035ae4 100644 --- a/Kconfig +++ b/Kconfig @@ -322,6 +322,15 @@ config FIT_SIGNATURE format support in this case, enable it using CONFIG_IMAGE_FORMAT_LEGACY. +if FIT_SIGNATURE +config FIT_RUNTIME_SIGNATURE + bool "Control verification of FIT uImages at runtime" + help + This option allows board support to disable verification of + signatures at runtime, for example through the state of a GPIO. +endif # FIT_SIGNATURE + + config FIT_SIGNATURE_MAX_SIZE hex "Max size of signed FIT structures" depends on FIT_SIGNATURE diff --git a/common/image-fit.c b/common/image-fit.c index 3c8667f93de2..eb1e66b02b68 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1199,6 +1199,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data, return 0; } +#ifndef __weak +#define __weak +#endif +__weak int board_fit_image_require_verified(void) +{ + return 1; +} + int fit_image_verify_with_data(const void *fit, int image_noffset, const void *data, size_t size) { @@ -1209,6 +1217,7 @@ int fit_image_verify_with_data(const void *fit, int image_noffset, /* Verify all required signatures */ if (IMAGE_ENABLE_VERIFY && + fit_image_require_verified() && fit_image_verify_required_sigs(fit, image_noffset, data, size, gd_fdt_blob(), &verify_all)) { err_msg = "Unable to verify required signature"; @@ -1230,7 +1239,9 @@ int fit_image_verify_with_data(const void *fit, int image_noffset, &err_msg)) goto error; puts("+ "); - } else if (IMAGE_ENABLE_VERIFY && verify_all && + } else if (IMAGE_ENABLE_VERIFY && + fit_image_require_verified() && + verify_all && !strncmp(name, FIT_SIG_NODENAME, strlen(FIT_SIG_NODENAME))) { ret = fit_image_check_sig(fit, noffset, data, @@ -1849,7 +1860,9 @@ int fit_image_load(bootm_headers_t *images, ulong addr, if (image_type == IH_TYPE_KERNEL) images->fit_uname_cfg = fit_base_uname_config; - if (IMAGE_ENABLE_VERIFY && images->verify) { + if (IMAGE_ENABLE_VERIFY && + fit_image_require_verified() && + images->verify) { puts(" Verifying Hash Integrity ... "); if (fit_config_verify(fit, cfg_noffset)) { puts("Bad Data Hash\n"); diff --git a/include/image.h b/include/image.h index 937c7eee8ffb..19ea743af08f 100644 --- a/include/image.h +++ b/include/image.h @@ -1103,6 +1103,15 @@ int calculate_hash(const void *data, int data_len, const char *algo, # define IMAGE_ENABLE_VERIFY 0 #endif +/* + * Further, allow run-time control of verification, e.g. via a jumper + */ +#if defined(CONFIG_FIT_RUNTIME_SIGNATURE) +# define fit_image_require_verified() board_fit_image_require_verified() +#else +# define fit_image_require_verified() IMAGE_ENABLE_VERIFY +#endif + #ifdef USE_HOSTCC void *image_get_host_blob(void); void image_set_host_blob(void *host_blob);
Some platform designs include support for disabling secure-boot via a jumper on the board. Sometimes this control can be separate from the mechanism enabling the root-of-trust for the platform. Add support for this latter scenario by allowing boards to implement board_fit_image_require_verfied(), which is then invoked in the usual FIT verification paths. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- Kconfig | 9 +++++++++ common/image-fit.c | 17 +++++++++++++++-- include/image.h | 9 +++++++++ 3 files changed, 33 insertions(+), 2 deletions(-)