[PATCH 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[PATCH 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[Marek Vasut]

In case Dcache is enabled while the ECDSA authentication function is
called via BootROM ROM API, the CRYP DMA might pick stale version of
data from DRAM. Disable Dcache around the BootROM call to avoid this
issue.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
 arch/arm/mach-stm32mp/ecdsa_romapi.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
index a2f63ff879f..72b87bf2c64 100644
--- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
+++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
@@ -64,6 +64,7 @@ static int romapi_ecdsa_verify(struct udevice *dev,
 			       const void *signature, size_t sig_len)
 {
 	struct ecdsa_rom_api rom;
+	bool reenable_dcache;
 	uint8_t raw_key[64];
 	uint32_t rom_ret;
 	int algo;
@@ -81,8 +82,21 @@ static int romapi_ecdsa_verify(struct udevice *dev,
 	memcpy(raw_key + 32, pubkey->y, 32);
 
 	stm32mp_rom_get_ecdsa_functions(&rom);
+
+	/*
+	 * Disable D-cache before calling into BootROM, else CRYP DMA
+	 * may fail to pick up the correct data.
+	 */
+	if (dcache_status()) {
+		dcache_disable();
+		reenable_dcache = true;
+	}
+
 	rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, algo);
 
+	if (reenable_dcache)
+		dcache_enable();
+
 	return rom_ret == ROM_API_SUCCESS ? 0 : -EPERM;
 }
 
-- 
2.35.1

[PATCH 2/4] ARM: stm32: Factor out save_boot_params

[Marek Vasut]

The STM32MP15xx platform currently comes with two incompatible
implementations of save_boot_params() weak function override.
Factor the save_boot_params() implementation into common cpu.c
code and provide accessors to read out both ROM API table address
and DT address from any place in the code instead.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
 arch/arm/mach-stm32mp/boot_params.c           | 20 ++---------
 arch/arm/mach-stm32mp/cpu.c                   | 35 +++++++++++++++++++
 arch/arm/mach-stm32mp/ecdsa_romapi.c          | 20 ++---------
 .../arm/mach-stm32mp/include/mach/sys_proto.h |  3 ++
 4 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/arch/arm/mach-stm32mp/boot_params.c b/arch/arm/mach-stm32mp/boot_params.c
index e91ef1b2fc7..e40cca938ef 100644
--- a/arch/arm/mach-stm32mp/boot_params.c
+++ b/arch/arm/mach-stm32mp/boot_params.c
@@ -11,30 +11,14 @@
 #include <asm/sections.h>
 #include <asm/system.h>
 
-/*
- * Force data-section, as .bss will not be valid
- * when save_boot_params is invoked.
- */
-static unsigned long nt_fw_dtb __section(".data");
-
-/*
- * Save the FDT address provided by TF-A in r2 at boot time
- * This function is called from start.S
- */
-void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
-		      unsigned long r3)
-{
-	nt_fw_dtb = r2;
-
-	save_boot_params_ret();
-}
-
 /*
  * Use the saved FDT address provided by TF-A at boot time (NT_FW_CONFIG =
  * Non Trusted Firmware configuration file) when the pointer is valid
  */
 void *board_fdt_blob_setup(int *err)
 {
+	unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
+
 	log_debug("%s: nt_fw_dtb=%lx\n", __func__, nt_fw_dtb);
 
 	*err = 0;
diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index 855fc755fe0..fa02a11d867 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -378,3 +378,38 @@ int arch_misc_init(void)
 
 	return 0;
 }
+
+/*
+ * Without forcing the ".data" section, this would get saved in ".bss". BSS
+ * will be cleared soon after, so it's not suitable.
+ */
+static uintptr_t rom_api_table __section(".data");
+static uintptr_t nt_fw_dtb __section(".data");
+
+/*
+ * The ROM gives us the API location in r0 when starting. This is only available
+ * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot. Save
+ * the FDT address provided by TF-A in r2 at boot time. This function is called
+ * from start.S
+ */
+void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
+		      unsigned long r3)
+{
+#if IS_ENABLED(CONFIG_STM32_ECDSA_VERIFY)
+	rom_api_table = r0;
+#endif
+#if IS_ENABLED(CONFIG_TFABOOT)
+	nt_fw_dtb = r2;
+#endif
+	save_boot_params_ret();
+}
+
+uintptr_t get_stm32mp_rom_api_table(void)
+{
+	return rom_api_table;
+}
+
+uintptr_t get_stm32mp_bl2_dtb(void)
+{
+	return nt_fw_dtb;
+}
diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
index 72b87bf2c64..32a7357ad56 100644
--- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
+++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
@@ -24,26 +24,10 @@ struct ecdsa_rom_api {
 					   uint32_t ecc_algo);
 };
 
