[PATCH u-boot v2019.04-aspeed-openbmc 0/6] Runtime control of vboot via GPIO

[PATCH u-boot v2019.04-aspeed-openbmc 0/6] Runtime control of vboot via GPIO

[Andrew Jeffery]

Hello,

This u-boot series implements support for controlling whether verified
boot is enabled at runtime by measuring the state of the
"bmc-secure-boot" GPIO for AST2600-based BMCs.

Previously, whether or not verified boot was required was configured at
build time.

These patches build on top of the series Eddie recently sent to the list
to enable use of GPIOs in the SPL:

https://lore.kernel.org/openbmc/20220124191503.88452-1-eajames@linux.ibm.com/

I've tested the changes under qemu, and they behave as expected for the
Rainier platform configuration.

I'll figure out how we go about upstreaming the series once Eddie's
series has been picked up.

Please review!

Andrew

Andrew Jeffery (6):
  gpio: Add gpio_request_by_line_name()
  image: Control FIT uImage signature verification at runtime
  ARM: ast2600: Control FIT uImage signature verification at runtime
  configs: ast2600: Runtime control of FIT signature verification
  ARM: dts: rainier: Add gpio-line-names property with bmc-secure-boot
  image: Fix indentation of macros

 Kconfig                                    |  9 ++++
 arch/arm/dts/ast2600-rainier.dts           | 32 +++++++++++++
 arch/arm/mach-aspeed/ast2600/Makefile      |  1 +
 arch/arm/mach-aspeed/ast2600/secure-boot.c | 53 ++++++++++++++++++++++
 common/image-fit.c                         | 17 ++++++-
 configs/ast2600_openbmc_spl_emmc_defconfig |  1 +
 drivers/gpio/gpio-uclass.c                 | 26 +++++++++++
 include/asm-generic/gpio.h                 | 19 ++++++++
 include/image.h                            | 13 +++++-
 9 files changed, 167 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm/mach-aspeed/ast2600/secure-boot.c

-- 
2.32.0

[PATCH u-boot v2019.04-aspeed-openbmc 1/6] gpio: Add gpio_request_by_line_name()

[Andrew Jeffery]

Add support for the upstream gpio-line-names property already described
in the common GPIO binding document[1]. The ability to search for a line
name allows boards to lift the implementation of common GPIO behaviours
away from specific line indexes on a GPIO controller.

[1] https://github.com/devicetree-org/dt-schema/blob/3c35bfee83c2e38e2ae7af5f83eb89ca94a521e8/dtschema/schemas/gpio/gpio.yaml#L17

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/gpio/gpio-uclass.c | 26 ++++++++++++++++++++++++++
 include/asm-generic/gpio.h | 19 +++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 219caa651bb2..425bbc5cb880 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -878,6 +878,32 @@ int gpio_request_by_name(struct udevice *dev, const char *list_name, int index,
 				 index, desc, flags, index > 0, NULL);
 }
 
+int gpio_request_by_line_name(struct udevice *dev, const char *line_name,
+			      struct gpio_desc *desc, int flags)
+{
+	int ret;
+
+	ret = dev_read_stringlist_search(dev, "gpio-line-names", line_name);
+	if (ret < 0)
+		return ret;
+
+	desc->dev = dev;
+	desc->offset = ret;
+	desc->flags = 0;
+
+	ret = dm_gpio_request(desc, line_name);
+	if (ret) {
+		debug("%s: dm_gpio_requestf failed\n", __func__);
+		return ret;
+	}
+
+	ret = dm_gpio_set_dir_flags(desc, flags | desc->flags);
+	if (ret)
+		debug("%s: dm_gpio_set_dir failed\n", __func__);
+
+	return ret;
+}
+
 int gpio_request_list_by_name_nodev(ofnode node, const char *list_name,
 				    struct gpio_desc *desc, int max_count,
 				    int flags)
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index d6cf18744fda..6ed0ba11b6c1 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -451,6 +451,25 @@ int gpio_claim_vector(const int *gpio_num_array, const char *fmt);
 int gpio_request_by_name(struct udevice *dev, const char *list_name,
 			 int index, struct gpio_desc *desc, int flags);
 
