[PATCH v1 0/2] imx8m: fix secure boot

[PATCH v1 0/2] imx8m: fix secure boot

[Heiko Schocher]

This series fixes secure boot on imx8m based boards. Tim
also detected this issue and the patches fixed on his hardware
also the problem, see discussion here:

https://lists.denx.de/pipermail/u-boot/2021-July/454351.html

Problem is that the IVT header gets loaded to a memallocated
buffer, but it needs to sit on memaddress coded in IVT header
itself. This patchseries adds a weak function spl_load_simple_fit()
in common spl code, which does not change current code behaviour.

Second patch than implements this weak function for imx based
boards (if no IVT header is found on address which is passed
to it, it does nothing).

I am not sure if this is the best solution, but it fixes a real
bug, and may could be made clearer, if possible.


Heiko Schocher (2):
  spl_fit. add hook to make fixes after fit header is loaded
  imx: spl: implement spl_load_simple_fit_fix_load

 arch/arm/mach-imx/spl.c | 33 +++++++++++++++++++++++++++++++++
 common/spl/spl_fit.c    | 11 +++++++++++
 include/spl.h           |  8 ++++++++
 3 files changed, 52 insertions(+)

-- 
2.31.1

[PATCH v1 1/2] spl_fit. add hook to make fixes after fit header is loaded

[Heiko Schocher]

add hook function spl_load_simple_fit_fix_load()
which is called after fit image header is loaded.

Signed-off-by: Heiko Schocher <hs@denx.de>
---

 common/spl/spl_fit.c | 11 +++++++++++
 include/spl.h        |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index f41abca0cc..633fac2e6b 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -548,6 +548,15 @@ __weak bool spl_load_simple_fit_skip_processing(void)
 	return false;
 }
 