-/*
- * Without forcing the ".data" section, this would get saved in ".bss". BSS
- * will be cleared soon after, so it's not suitable.
- */
-static uintptr_t rom_api_loc __section(".data");
-
-/*
- * The ROM gives us the API location in r0 when starting. This is only available
- * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot.
- */
-void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
-		      unsigned long r3)
-{
-	rom_api_loc = r0;
-	save_boot_params_ret();
-}
-
 static void stm32mp_rom_get_ecdsa_functions(struct ecdsa_rom_api *rom)
 {
-	uintptr_t verify_ptr = rom_api_loc + ROM_API_OFFSET_ECDSA_VERIFY;
+	uintptr_t verify_ptr = get_stm32mp_rom_api_table() +
+			       ROM_API_OFFSET_ECDSA_VERIFY;
 
 	rom->ecdsa_verify_signature = *(void **)verify_ptr;
 }
diff --git a/arch/arm/mach-stm32mp/include/mach/sys_proto.h b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
index f19a70e53e0..0d39b67178e 100644
--- a/arch/arm/mach-stm32mp/include/mach/sys_proto.h
+++ b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
@@ -77,3 +77,6 @@ void stm32mp_misc_init(void);
 
 /* helper function: read data from OTP */
 u32 get_otp(int index, int shift, int mask);
+
+uintptr_t get_stm32mp_rom_api_table(void);
+uintptr_t get_stm32mp_bl2_dtb(void);
-- 
2.35.1

[PATCH 3/4] ARM: stm32: Pass ROM API table pointer to U-Boot proper

[Marek Vasut]

The ROM API table pointer is no longer accessible from U-Boot, fix
this by passing the ROM API pointer through. This makes it possible
for U-Boot to call ROM API functions to authenticate payload like
signed fitImages.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
 arch/arm/mach-stm32mp/cpu.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index fa02a11d867..9553ecd243c 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -22,6 +22,7 @@
 #include <dm/device.h>
 #include <dm/uclass.h>
 #include <linux/bitops.h>
+#include <spl.h>
 
 /*
  * early TLB into the .data section so that it not get cleared
@@ -413,3 +414,17 @@ uintptr_t get_stm32mp_bl2_dtb(void)
 {
 	return nt_fw_dtb;
 }
+
+#ifdef CONFIG_SPL_BUILD
+void jump_to_image_no_args(struct spl_image_info *spl_image)
+{
+	typedef void __noreturn (*image_entry_noargs_t)(u32 romapi);
+	uintptr_t romapi = get_stm32mp_rom_api_table();
+
+	image_entry_noargs_t image_entry =
+		(image_entry_noargs_t)spl_image->entry_point;
+
+	printf("image entry point: 0x%lx\n", spl_image->entry_point);
+	image_entry(romapi);
+}
+#endif
-- 
2.35.1

[PATCH 4/4] ARM: stm32: Make ECDSA authentication available to U-Boot

[Marek Vasut]

With U-Boot having access to ROM API call table, it is possible to use
the ROM API call it authenticate e.g. signed kernel fitImages using the
BootROM ECDSA support. Make this available by pulling the ECDSA BootROM
call support from SPL-only guard.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
 arch/arm/mach-stm32mp/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-stm32mp/Makefile b/arch/arm/mach-stm32mp/Makefile
index 1db9057e049..a19b2797c8b 100644
--- a/arch/arm/mach-stm32mp/Makefile
+++ b/arch/arm/mach-stm32mp/Makefile
@@ -11,10 +11,10 @@ obj-y += bsec.o
 obj-$(CONFIG_STM32MP13x) += stm32mp13x.o
 obj-$(CONFIG_STM32MP15x) += stm32mp15x.o
 
+obj-$(CONFIG_STM32_ECDSA_VERIFY) += ecdsa_romapi.o
 ifdef CONFIG_SPL_BUILD
 obj-y += spl.o
 obj-y += tzc400.o
-obj-$(CONFIG_STM32_ECDSA_VERIFY) += ecdsa_romapi.o
 else
 obj-y += cmd_stm32prog/
 obj-$(CONFIG_CMD_STM32KEY) += cmd_stm32key.o
-- 
2.35.1

Re: [PATCH 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[Patrice CHOTARD]

On 12/6/22 03:33, Marek Vasut wrote:
> In case Dcache is enabled while the ECDSA authentication function is
> called via BootROM ROM API, the CRYP DMA might pick stale version of
> data from DRAM. Disable Dcache around the BootROM call to avoid this
> issue.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>  arch/arm/mach-stm32mp/ecdsa_romapi.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> index a2f63ff879f..72b87bf2c64 100644
> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> @@ -64,6 +64,7 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>  			       const void *signature, size_t sig_len)
>  {
>  	struct ecdsa_rom_api rom;
> +	bool reenable_dcache;
>  	uint8_t raw_key[64];
>  	uint32_t rom_ret;
>  	int algo;
> @@ -81,8 +82,21 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>  	memcpy(raw_key + 32, pubkey->y, 32);
>  
>  	stm32mp_rom_get_ecdsa_functions(&rom);
> +
> +	/*
> +	 * Disable D-cache before calling into BootROM, else CRYP DMA
> +	 * may fail to pick up the correct data.
> +	 */
> +	if (dcache_status()) {
> +		dcache_disable();
> +		reenable_dcache = true;
> +	}
> +
>  	rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, algo);
>  
> +	if (reenable_dcache)
> +		dcache_enable();
> +
>  	return rom_ret == ROM_API_SUCCESS ? 0 : -EPERM;
>  }
>  
Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>
Thanks
Patrice