+/* gpio_request_by_line_name - Locate and request a GPIO by line name
+ *
+ * Request a GPIO using the offset of the provided line name in the
+ * gpio-line-names property found in the OF node of the GPIO udevice.
+ *
+ * This allows boards to implement common behaviours using GPIOs while not
+ * requiring specific GPIO offsets be used.
+ *
+ * @dev:	An instance of a GPIO controller udevice
+ * @line_name:	The name of the GPIO (e.g. "bmc-secure-boot")
+ * @desc:	A GPIO descriptor that is populated with the requested GPIO
+ *              upon return
+ * @flags:	The GPIO settings apply to the request
+ * @return 0 if the named line was found and requested successfully, or a
+ * negative error code if the GPIO cannot be found or the request failed.
+ */
+int gpio_request_by_line_name(struct udevice *dev, const char *line_name,
+			      struct gpio_desc *desc, int flags);
+
 /**
  * gpio_request_list_by_name() - Request a list of GPIOs
  *
-- 
2.32.0

Re: [PATCH u-boot v2019.04-aspeed-openbmc 1/6] gpio: Add gpio_request_by_line_name()

[Eddie James]

On 1/30/22 19:25, Andrew Jeffery wrote:
> Add support for the upstream gpio-line-names property already described
> in the common GPIO binding document[1]. The ability to search for a line
> name allows boards to lift the implementation of common GPIO behaviours
> away from specific line indexes on a GPIO controller.
>
> [1] https://github.com/devicetree-org/dt-schema/blob/3c35bfee83c2e38e2ae7af5f83eb89ca94a521e8/dtschema/schemas/gpio/gpio.yaml#L17


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/gpio/gpio-uclass.c | 26 ++++++++++++++++++++++++++
>   include/asm-generic/gpio.h | 19 +++++++++++++++++++
>   2 files changed, 45 insertions(+)
>
> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> index 219caa651bb2..425bbc5cb880 100644
> --- a/drivers/gpio/gpio-uclass.c
> +++ b/drivers/gpio/gpio-uclass.c
> @@ -878,6 +878,32 @@ int gpio_request_by_name(struct udevice *dev, const char *list_name, int index,
>   				 index, desc, flags, index > 0, NULL);
>   }
>   
> +int gpio_request_by_line_name(struct udevice *dev, const char *line_name,
> +			      struct gpio_desc *desc, int flags)
> +{
> +	int ret;
> +
> +	ret = dev_read_stringlist_search(dev, "gpio-line-names", line_name);
> +	if (ret < 0)
> +		return ret;
> +
> +	desc->dev = dev;
> +	desc->offset = ret;
> +	desc->flags = 0;
> +
> +	ret = dm_gpio_request(desc, line_name);
> +	if (ret) {
> +		debug("%s: dm_gpio_requestf failed\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = dm_gpio_set_dir_flags(desc, flags | desc->flags);
> +	if (ret)
> +		debug("%s: dm_gpio_set_dir failed\n", __func__);
> +
> +	return ret;
> +}
> +
>   int gpio_request_list_by_name_nodev(ofnode node, const char *list_name,
>   				    struct gpio_desc *desc, int max_count,
>   				    int flags)
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index d6cf18744fda..6ed0ba11b6c1 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -451,6 +451,25 @@ int gpio_claim_vector(const int *gpio_num_array, const char *fmt);
>   int gpio_request_by_name(struct udevice *dev, const char *list_name,
>   			 int index, struct gpio_desc *desc, int flags);
>   
> +/* gpio_request_by_line_name - Locate and request a GPIO by line name
> + *
> + * Request a GPIO using the offset of the provided line name in the
> + * gpio-line-names property found in the OF node of the GPIO udevice.
> + *
> + * This allows boards to implement common behaviours using GPIOs while not
> + * requiring specific GPIO offsets be used.
> + *
> + * @dev:	An instance of a GPIO controller udevice
> + * @line_name:	The name of the GPIO (e.g. "bmc-secure-boot")
> + * @desc:	A GPIO descriptor that is populated with the requested GPIO
> + *              upon return
> + * @flags:	The GPIO settings apply to the request
> + * @return 0 if the named line was found and requested successfully, or a
> + * negative error code if the GPIO cannot be found or the request failed.
> + */
> +int gpio_request_by_line_name(struct udevice *dev, const char *line_name,
> +			      struct gpio_desc *desc, int flags);
> +
>   /**
>    * gpio_request_list_by_name() - Request a list of GPIOs
>    *

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

[Andrew Jeffery]

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"
+	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);
-- 
2.32.0

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

[Eddie James]

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);

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

[Joel Stanley]

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
>

Re: [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@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

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

[Andrew Jeffery]

Implement support for disabling signature verification of FIT images at
runtime by sampling the "bmc-secure-boot" GPIO. If the line name is not
provided in the devicetree then secure-boot continues to be required as
if the feature were not present.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/mach-aspeed/ast2600/Makefile      |  1 +
 arch/arm/mach-aspeed/ast2600/secure-boot.c | 53 ++++++++++++++++++++++
 2 files changed, 54 insertions(+)
 create mode 100644 arch/arm/mach-aspeed/ast2600/secure-boot.c

diff --git a/arch/arm/mach-aspeed/ast2600/Makefile b/arch/arm/mach-aspeed/ast2600/Makefile
index d07e8c737cfe..70b7ae11df56 100644
--- a/arch/arm/mach-aspeed/ast2600/Makefile
+++ b/arch/arm/mach-aspeed/ast2600/Makefile
@@ -1,4 +1,5 @@
 obj-y   += platform.o board_common.o scu_info.o utils.o cache.o
+obj-$(CONFIG_FIT_RUNTIME_SIGNATURE) += secure-boot.o
 obj-$(CONFIG_ASPEED_SECURE_BOOT) += crypto.o aspeed_verify.o
 obj-$(CONFIG_ASPEED_LOADERS) += spl_boot.o
 obj-$(CONFIG_SPL_BUILD) += spl.o
diff --git a/arch/arm/mach-aspeed/ast2600/secure-boot.c b/arch/arm/mach-aspeed/ast2600/secure-boot.c
new file mode 100644
index 000000000000..ced353686387
--- /dev/null
+++ b/arch/arm/mach-aspeed/ast2600/secure-boot.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// (C) Copyright IBM Corp. 2022
+
+#include <common.h>
+#include <asm-generic/gpio.h>
+#include <dm.h>
+
+static int aspeed_get_chained_secboot_state(void)
+{
+	struct gpio_desc desc;
+	struct udevice *dev;
+	int secboot;
+	int rc;
+
+	rc = uclass_get_device_by_driver(UCLASS_GPIO,
+					 DM_GET_DRIVER(gpio_aspeed),
+					 &dev);
+	if (rc < 0) {
+		debug("Warning: GPIO initialization failure: %d\n", rc);
+		return rc;
+	}
+
+	rc = gpio_request_by_line_name(dev, "bmc-secure-boot", &desc,
+				       GPIOD_IS_IN);
+	if (rc < 0) {
+		debug("Failed to acquire secure-boot GPIO: %d\n", rc);
+		return rc;
+	}
+
+	secboot = dm_gpio_get_value(&desc);
+	if (secboot < 0)
+		debug("Failed to read secure-boot GPIO value: %d\n", rc);
+
+	rc = dm_gpio_free(dev, &desc);
+	if (rc < 0)
+		debug("Failed to free secure-boot GPIO: %d\n", rc);
+
+	return secboot;
+}
+
+int board_fit_image_require_verified(void)
+{
+	int secboot;
+
+	secboot = aspeed_get_chained_secboot_state();
+
+	/*
+	 * If secure-boot is enabled then require signature verification.
+	 * Otherwise, if we fail to read the GPIO, enforce FIT signature
+	 * verification
+	 */
+	return secboot >= 0 ? secboot : 1;
+}
-- 
2.32.0

