From: "Alex G." <mr.nuke.me@gmail.com>
To: Andrew Jeffery <andrew@aj.id.au>, u-boot@lists.denx.de
Cc: openbmc@lists.ozlabs.org, sjg@chromium.org, chiawei_wang@aspeedtech.com
Subject: Re: [PATCH] image: Control FIT signature verification at runtime
Date: Sat, 12 Feb 2022 12:55:24 -0600 [thread overview]
Message-ID: <97430094-7d2a-432b-a121-96812fac87f9@gmail.com> (raw)
In-Reply-To: <20220131034147.106415-1-andrew@aj.id.au>
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
next prev parent reply other threads:[~2022-02-15 4:35 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-31 3:41 [PATCH] image: Control FIT signature verification at runtime Andrew Jeffery
2022-02-07 1:07 ` ChiaWei Wang
2022-02-08 21:55 ` Andrew Jeffery
2022-02-12 22:54 ` Dhananjay Phadke
2022-02-12 18:55 ` Alex G. [this message]
2022-02-14 1:13 ` Andrew Jeffery
2022-02-14 19:14 ` Dhananjay Phadke
2022-02-14 23:08 ` Andrew Jeffery
2022-02-14 23:13 ` Patrick Williams
2022-02-15 0:21 ` Andrew Jeffery
2022-02-15 3:12 ` Dhananjay Phadke
2022-02-15 3:25 ` Andrew Jeffery
2022-02-28 1:29 ` Andrew Jeffery
2022-02-28 22:12 ` Alex G.
2022-02-28 22:42 ` Andrew Jeffery
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=97430094-7d2a-432b-a121-96812fac87f9@gmail.com \
--to=mr.nuke.me@gmail.com \
--cc=andrew@aj.id.au \
--cc=chiawei_wang@aspeedtech.com \
--cc=openbmc@lists.ozlabs.org \
--cc=sjg@chromium.org \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).