Re: [PATCH 2/4] ARM: stm32: Factor out save_boot_params

[Patrice CHOTARD]

On 12/6/22 03:33, Marek Vasut wrote:
> The STM32MP15xx platform currently comes with two incompatible
> implementations of save_boot_params() weak function override.
> Factor the save_boot_params() implementation into common cpu.c
> code and provide accessors to read out both ROM API table address
> and DT address from any place in the code instead.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>  arch/arm/mach-stm32mp/boot_params.c           | 20 ++---------
>  arch/arm/mach-stm32mp/cpu.c                   | 35 +++++++++++++++++++
>  arch/arm/mach-stm32mp/ecdsa_romapi.c          | 20 ++---------
>  .../arm/mach-stm32mp/include/mach/sys_proto.h |  3 ++
>  4 files changed, 42 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/arm/mach-stm32mp/boot_params.c b/arch/arm/mach-stm32mp/boot_params.c
> index e91ef1b2fc7..e40cca938ef 100644
> --- a/arch/arm/mach-stm32mp/boot_params.c
> +++ b/arch/arm/mach-stm32mp/boot_params.c
> @@ -11,30 +11,14 @@
>  #include <asm/sections.h>
>  #include <asm/system.h>
>  
> -/*
> - * Force data-section, as .bss will not be valid
> - * when save_boot_params is invoked.
> - */
> -static unsigned long nt_fw_dtb __section(".data");
> -
> -/*
> - * Save the FDT address provided by TF-A in r2 at boot time
> - * This function is called from start.S
> - */
> -void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
> -		      unsigned long r3)
> -{
> -	nt_fw_dtb = r2;
> -
> -	save_boot_params_ret();
> -}
> -
>  /*
>   * Use the saved FDT address provided by TF-A at boot time (NT_FW_CONFIG =
>   * Non Trusted Firmware configuration file) when the pointer is valid
>   */
>  void *board_fdt_blob_setup(int *err)
>  {
> +	unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
> +
>  	log_debug("%s: nt_fw_dtb=%lx\n", __func__, nt_fw_dtb);
>  
>  	*err = 0;
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index 855fc755fe0..fa02a11d867 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -378,3 +378,38 @@ int arch_misc_init(void)
>  
>  	return 0;
>  }
> +
> +/*
> + * Without forcing the ".data" section, this would get saved in ".bss". BSS
> + * will be cleared soon after, so it's not suitable.
> + */
> +static uintptr_t rom_api_table __section(".data");
> +static uintptr_t nt_fw_dtb __section(".data");
> +
> +/*
> + * The ROM gives us the API location in r0 when starting. This is only available
> + * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot. Save
> + * the FDT address provided by TF-A in r2 at boot time. This function is called
> + * from start.S
> + */
> +void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
> +		      unsigned long r3)
> +{
> +#if IS_ENABLED(CONFIG_STM32_ECDSA_VERIFY)
> +	rom_api_table = r0;
> +#endif
> +#if IS_ENABLED(CONFIG_TFABOOT)
> +	nt_fw_dtb = r2;
> +#endif
> +	save_boot_params_ret();
> +}
> +
> +uintptr_t get_stm32mp_rom_api_table(void)
> +{
> +	return rom_api_table;
> +}
> +
> +uintptr_t get_stm32mp_bl2_dtb(void)
> +{
> +	return nt_fw_dtb;
> +}
> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> index 72b87bf2c64..32a7357ad56 100644
> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> @@ -24,26 +24,10 @@ struct ecdsa_rom_api {
>  					   uint32_t ecc_algo);
>  };
>  
> -/*
> - * Without forcing the ".data" section, this would get saved in ".bss". BSS
> - * will be cleared soon after, so it's not suitable.
> - */
> -static uintptr_t rom_api_loc __section(".data");
> -
> -/*
> - * The ROM gives us the API location in r0 when starting. This is only available
> - * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot.
> - */
> -void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
> -		      unsigned long r3)
> -{
> -	rom_api_loc = r0;
> -	save_boot_params_ret();
> -}
> -
>  static void stm32mp_rom_get_ecdsa_functions(struct ecdsa_rom_api *rom)
>  {
> -	uintptr_t verify_ptr = rom_api_loc + ROM_API_OFFSET_ECDSA_VERIFY;
> +	uintptr_t verify_ptr = get_stm32mp_rom_api_table() +
> +			       ROM_API_OFFSET_ECDSA_VERIFY;
>  
>  	rom->ecdsa_verify_signature = *(void **)verify_ptr;
>  }
> diff --git a/arch/arm/mach-stm32mp/include/mach/sys_proto.h b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
> index f19a70e53e0..0d39b67178e 100644
> --- a/arch/arm/mach-stm32mp/include/mach/sys_proto.h
> +++ b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
> @@ -77,3 +77,6 @@ void stm32mp_misc_init(void);
>  
>  /* helper function: read data from OTP */
>  u32 get_otp(int index, int shift, int mask);
> +
> +uintptr_t get_stm32mp_rom_api_table(void);
> +uintptr_t get_stm32mp_bl2_dtb(void);

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [PATCH 3/4] ARM: stm32: Pass ROM API table pointer to U-Boot proper