Re: [PATCH u-boot v2019.04-aspeed-openbmc 3/6] ARM: ast2600: Control FIT uImage signature verification at runtime

[Eddie James]

On 1/30/22 19:25, Andrew Jeffery wrote:
> Implement support for disabling signature verification of FIT images at
> runtime by sampling the "bmc-secure-boot" GPIO. If the line name is not
> provided in the devicetree then secure-boot continues to be required as
> if the feature were not present.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   arch/arm/mach-aspeed/ast2600/Makefile      |  1 +
>   arch/arm/mach-aspeed/ast2600/secure-boot.c | 53 ++++++++++++++++++++++
>   2 files changed, 54 insertions(+)
>   create mode 100644 arch/arm/mach-aspeed/ast2600/secure-boot.c
>
> diff --git a/arch/arm/mach-aspeed/ast2600/Makefile b/arch/arm/mach-aspeed/ast2600/Makefile
> index d07e8c737cfe..70b7ae11df56 100644
> --- a/arch/arm/mach-aspeed/ast2600/Makefile
> +++ b/arch/arm/mach-aspeed/ast2600/Makefile
> @@ -1,4 +1,5 @@
>   obj-y   += platform.o board_common.o scu_info.o utils.o cache.o
> +obj-$(CONFIG_FIT_RUNTIME_SIGNATURE) += secure-boot.o
>   obj-$(CONFIG_ASPEED_SECURE_BOOT) += crypto.o aspeed_verify.o
>   obj-$(CONFIG_ASPEED_LOADERS) += spl_boot.o
>   obj-$(CONFIG_SPL_BUILD) += spl.o
> diff --git a/arch/arm/mach-aspeed/ast2600/secure-boot.c b/arch/arm/mach-aspeed/ast2600/secure-boot.c
> new file mode 100644
> index 000000000000..ced353686387
> --- /dev/null
> +++ b/arch/arm/mach-aspeed/ast2600/secure-boot.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// (C) Copyright IBM Corp. 2022
> +
> +#include <common.h>
> +#include <asm-generic/gpio.h>
> +#include <dm.h>
> +
> +static int aspeed_get_chained_secboot_state(void)
> +{
> +	struct gpio_desc desc;
> +	struct udevice *dev;
> +	int secboot;
> +	int rc;
> +
> +	rc = uclass_get_device_by_driver(UCLASS_GPIO,
> +					 DM_GET_DRIVER(gpio_aspeed),
> +					 &dev);
> +	if (rc < 0) {
> +		debug("Warning: GPIO initialization failure: %d\n", rc);
> +		return rc;
> +	}
> +
> +	rc = gpio_request_by_line_name(dev, "bmc-secure-boot", &desc,
> +				       GPIOD_IS_IN);
> +	if (rc < 0) {
> +		debug("Failed to acquire secure-boot GPIO: %d\n", rc);
> +		return rc;
> +	}
> +
> +	secboot = dm_gpio_get_value(&desc);
> +	if (secboot < 0)
> +		debug("Failed to read secure-boot GPIO value: %d\n", rc);
> +
> +	rc = dm_gpio_free(dev, &desc);
> +	if (rc < 0)
> +		debug("Failed to free secure-boot GPIO: %d\n", rc);
> +
> +	return secboot;
> +}
> +
> +int board_fit_image_require_verified(void)
> +{
> +	int secboot;
> +
> +	secboot = aspeed_get_chained_secboot_state();
> +
> +	/*
> +	 * If secure-boot is enabled then require signature verification.
> +	 * Otherwise, if we fail to read the GPIO, enforce FIT signature
> +	 * verification
> +	 */
> +	return secboot >= 0 ? secboot : 1;
> +}

