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 ofsignatures 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