[Patrice CHOTARD]

On 12/6/22 03:33, Marek Vasut wrote:
> The ROM API table pointer is no longer accessible from U-Boot, fix
> this by passing the ROM API pointer through. This makes it possible
> for U-Boot to call ROM API functions to authenticate payload like
> signed fitImages.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>  arch/arm/mach-stm32mp/cpu.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index fa02a11d867..9553ecd243c 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -22,6 +22,7 @@
>  #include <dm/device.h>
>  #include <dm/uclass.h>
>  #include <linux/bitops.h>
> +#include <spl.h>
>  
>  /*
>   * early TLB into the .data section so that it not get cleared
> @@ -413,3 +414,17 @@ uintptr_t get_stm32mp_bl2_dtb(void)
>  {
>  	return nt_fw_dtb;
>  }
> +
> +#ifdef CONFIG_SPL_BUILD
> +void jump_to_image_no_args(struct spl_image_info *spl_image)
> +{
> +	typedef void __noreturn (*image_entry_noargs_t)(u32 romapi);
> +	uintptr_t romapi = get_stm32mp_rom_api_table();
> +
> +	image_entry_noargs_t image_entry =
> +		(image_entry_noargs_t)spl_image->entry_point;
> +
> +	printf("image entry point: 0x%lx\n", spl_image->entry_point);
> +	image_entry(romapi);
> +}
> +#endif

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [PATCH 4/4] ARM: stm32: Make ECDSA authentication available to U-Boot

[Patrice CHOTARD]

On 12/6/22 03:33, Marek Vasut wrote:
> With U-Boot having access to ROM API call table, it is possible to use
> the ROM API call it authenticate e.g. signed kernel fitImages using the
> BootROM ECDSA support. Make this available by pulling the ECDSA BootROM
> call support from SPL-only guard.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>  arch/arm/mach-stm32mp/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-stm32mp/Makefile b/arch/arm/mach-stm32mp/Makefile
> index 1db9057e049..a19b2797c8b 100644
> --- a/arch/arm/mach-stm32mp/Makefile
> +++ b/arch/arm/mach-stm32mp/Makefile
> @@ -11,10 +11,10 @@ obj-y += bsec.o
>  obj-$(CONFIG_STM32MP13x) += stm32mp13x.o
>  obj-$(CONFIG_STM32MP15x) += stm32mp15x.o
>  
> +obj-$(CONFIG_STM32_ECDSA_VERIFY) += ecdsa_romapi.o
>  ifdef CONFIG_SPL_BUILD
>  obj-y += spl.o
>  obj-y += tzc400.o
> -obj-$(CONFIG_STM32_ECDSA_VERIFY) += ecdsa_romapi.o
>  else
>  obj-y += cmd_stm32prog/
>  obj-$(CONFIG_CMD_STM32KEY) += cmd_stm32key.o

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [PATCH 2/4] ARM: stm32: Factor out save_boot_params

[Patrick DELAUNAY]

Hi Marek,

On 12/6/22 03:33, Marek Vasut wrote:
> The STM32MP15xx platform currently comes with two incompatible
> implementations of save_boot_params() weak function override.
> Factor the save_boot_params() implementation into common cpu.c
> code and provide accessors to read out both ROM API table address
> and DT address from any place in the code instead.


good idea.


>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>   arch/arm/mach-stm32mp/boot_params.c           | 20 ++---------
>   arch/arm/mach-stm32mp/cpu.c                   | 35 +++++++++++++++++++
>   arch/arm/mach-stm32mp/ecdsa_romapi.c          | 20 ++---------
>   .../arm/mach-stm32mp/include/mach/sys_proto.h |  3 ++
>   4 files changed, 42 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/boot_params.c b/arch/arm/mach-stm32mp/boot_params.c
> index e91ef1b2fc7..e40cca938ef 100644
> --- a/arch/arm/mach-stm32mp/boot_params.c
> +++ b/arch/arm/mach-stm32mp/boot_params.c
> @@ -11,30 +11,14 @@
>   #include <asm/sections.h>
>   #include <asm/system.h>
>   
> -/*
> - * Force data-section, as .bss will not be valid
> - * when save_boot_params is invoked.
> - */
> -static unsigned long nt_fw_dtb __section(".data");
> -
> -/*
> - * Save the FDT address provided by TF-A in r2 at boot time
> - * This function is called from start.S
> - */
> -void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
> -		      unsigned long r3)
> -{
> -	nt_fw_dtb = r2;
> -
> -	save_boot_params_ret();
> -}
> -
>   /*
>    * Use the saved FDT address provided by TF-A at boot time (NT_FW_CONFIG =
>    * Non Trusted Firmware configuration file) when the pointer is valid
>    */
>   void *board_fdt_blob_setup(int *err)
>   {
> +	unsigned long nt_fw_dtb = get_stm32mp_bl2_dtb();
> +
>   	log_debug("%s: nt_fw_dtb=%lx\n", __func__, nt_fw_dtb);
>   
>   	*err = 0;
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index 855fc755fe0..fa02a11d867 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -378,3 +378,38 @@ int arch_misc_init(void)
>   
>   	return 0;
>   }
> +
> +/*
> + * Without forcing the ".data" section, this would get saved in ".bss". BSS
> + * will be cleared soon after, so it's not suitable.
> + */
> +static uintptr_t rom_api_table __section(".data");
> +static uintptr_t nt_fw_dtb __section(".data");
> +
> +/*
> + * The ROM gives us the API location in r0 when starting. This is only available
> + * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot. Save
> + * the FDT address provided by TF-A in r2 at boot time. This function is called
> + * from start.S
> + */
> +void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
> +		      unsigned long r3)
> +{
> +#if IS_ENABLED(CONFIG_STM32_ECDSA_VERIFY)
> +	rom_api_table = r0;
> +#endif
> +#if IS_ENABLED(CONFIG_TFABOOT)
> +	nt_fw_dtb = r2;
> +#endif

jut avoid #if when it is possible.


if (IS_ENABLED(CONFIG_STM32_ECDSA_VERIFY))

	rom_api_table = r0;

if (IS_ENABLED(CONFIG_TFABOOT))

	nt_fw_dtb = r2;


> +	save_boot_params_ret();
> +}
> +
> +uintptr_t get_stm32mp_rom_api_table(void)
> +{
> +	return rom_api_table;
> +}
> +
> +uintptr_t get_stm32mp_bl2_dtb(void)
> +{
> +	return nt_fw_dtb;
> +}
> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> index 72b87bf2c64..32a7357ad56 100644
> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> @@ -24,26 +24,10 @@ struct ecdsa_rom_api {
>   					   uint32_t ecc_algo);
>   };
>   
> -/*
> - * Without forcing the ".data" section, this would get saved in ".bss". BSS
> - * will be cleared soon after, so it's not suitable.
> - */
> -static uintptr_t rom_api_loc __section(".data");
> -
> -/*
> - * The ROM gives us the API location in r0 when starting. This is only available
> - * during SPL, as there isn't (yet) a mechanism to pass this on to u-boot.
> - */
> -void save_boot_params(unsigned long r0, unsigned long r1, unsigned long r2,
> -		      unsigned long r3)
> -{
> -	rom_api_loc = r0;
> -	save_boot_params_ret();
> -}
> -
>   static void stm32mp_rom_get_ecdsa_functions(struct ecdsa_rom_api *rom)
>   {
> -	uintptr_t verify_ptr = rom_api_loc + ROM_API_OFFSET_ECDSA_VERIFY;
> +	uintptr_t verify_ptr = get_stm32mp_rom_api_table() +
> +			       ROM_API_OFFSET_ECDSA_VERIFY;
>   
>   	rom->ecdsa_verify_signature = *(void **)verify_ptr;
>   }
> diff --git a/arch/arm/mach-stm32mp/include/mach/sys_proto.h b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
> index f19a70e53e0..0d39b67178e 100644
> --- a/arch/arm/mach-stm32mp/include/mach/sys_proto.h
> +++ b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
> @@ -77,3 +77,6 @@ void stm32mp_misc_init(void);
>   
>   /* helper function: read data from OTP */
>   u32 get_otp(int index, int shift, int mask);
> +
> +uintptr_t get_stm32mp_rom_api_table(void);
> +uintptr_t get_stm32mp_bl2_dtb(void);