[PATCH u-boot v2019.04-aspeed-openbmc 4/6] configs: ast2600: Runtime control of FIT signature verification

[Andrew Jeffery]

Turn on runtime control of FIT signature verification for systems using
the ast2600_openbmc_spl_emmc_defconfig, such as IBM's Rainier platform.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 configs/ast2600_openbmc_spl_emmc_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/ast2600_openbmc_spl_emmc_defconfig b/configs/ast2600_openbmc_spl_emmc_defconfig
index 5f50298a589c..a3d229d786b8 100644
--- a/configs/ast2600_openbmc_spl_emmc_defconfig
+++ b/configs/ast2600_openbmc_spl_emmc_defconfig
@@ -29,6 +29,7 @@ CONFIG_NR_DRAM_BANKS=1
 CONFIG_FIT=y
 CONFIG_FIT_ENABLE_SHA512_SUPPORT=y
 CONFIG_FIT_SIGNATURE=y
+CONFIG_FIT_RUNTIME_SIGNATURE=y
 CONFIG_SPL_FIT_SIGNATURE=y
 CONFIG_SPL_LOAD_FIT=y
 CONFIG_USE_BOOTARGS=y
-- 
2.32.0

Re: [PATCH u-boot v2019.04-aspeed-openbmc 4/6] configs: ast2600: Runtime control of FIT signature verification

