[PATCH v3] spl: allow board_spl_fit_post_load() to fail

[PATCH v3] spl: allow board_spl_fit_post_load() to fail

[Patrick Wildt]

On i.MX platforms board_spl_fit_post_load() can check the loaded
SPL image for authenticity using its HAB engine.  U-Boot's SPL
mechanism allows booting images from other sources as well, but
in the current setup the SPL would just hang if it encounters an
image that does not pass scrutiny.  Allowing the function to return
an error, allows the SPL to try booting from another source as a
fallback instead of ending up as a brick.

Signed-off-by: Patrick Wildt <patrick@blueri.se>
---
Changes in v3:
 - use EINVAL as return value to have a proper errno

Changes in v2:
 - set SPL_FIT_FOUND only after successful post load

 arch/arm/mach-imx/spl.c |  6 ++++--
 common/spl/spl_fit.c    | 10 ++++++----
 include/spl.h           |  2 +-
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index 1a231c67f5a..1a0d979e2d0 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -313,7 +313,7 @@ ulong board_spl_fit_size_align(ulong size)
 	return size;
 }
 
-void board_spl_fit_post_load(ulong load_addr, size_t length)
+int board_spl_fit_post_load(ulong load_addr, size_t length)
 {
 	u32 offset = length - CONFIG_CSF_SIZE;
 
@@ -321,8 +321,10 @@ void board_spl_fit_post_load(ulong load_addr, size_t length)
 				       offset + IVT_SIZE + CSF_PAD_SIZE,
 				       offset)) {
 		puts("spl: ERROR:  image authentication unsuccessful\n");
-		hang();
+		return -EINVAL;
 	}
+
+	return 0;
 }
 #endif
 
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index f581a224213..ead4c6713af 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -26,8 +26,9 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
 #endif
 
-__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
+__weak int board_spl_fit_post_load(ulong load_addr, size_t length)
 {
+	return 0;
 }
 
 __weak ulong board_spl_fit_size_align(ulong size)
@@ -677,11 +678,12 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (spl_image->entry_point == FDT_ERROR || spl_image->entry_point == 0)
 		spl_image->entry_point = spl_image->load_addr;
 
-	spl_image->flags |= SPL_FIT_FOUND;
-
 #ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load((ulong)fit, size);
+	ret = board_spl_fit_post_load((ulong)fit, size);
+	if (ret)
+		return ret;
 #endif
 
+	spl_image->flags |= SPL_FIT_FOUND;
 	return 0;
 }