regards

Patrick

Re: [PATCH 3/4] ARM: stm32: Pass ROM API table pointer to U-Boot proper

[Patrick DELAUNAY]

Hi,

minor comment

On 12/6/22 03:33, Marek Vasut wrote:
> The ROM API table pointer is no longer accessible from U-Boot, fix
> this by passing the ROM API pointer through. This makes it possible
> for U-Boot to call ROM API functions to authenticate payload like
> signed fitImages.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>   arch/arm/mach-stm32mp/cpu.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index fa02a11d867..9553ecd243c 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -22,6 +22,7 @@
>   #include <dm/device.h>
>   #include <dm/uclass.h>
>   #include <linux/bitops.h>
> +#include <spl.h>
>   
>   /*
>    * early TLB into the .data section so that it not get cleared
> @@ -413,3 +414,17 @@ uintptr_t get_stm32mp_bl2_dtb(void)
>   {
>   	return nt_fw_dtb;
>   }
> +
> +#ifdef CONFIG_SPL_BUILD
> +void jump_to_image_no_args(struct spl_image_info *spl_image)

missing "__noreturn" ?


void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)

> +{
> +	typedef void __noreturn (*image_entry_noargs_t)(u32 romapi);

really 'noargs' ?

  I propose to replace to 'arg' => image_entry_arg_t

arch/powerpc/lib/spl.c:20

base on arch/arm/lib/spl.c:71

arch/microblaze/cpu/spl.c:37

typedef void __noreturn (*image_entry_noargs_t)(u32 romapi);


or with stm32:

typedef void __noreturn (*image_entry_stm32_t)(u32 romapi);

based on arch/riscv/lib/spl.c:40


> +	uintptr_t romapi = get_stm32mp_rom_api_table();
> +
> +	image_entry_noargs_t image_entry =
> +		(image_entry_noargs_t)spl_image->entry_point;
> +
> +	printf("image entry point: 0x%lx\n", spl_image->entry_point);
> +	image_entry(romapi);
> +}
> +#endif


regards

Patrick

Re: [PATCH 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[Patrice CHOTARD]

just one remark 

On 12/6/22 03:33, Marek Vasut wrote:
> In case Dcache is enabled while the ECDSA authentication function is
> called via BootROM ROM API, the CRYP DMA might pick stale version of
> data from DRAM. Disable Dcache around the BootROM call to avoid this
> issue.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>  arch/arm/mach-stm32mp/ecdsa_romapi.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> index a2f63ff879f..72b87bf2c64 100644
> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> @@ -64,6 +64,7 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>  			       const void *signature, size_t sig_len)
>  {
>  	struct ecdsa_rom_api rom;
> +	bool reenable_dcache;

reenable_dcache is used without being initialized

>  	uint8_t raw_key[64];
>  	uint32_t rom_ret;
>  	int algo;
> @@ -81,8 +82,21 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>  	memcpy(raw_key + 32, pubkey->y, 32);
>  
>  	stm32mp_rom_get_ecdsa_functions(&rom);
> +
> +	/*
> +	 * Disable D-cache before calling into BootROM, else CRYP DMA
> +	 * may fail to pick up the correct data.
> +	 */
> +	if (dcache_status()) {
> +		dcache_disable();
> +		reenable_dcache = true;
> +	}
> +
>  	rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, algo);
>  
> +	if (reenable_dcache)
> +		dcache_enable();
> +
>  	return rom_ret == ROM_API_SUCCESS ? 0 : -EPERM;
>  }
>  

