Re: [PATCH] image: Control FIT signature verification at runtime

["Alex G." <mr.nuke.me@gmail.com>]

On 1/30/22 21:41, 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.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> Hi,
> 
> This patch is extracted from and motivated by a series adding run-time
> control of FIT signature verification to u-boot in OpenBMC:
> 
> https://lore.kernel.org/openbmc/20220131012538.73021-1-andrew@aj.id.au/
> 
> Unfortunately the OpenBMC u-boot tree is quite a way behind on tracking
> upstream and contains a bunch of out-of-tree work as well. As such I'm
> looking to upstream the couple of changes that make sense against
> master.

I don't understand the practical use of a mechanism to disable security 
if secure boot was enabled in the first place. Sure it can be 
technically done, but why is this not a bad idea?

> Please take a look!
> 
> Andrew
> 
>   boot/Kconfig     |  8 ++++++++
>   boot/image-fit.c | 21 +++++++++++++++++----
>   include/image.h  |  9 +++++++++
>   3 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/boot/Kconfig b/boot/Kconfig
> index c8d5906cd304..ec413151fd5a 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -78,6 +78,14 @@ config FIT_SIGNATURE
>   	  format support in this case, enable it using
>   	  CONFIG_LEGACY_IMAGE_FORMAT.
>   
> +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.

The commit message does a much nicer job explaining this option, even 
though it is this kconfig help text that almost all users will ever see.

> +endif # FIT_SIGNATURE
> +
>   config FIT_SIGNATURE_MAX_SIZE
>   	hex "Max size of signed FIT structures"
>   	depends on FIT_SIGNATURE
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> index f01cafe4e277..919dbfa4ee1d 100644
> --- a/boot/image-fit.c
> +++ b/boot/image-fit.c
> @@ -1308,6 +1308,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data,
>   	return 0;
>   }
>   
> +#ifndef __weak
> +#define __weak
> +#endif

What?

> +__weak int board_fit_image_require_verified(void)
> +{
> +	return 1;
> +}
> +

I caution against having any platform security related functionality as 
a weak function. Did I get the right function, or something else with 
the same name was compiled that returns 0 and silently disables security 
in my platform?

I think a weak function in this context is a source of confusion and 
future bugs. You could instead require the symbol to be defined if and 
only if the appropriate kconfig is selected. Symbol not defined -> 
compiler error. Symbol defined twice -> compiler error. Solves the 
issues in the preceding paragraph.

[snip]

> diff --git a/include/image.h b/include/image.h
> index 97e5f2eb24d6..98900c2e839b 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1173,6 +1173,15 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>   # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
>   #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()	FIT_IMAGE_ENABLE_VERIFY
> +#endif
> +

image.h is also used for host code. When only changing target code 
behavior, there should be a very good reason to modify common code. I'm 
not convinced that threshold has been met.

My second concern here is subjective: I don't like functions defined as 
macros, further depending on config selections. It makes many code 
parsers and IDEs poop their pantaloons. It makes u-boot harder to work 
with as a result. I suggest finding a way to turn this into a static inline.

Alex