diff --git a/include/spl.h b/include/spl.h
index b31c9bb4ab2..2607767d940 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -564,7 +564,7 @@ int board_return_to_bootrom(struct spl_image_info *spl_image,
  * board_spl_fit_post_load - allow process images after loading finished
  *
  */
-void board_spl_fit_post_load(ulong load_addr, size_t length);
+int board_spl_fit_post_load(ulong load_addr, size_t length);
 
 /**
  * board_spl_fit_size_align - specific size align before processing payload
-- 
2.26.2

[PATCH v3] spl: allow board_spl_fit_post_load() to fail

[Marek Vasut]

On 5/31/20 7:25 PM, Patrick Wildt wrote:
> On i.MX platforms board_spl_fit_post_load() can check the loaded
> SPL image for authenticity using its HAB engine.  U-Boot's SPL
> mechanism allows booting images from other sources as well, but
> in the current setup the SPL would just hang if it encounters an
> image that does not pass scrutiny.  Allowing the function to return
> an error, allows the SPL to try booting from another source as a
> fallback instead of ending up as a brick.
> 
> Signed-off-by: Patrick Wildt <patrick@blueri.se>

Reviewed-by: Marek Vasut <marex@denx.de>

[PATCH v3] spl: allow board_spl_fit_post_load() to fail

[Peng Fan]

> Subject: [PATCH v3] spl: allow board_spl_fit_post_load() to fail
> 
> On i.MX platforms board_spl_fit_post_load() can check the loaded SPL image
> for authenticity using its HAB engine.  U-Boot's SPL mechanism allows
> booting images from other sources as well, but in the current setup the SPL
> would just hang if it encounters an image that does not pass scrutiny.

security.

> Allowing the function to return an error, allows the SPL to try booting from
> another source as a fallback instead of ending up as a brick.

This will break secure boot chain.

Regards,
Peng.

> 
> Signed-off-by: Patrick Wildt <patrick@blueri.se>
> ---
> Changes in v3:
>  - use EINVAL as return value to have a proper errno
> 
> Changes in v2:
>  - set SPL_FIT_FOUND only after successful post load
> 
>  arch/arm/mach-imx/spl.c |  6 ++++--
>  common/spl/spl_fit.c    | 10 ++++++----
>  include/spl.h           |  2 +-
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index
> 1a231c67f5a..1a0d979e2d0 100644
> --- a/arch/arm/mach-imx/spl.c
> +++ b/arch/arm/mach-imx/spl.c
> @@ -313,7 +313,7 @@ ulong board_spl_fit_size_align(ulong size)
>  	return size;
>  }
> 
> -void board_spl_fit_post_load(ulong load_addr, size_t length)
> +int board_spl_fit_post_load(ulong load_addr, size_t length)
>  {
>  	u32 offset = length - CONFIG_CSF_SIZE;
> 
> @@ -321,8 +321,10 @@ void board_spl_fit_post_load(ulong load_addr,
> size_t length)
>  				       offset + IVT_SIZE + CSF_PAD_SIZE,
>  				       offset)) {
>  		puts("spl: ERROR:  image authentication unsuccessful\n");
> -		hang();
> +		return -EINVAL;
>  	}
> +
> +	return 0;
>  }
>  #endif
> 
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index
> f581a224213..ead4c6713af 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -26,8 +26,9 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
>  #endif
> 
> -__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
> +__weak int board_spl_fit_post_load(ulong load_addr, size_t length)
>  {
> +	return 0;
>  }
> 
>  __weak ulong board_spl_fit_size_align(ulong size) @@ -677,11 +678,12 @@
> int spl_load_simple_fit(struct spl_image_info *spl_image,
>  	if (spl_image->entry_point == FDT_ERROR || spl_image->entry_point ==
> 0)
>  		spl_image->entry_point = spl_image->load_addr;
> 
> -	spl_image->flags |= SPL_FIT_FOUND;
> -
>  #ifdef CONFIG_IMX_HAB
> -	board_spl_fit_post_load((ulong)fit, size);
> +	ret = board_spl_fit_post_load((ulong)fit, size);
> +	if (ret)
> +		return ret;
>  #endif
> 
> +	spl_image->flags |= SPL_FIT_FOUND;
>  	return 0;
>  }
> diff --git a/include/spl.h b/include/spl.h index b31c9bb4ab2..2607767d940
> 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -564,7 +564,7 @@ int board_return_to_bootrom(struct spl_image_info
> *spl_image,
>   * board_spl_fit_post_load - allow process images after loading finished
>   *
>   */
> -void board_spl_fit_post_load(ulong load_addr, size_t length);
> +int board_spl_fit_post_load(ulong load_addr, size_t length);
> 
>  /**
>   * board_spl_fit_size_align - specific size align before processing payload
> --
> 2.26.2

[PATCH v3] spl: allow board_spl_fit_post_load() to fail

[Marek Vasut]

On 6/1/20 4:30 AM, Peng Fan wrote:
>> Subject: [PATCH v3] spl: allow board_spl_fit_post_load() to fail
>>
>> On i.MX platforms board_spl_fit_post_load() can check the loaded SPL image
>> for authenticity using its HAB engine.  U-Boot's SPL mechanism allows
>> booting images from other sources as well, but in the current setup the SPL
>> would just hang if it encounters an image that does not pass scrutiny.
> 
> security.
> 
>> Allowing the function to return an error, allows the SPL to try booting from
>> another source as a fallback instead of ending up as a brick.
> 
> This will break secure boot chain.

How? Please elaborate.

jump_to_image_no_args() will authenticate the image before starting it,
so I don't think so. However, that is still prone to
time-of-check/time-of-use attack anyway.

[PATCH v3] spl: allow board_spl_fit_post_load() to fail

[Tom Rini]

On Mon, Jun 01, 2020 at 12:08:45PM +0200, Marek Vasut wrote:
> On 6/1/20 4:30 AM, Peng Fan wrote:
> >> Subject: [PATCH v3] spl: allow board_spl_fit_post_load() to fail
> >>
> >> On i.MX platforms board_spl_fit_post_load() can check the loaded SPL image
> >> for authenticity using its HAB engine.  U-Boot's SPL mechanism allows
> >> booting images from other sources as well, but in the current setup the SPL
> >> would just hang if it encounters an image that does not pass scrutiny.
> > 
> > security.
> > 
> >> Allowing the function to return an error, allows the SPL to try booting from
> >> another source as a fallback instead of ending up as a brick.
> > 
> > This will break secure boot chain.
> 
> How? Please elaborate.
> 
> jump_to_image_no_args() will authenticate the image before starting it,
> so I don't think so. However, that is still prone to
> time-of-check/time-of-use attack anyway.

Yes, please elaborate, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200605/d007f6de/attachment.sig>

[PATCH v3] spl: allow board_spl_fit_post_load() to fail

[Patrick Wildt]

On Fri, Jun 05, 2020 at 03:54:14PM -0400, Tom Rini wrote:
> On Mon, Jun 01, 2020 at 12:08:45PM +0200, Marek Vasut wrote:
> > On 6/1/20 4:30 AM, Peng Fan wrote:
> > >> Subject: [PATCH v3] spl: allow board_spl_fit_post_load() to fail
> > >>
> > >> On i.MX platforms board_spl_fit_post_load() can check the loaded SPL image
> > >> for authenticity using its HAB engine.  U-Boot's SPL mechanism allows
> > >> booting images from other sources as well, but in the current setup the SPL
> > >> would just hang if it encounters an image that does not pass scrutiny.
> > > 
> > > security.
> > > 
> > >> Allowing the function to return an error, allows the SPL to try booting from
> > >> another source as a fallback instead of ending up as a brick.
> > > 
> > > This will break secure boot chain.
> > 
> > How? Please elaborate.
> > 
> > jump_to_image_no_args() will authenticate the image before starting it,
> > so I don't think so. However, that is still prone to
> > time-of-check/time-of-use attack anyway.
> 
> Yes, please elaborate, thanks!

Ping?  How will this break the secure boot chain?

[PATCH v3] spl: allow board_spl_fit_post_load() to fail

[Stefano Babic]

Hi Patrick,

On 22.07.20 23:20, Patrick Wildt wrote:
> On Fri, Jun 05, 2020 at 03:54:14PM -0400, Tom Rini wrote:
>> On Mon, Jun 01, 2020 at 12:08:45PM +0200, Marek Vasut wrote:
>>> On 6/1/20 4:30 AM, Peng Fan wrote:
>>>>> Subject: [PATCH v3] spl: allow board_spl_fit_post_load() to fail
>>>>>
>>>>> On i.MX platforms board_spl_fit_post_load() can check the loaded SPL image
>>>>> for authenticity using its HAB engine.  U-Boot's SPL mechanism allows
>>>>> booting images from other sources as well, but in the current setup the SPL
>>>>> would just hang if it encounters an image that does not pass scrutiny.
>>>>
>>>> security.
>>>>
>>>>> Allowing the function to return an error, allows the SPL to try booting from
>>>>> another source as a fallback instead of ending up as a brick.
>>>>
>>>> This will break secure boot chain.
>>>
>>> How? Please elaborate.
>>>
>>> jump_to_image_no_args() will authenticate the image before starting it,
>>> so I don't think so. However, that is still prone to
>>> time-of-check/time-of-use attack anyway.
>>
>> Yes, please elaborate, thanks!
> 
> Ping?  How will this break the secure boot chain?

To be honest: I had merged this one (after the discussion with Marek and 
his patch calling panic()), but I worried if there is a hidden reason to 
break secure boot. I do not know the reason, I am curious, too, which is 
the reason because I will see this patch in (this helps to provide a 
safe update of bootloader).

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================