+/*
+ * Weak default function to allow fixes after fit header
+ * is loaded.
+ */
+__weak void *spl_load_simple_fit_fix_load(void *fit)
+{
+	return fit;
+}
+
 static void warn_deprecated(const char *msg)
 {
 	printf("DEPRECATED: %s\n", msg);
@@ -685,6 +694,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (spl_load_simple_fit_skip_processing())
 		return 0;
 
+	ctx.fit = spl_load_simple_fit_fix_load(ctx.fit);
+
 	ret = spl_simple_fit_parse(&ctx);
 	if (ret < 0)
 		return ret;
diff --git a/include/spl.h b/include/spl.h
index afbf39bef4..fd1d47cd05 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -304,6 +304,14 @@ ulong spl_get_image_text_base(void);
  */
 bool spl_load_simple_fit_skip_processing(void);
 
+/**
+ * spl_load_simple_fit_fix_load() - Hook to make fixes
+ * after fit image header is loaded
+ *
+ * Returns pointer to fit
+ */
+void *spl_load_simple_fit_fix_load(void *fit);
+
 /**
  * spl_load_simple_fit() - Loads a fit image from a device.
  * @spl_image:	Image description to set up
-- 
2.31.1

Re: [PATCH v1 1/2] spl_fit. add hook to make fixes after fit header is loaded

[Simon Glass]

Hi Heiko,

On Thu, 5 Aug 2021 at 22:45, Heiko Schocher <hs@denx.de> wrote:
>
> add hook function spl_load_simple_fit_fix_load()
> which is called after fit image header is loaded.

Please add motivation to the commit message.

>
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
>
>  common/spl/spl_fit.c | 11 +++++++++++
>  include/spl.h        |  8 ++++++++
>  2 files changed, 19 insertions(+)
>
Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v1 1/2] spl_fit. add hook to make fixes after fit header is loaded

[sbabic]

> add hook function spl_load_simple_fit_fix_load()
> which is called after fit image header is loaded.
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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@denx.de
=====================================================================

[PATCH v1 2/2] imx: spl: implement spl_load_simple_fit_fix_load

[Heiko Schocher]

read the address where the IVT header must sit
from IVT image header, loaded from SPL into
an malloced buffer and copy the IVT header
to this address

May make this dependend on SoC ?

Signed-off-by: Heiko Schocher <hs@denx.de>
---

 arch/arm/mach-imx/spl.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index 36033d611c..cc29f337d7 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -345,3 +345,36 @@ int dram_init_banksize(void)
 	return 0;
 }
 #endif
+
+/*
+ * read the address where the IVT header must sit
+ * from IVT image header, loaded from SPL into
+ * an malloced buffer and copy the IVT header
+ * to this address
+ */
+void *spl_load_simple_fit_fix_load(void *fit)
+{
+	struct ivt *ivt;
+	unsigned long new;
+	unsigned long offset;
+	unsigned long size;
+	u8 *tmp = (u8 *)fit;
+
+	offset = ALIGN(fdt_totalsize(fit), 0x1000);
+	size = ALIGN(fdt_totalsize(fit), 4);
+	size = board_spl_fit_size_align(size);
+	tmp += offset;
+	ivt = (struct ivt *)tmp;
+	if (ivt->hdr.magic != IVT_HEADER_MAGIC) {
+		debug("no IVT header found\n");
+		return fit;
+	}
+	debug("%s: ivt: %p offset: %lx size: %lx\n", __func__, ivt, offset, size);
+	debug("%s: ivt self: %x\n", __func__, ivt->self);
+	new = ivt->self;
+	new -= offset;
+	debug("%s: new %lx\n", __func__, new);
+	memcpy((void *)new, fit, size);
+
+	return (void *)new;
+}
-- 
2.31.1

[PATCH v1 2/2] imx: spl: implement spl_load_simple_fit_fix_load

[sbabic]

> read the address where the IVT header must sit
> from IVT image header, loaded from SPL into
> an malloced buffer and copy the IVT header
> to this address
> May make this dependend on SoC ?
> Signed-off-by: Heiko Schocher <hs@denx.de>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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@denx.de
=====================================================================

Re: [PATCH v1 0/2] imx8m: fix secure boot

[Peng Fan (OSS)]

On 2021/8/6 12:44, Heiko Schocher wrote:
> 
> This series fixes secure boot on imx8m based boards. Tim
> also detected this issue and the patches fixed on his hardware
> also the problem, see discussion here:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.denx.de%2Fpipermail%2Fu-boot%2F2021-July%2F454351.html&data=04%7C01%7Cpeng.fan%40nxp.com%7C60053de52160416b6d9608d95894ead0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637638218882598843%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ZF%2FCsb%2FdBiLKHa5o6wRmAsQOIduZHhFI5MC%2BuIB0Vz4%3D&reserved=0
> 
> Problem is that the IVT header gets loaded to a memallocated
> buffer, but it needs to sit on memaddress coded in IVT header
> itself. This patchseries adds a weak function spl_load_simple_fit()
> in common spl code, which does not change current code behaviour.
> 
> Second patch than implements this weak function for imx based
> boards (if no IVT header is found on address which is passed
> to it, it does nothing).
> 
> I am not sure if this is the best solution, but it fixes a real
> bug, and may could be made clearer, if possible.

NXP downstream dropped malloc, with
buf = board_spl_fit_buffer_addr(size, sectors, info->bl_len);

And this will use previous fixed address.

Regards,
Peng.

> 
> 
> Heiko Schocher (2):
>    spl_fit. add hook to make fixes after fit header is loaded
>    imx: spl: implement spl_load_simple_fit_fix_load
> 
>   arch/arm/mach-imx/spl.c | 33 +++++++++++++++++++++++++++++++++
>   common/spl/spl_fit.c    | 11 +++++++++++
>   include/spl.h           |  8 ++++++++
>   3 files changed, 52 insertions(+)
> 

Re: [PATCH v1 0/2] imx8m: fix secure boot

[Heiko Schocher]

Hello Peng,

On 06.08.21 07:56, Peng Fan (OSS) wrote:
> 
> 
> On 2021/8/6 12:44, Heiko Schocher wrote:
>>
>> This series fixes secure boot on imx8m based boards. Tim
>> also detected this issue and the patches fixed on his hardware
>> also the problem, see discussion here:
>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.denx.de%2Fpipermail%2Fu-boot%2F2021-July%2F454351.html&data=04%7C01%7Cpeng.fan%40nxp.com%7C60053de52160416b6d9608d95894ead0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637638218882598843%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ZF%2FCsb%2FdBiLKHa5o6wRmAsQOIduZHhFI5MC%2BuIB0Vz4%3D&reserved=0
>>
>>
>> Problem is that the IVT header gets loaded to a memallocated
>> buffer, but it needs to sit on memaddress coded in IVT header
>> itself. This patchseries adds a weak function spl_load_simple_fit()
>> in common spl code, which does not change current code behaviour.
>>
>> Second patch than implements this weak function for imx based
>> boards (if no IVT header is found on address which is passed
>> to it, it does nothing).
>>
>> I am not sure if this is the best solution, but it fixes a real
>> bug, and may could be made clearer, if possible.
> 
> NXP downstream dropped malloc, with
> buf = board_spl_fit_buffer_addr(size, sectors, info->bl_len);
> 
> And this will use previous fixed address.

Ah, okay, you mean:

https://source.codeaurora.org/external/imx/uboot-imx/tree/arch/arm/mach-imx/spl.c?h=lf_v2021.04#n334

https://source.codeaurora.org/external/imx/uboot-imx/tree/common/spl/spl_fit.c?h=lf_v2021.04#n541

and

https://source.codeaurora.org/external/imx/uboot-imx/tree/common/spl/spl_fit.c?h=lf_v2021.04#n581

correct?

But I do not see, where ivt->self is used... or is per definiton
ivt->self equal to:
https://source.codeaurora.org/external/imx/uboot-imx/tree/arch/arm/mach-imx/spl.c?h=lf_v2021.04#n345

?

bye,
Heiko

> 
> Regards,
> Peng.
> 
>>
>>
>> Heiko Schocher (2):
>>    spl_fit. add hook to make fixes after fit header is loaded
>>    imx: spl: implement spl_load_simple_fit_fix_load
>>
>>   arch/arm/mach-imx/spl.c | 33 +++++++++++++++++++++++++++++++++
>>   common/spl/spl_fit.c    | 11 +++++++++++
>>   include/spl.h           |  8 ++++++++
>>   3 files changed, 52 insertions(+)
>>

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

Re: [EXT] Re: [PATCH v1 0/2] imx8m: fix secure boot

[Ye Li]

On Fri, 2021-08-06 at 08:39 +0200, Heiko Schocher wrote:
> Caution: EXT Email
> 
> Hello Peng,
> 
> On 06.08.21 07:56, Peng Fan (OSS) wrote:
> > 
> > 
> > 
> > On 2021/8/6 12:44, Heiko Schocher wrote:
> > > 
> > > 
> > > This series fixes secure boot on imx8m based boards. Tim
> > > also detected this issue and the patches fixed on his hardware
> > > also the problem, see discussion here:
> > > 
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2
> > > Flists.denx.de%2Fpipermail%2Fu-boot%2F2021-
> > > July%2F454351.html&data=04%7C01%7Cye.li%40nxp.com%7C4e50cef1a
> > > 559457dc78c08d958a4f5d9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> > > 0%7C637638287788477666%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> > > AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata
> > > =Rcmml9Sg0hSc%2FE68Dzjcn0wce1xYSpQNfJ0wfT4Jork%3D&reserved=0
> > > 
> > > 
> > > Problem is that the IVT header gets loaded to a memallocated
> > > buffer, but it needs to sit on memaddress coded in IVT header
> > > itself. This patchseries adds a weak function
> > > spl_load_simple_fit()
> > > in common spl code, which does not change current code behaviour.
> > > 
> > > Second patch than implements this weak function for imx based
> > > boards (if no IVT header is found on address which is passed
> > > to it, it does nothing).
> > > 
> > > I am not sure if this is the best solution, but it fixes a real
> > > bug, and may could be made clearer, if possible.
> > NXP downstream dropped malloc, with
> > buf = board_spl_fit_buffer_addr(size, sectors, info->bl_len);
> > 
> > And this will use previous fixed address.
> Ah, okay, you mean:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsou
> rce.codeaurora.org%2Fexternal%2Fimx%2Fuboot-
> imx%2Ftree%2Farch%2Farm%2Fmach-
> imx%2Fspl.c%3Fh%3Dlf_v2021.04%23n334&data=04%7C01%7Cye.li%40nxp.c
> om%7C4e50cef1a559457dc78c08d958a4f5d9%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C637638287788487624%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sda
> ta=prhVBpPvqD1CDGWi7tWcN5%2BzChBeSQzeIK%2FvhedGcfE%3D&reserved=0
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsou
> rce.codeaurora.org%2Fexternal%2Fimx%2Fuboot-
> imx%2Ftree%2Fcommon%2Fspl%2Fspl_fit.c%3Fh%3Dlf_v2021.04%23n541&da
> ta=04%7C01%7Cye.li%40nxp.com%7C4e50cef1a559457dc78c08d958a4f5d9%7C686
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637638287788487624%7CUnknown%
> 7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ
> XVCI6Mn0%3D%7C1000&sdata=J%2FH%2FBBtiMMl9G744CjjPESEUVCxmO%2Bg7%2
> BHVJsM1yKc4%3D&reserved=0
> 
> and
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsou
> rce.codeaurora.org%2Fexternal%2Fimx%2Fuboot-
> imx%2Ftree%2Fcommon%2Fspl%2Fspl_fit.c%3Fh%3Dlf_v2021.04%23n581&da
> ta=04%7C01%7Cye.li%40nxp.com%7C4e50cef1a559457dc78c08d958a4f5d9%7C686
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637638287788487624%7CUnknown%
> 7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ
> XVCI6Mn0%3D%7C1000&sdata=WVZ6Az8kazKu%2BWzysM7%2B3u5XHOb6gtggwiCK
> rewnI2o%3D&reserved=0
> 
> correct?

Yes. correct.

> 
> But I do not see, where ivt->self is used... or is per definiton
> ivt->self equal to:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsou
> rce.codeaurora.org%2Fexternal%2Fimx%2Fuboot-
> imx%2Ftree%2Farch%2Farm%2Fmach-
> imx%2Fspl.c%3Fh%3Dlf_v2021.04%23n345&data=04%7C01%7Cye.li%40nxp.c
> om%7C4e50cef1a559457dc78c08d958a4f5d9%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C637638287788487624%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sda
> ta=Fo5efFghnqsyUvtuykwVm68NvDnk%2Fb1hCoiuQW1JkiA%3D&reserved=0
> 
> ?
> 

The fit buffer was used in SPL is a fit size related offset to u-boot
base. In mkimage, we generate IVT following the same calculation. So we
don't use ivt->self, this address is aligned between SPL and IVT. 

Your patch depends on IVT. But actually IVT is not necessary for non-
secure boot. The board_spl_fit_size_align in mach-imx/spl.c is only
defined for HAB enabled. So for non-secure boot, it does not include
size for IVT. This will be an issue.

Best regards,
Ye Li

> bye,
> Heiko
> 
> > 
> > 
> > Regards,
> > Peng.
> > 
> > > 
> > > 
> > > 
> > > Heiko Schocher (2):
> > >    spl_fit. add hook to make fixes after fit header is loaded
> > >    imx: spl: implement spl_load_simple_fit_fix_load
> > > 
> > >   arch/arm/mach-imx/spl.c | 33 +++++++++++++++++++++++++++++++++
> > >   common/spl/spl_fit.c    | 11 +++++++++++
> > >   include/spl.h           |  8 ++++++++
> > >   3 files changed, 52 insertions(+)
> > > 
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de