[PATCH u-boot v2019.04-aspeed-openbmc 2/6] image: Control FIT uImage signature verification at runtime

[Andrew Jeffery]

On Tue, 8 Feb 2022, at 16:33, Joel Stanley wrote:
> On Mon, 31 Jan 2022 at 01:26, Andrew Jeffery <andrew at 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 at 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