Re: [PATCH 4/4] ARM: stm32: Make ECDSA authentication available to U-Boot

[Patrick DELAUNAY]

Hi,


On 12/6/22 03:33, Marek Vasut wrote:
> With U-Boot having access to ROM API call table, it is possible to use
> the ROM API call it authenticate e.g. signed kernel fitImages using the
> BootROM ECDSA support. Make this available by pulling the ECDSA BootROM
> call support from SPL-only guard.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>   arch/arm/mach-stm32mp/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>


Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

Re: [PATCH 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[Patrick DELAUNAY]

Hi Marek,

On 12/6/22 03:33, Marek Vasut wrote:
> In case Dcache is enabled while the ECDSA authentication function is
> called via BootROM ROM API, the CRYP DMA might pick stale version of
> data from DRAM. Disable Dcache around the BootROM call to avoid this
> issue.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>   arch/arm/mach-stm32mp/ecdsa_romapi.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> index a2f63ff879f..72b87bf2c64 100644
> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
> @@ -64,6 +64,7 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>   			       const void *signature, size_t sig_len)
>   {
>   	struct ecdsa_rom_api rom;
> +	bool reenable_dcache;
>   	uint8_t raw_key[64];
>   	uint32_t rom_ret;
>   	int algo;
> @@ -81,8 +82,21 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>   	memcpy(raw_key + 32, pubkey->y, 32);
>   
>   	stm32mp_rom_get_ecdsa_functions(&rom);
> +
> +	/*
> +	 * Disable D-cache before calling into BootROM, else CRYP DMA
> +	 * may fail to pick up the correct data.
> +	 */
> +	if (dcache_status()) {
> +		dcache_disable();
> +		reenable_dcache = true;
> +	}
> +
>   	rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, algo);