[Eddie James]

On 1/30/22 19:25, Andrew Jeffery wrote:
> Turn on runtime control of FIT signature verification for systems using
> the ast2600_openbmc_spl_emmc_defconfig, such as IBM's Rainier platform.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   configs/ast2600_openbmc_spl_emmc_defconfig | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/configs/ast2600_openbmc_spl_emmc_defconfig b/configs/ast2600_openbmc_spl_emmc_defconfig
> index 5f50298a589c..a3d229d786b8 100644
> --- a/configs/ast2600_openbmc_spl_emmc_defconfig
> +++ b/configs/ast2600_openbmc_spl_emmc_defconfig
> @@ -29,6 +29,7 @@ CONFIG_NR_DRAM_BANKS=1
>   CONFIG_FIT=y
>   CONFIG_FIT_ENABLE_SHA512_SUPPORT=y
>   CONFIG_FIT_SIGNATURE=y
> +CONFIG_FIT_RUNTIME_SIGNATURE=y
>   CONFIG_SPL_FIT_SIGNATURE=y
>   CONFIG_SPL_LOAD_FIT=y
>   CONFIG_USE_BOOTARGS=y

[PATCH u-boot v2019.04-aspeed-openbmc 5/6] ARM: dts: rainier: Add gpio-line-names property with bmc-secure-boot

[Andrew Jeffery]

The "bmc-secure-boot" GPIO controls at runtime whether FIT signature
verification is performed by u-boot and the SPL.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/dts/ast2600-rainier.dts | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm/dts/ast2600-rainier.dts b/arch/arm/dts/ast2600-rainier.dts
index d0e82d151239..af35afb911fb 100755
--- a/arch/arm/dts/ast2600-rainier.dts
+++ b/arch/arm/dts/ast2600-rainier.dts
@@ -104,6 +104,38 @@
 	sdhci-drive-type = <1>;
 };
 