so the signature verification (the code execution) is done with dcache 
OFF....

flush the input data should be enought for DMA operation ?

=> call flush_dcache_all() or flush_dcache_range()

for example:

if (dcache_status())
	flush_dcache_all();


>   
> +	if (reenable_dcache)
> +		dcache_enable();
> +
>   	return rom_ret == ROM_API_SUCCESS ? 0 : -EPERM;
>   }
>   


Regards

Patrick

Re: [PATCH 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[Marek Vasut]

On 12/6/22 10:13, Patrick DELAUNAY wrote:
> Hi Marek,

Hi,

[...]

>> @@ -81,8 +82,21 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>>       memcpy(raw_key + 32, pubkey->y, 32);
>>       stm32mp_rom_get_ecdsa_functions(&rom);
>> +
>> +    /*
>> +     * Disable D-cache before calling into BootROM, else CRYP DMA
>> +     * may fail to pick up the correct data.
>> +     */
>> +    if (dcache_status()) {
>> +        dcache_disable();
>> +        reenable_dcache = true;
>> +    }
>> +
>>       rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, 
>> algo);
> 
> 
> so the signature verification (the code execution) is done with dcache 
> OFF....
> 
> flush the input data should be enought for DMA operation ?
> 
> => call flush_dcache_all() or flush_dcache_range()
> 
> for example:
> 
> if (dcache_status())
>      flush_dcache_all();

Wouldn't you then also need to invalidate the dcache after the BootROM 
call, so that the CPU with dcache enabled could read what the CRYP wrote 
to DRAM instead of pulling stale data from Dcache ?

That's very much what the enable/disable trick does for you.