+&gpio0 {
+	status = "okay";
+
+	gpio-line-names =
+	/*A0-A7*/	"","","","","","","","",
+	/*B0-B7*/	"","","","","","","","",
+	/*C0-C7*/	"","","","","","","","",
+	/*D0-D7*/	"","","","","","","","",
+	/*E0-E7*/	"","","","","","","","",
+	/*F0-F7*/	"","","","","","","","",
+	/*G0-G7*/	"","","","","","","","",
+	/*H0-H7*/	"","","","","","","","",
+	/*I0-I7*/	"","","","","","","bmc-secure-boot","",
+	/*J0-J7*/	"","","","","","","","",
+	/*K0-K7*/	"","","","","","","","",
+	/*L0-L7*/	"","","","","","","","",
+	/*M0-M7*/	"","","","","","","","",
+	/*N0-N7*/	"","","","","","","","",
+	/*O0-O7*/	"","","","","","","","",
+	/*P0-P7*/	"","","","","","","","",
+	/*Q0-Q7*/	"","","","","","","","",
+	/*R0-R7*/	"","","","","","","","",
+	/*S0-S7*/	"","","","","","","","",
+	/*T0-T7*/	"","","","","","","","",
+	/*U0-U7*/	"","","","","","","","",
+	/*V0-V7*/	"","","","","","","","",
+	/*W0-W7*/	"","","","","","","","",
+	/*X0-X7*/	"","","","","","","","",
+	/*Y0-Y7*/	"","","","","","","","",
+	/*Z0-Z7*/	"","","","","","","","";
+};
+
 &hace {
 	u-boot,dm-pre-reloc;
 	status = "okay";
-- 
2.32.0

Re: [PATCH u-boot v2019.04-aspeed-openbmc 5/6] ARM: dts: rainier: Add gpio-line-names property with bmc-secure-boot

[Eddie James]

On 1/30/22 19:25, Andrew Jeffery wrote:
> The "bmc-secure-boot" GPIO controls at runtime whether FIT signature
> verification is performed by u-boot and the SPL.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   arch/arm/dts/ast2600-rainier.dts | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm/dts/ast2600-rainier.dts b/arch/arm/dts/ast2600-rainier.dts
> index d0e82d151239..af35afb911fb 100755
> --- a/arch/arm/dts/ast2600-rainier.dts
> +++ b/arch/arm/dts/ast2600-rainier.dts
> @@ -104,6 +104,38 @@
>   	sdhci-drive-type = <1>;
>   };
>   
> +&gpio0 {
> +	status = "okay";


I'm surprised you don't need the u-boot,dm-pre-reloc flag here?


> +
> +	gpio-line-names =
> +	/*A0-A7*/	"","","","","","","","",
> +	/*B0-B7*/	"","","","","","","","",
> +	/*C0-C7*/	"","","","","","","","",
> +	/*D0-D7*/	"","","","","","","","",
> +	/*E0-E7*/	"","","","","","","","",
> +	/*F0-F7*/	"","","","","","","","",
> +	/*G0-G7*/	"","","","","","","","",
> +	/*H0-H7*/	"","","","","","","","",
> +	/*I0-I7*/	"","","","","","","bmc-secure-boot","",
> +	/*J0-J7*/	"","","","","","","","",
> +	/*K0-K7*/	"","","","","","","","",
> +	/*L0-L7*/	"","","","","","","","",
> +	/*M0-M7*/	"","","","","","","","",
> +	/*N0-N7*/	"","","","","","","","",
> +	/*O0-O7*/	"","","","","","","","",
> +	/*P0-P7*/	"","","","","","","","",
> +	/*Q0-Q7*/	"","","","","","","","",
> +	/*R0-R7*/	"","","","","","","","",
> +	/*S0-S7*/	"","","","","","","","",
> +	/*T0-T7*/	"","","","","","","","",
> +	/*U0-U7*/	"","","","","","","","",
> +	/*V0-V7*/	"","","","","","","","",
> +	/*W0-W7*/	"","","","","","","","",
> +	/*X0-X7*/	"","","","","","","","",
> +	/*Y0-Y7*/	"","","","","","","","",
> +	/*Z0-Z7*/	"","","","","","","","";
> +};
> +
>   &hace {
>   	u-boot,dm-pre-reloc;
>   	status = "okay";

[PATCH u-boot v2019.04-aspeed-openbmc 6/6] image: Fix indentation of macros

[Andrew Jeffery]

Make it clear which level of nesting they belong to.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 include/image.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/image.h b/include/image.h
index 19ea743af08f..9bccf00bb5cb 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1093,8 +1093,8 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 # ifdef USE_HOSTCC
 #  define IMAGE_ENABLE_SIGN	1
 #  define IMAGE_ENABLE_VERIFY	1
-# include  <openssl/evp.h>
-#else
+#  include  <openssl/evp.h>
+# else
 #  define IMAGE_ENABLE_SIGN	0
 #  define IMAGE_ENABLE_VERIFY	1
 # endif
-- 
2.32.0

Re: [PATCH u-boot v2019.04-aspeed-openbmc 6/6] image: Fix indentation of macros

[Eddie James]

On 1/30/22 19:25, Andrew Jeffery wrote:
> Make it clear which level of nesting they belong to.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   include/image.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/image.h b/include/image.h
> index 19ea743af08f..9bccf00bb5cb 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1093,8 +1093,8 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>   # ifdef USE_HOSTCC
>   #  define IMAGE_ENABLE_SIGN	1
>   #  define IMAGE_ENABLE_VERIFY	1
> -# include  <openssl/evp.h>
> -#else
> +#  include  <openssl/evp.h>
> +# else
>   #  define IMAGE_ENABLE_SIGN	0
>   #  define IMAGE_ENABLE_VERIFY	1
>   # endif