[PATCH v1 0/2] imx8m: fix secure boot
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(+)
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
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
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
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; +}
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
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...
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(+)
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...
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/...
https://source.codeaurora.org/external/imx/uboot-imx/tree/common/spl/spl_fit...
and
https://source.codeaurora.org/external/imx/uboot-imx/tree/common/spl/spl_fit...
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/...
?
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(+)
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
participants (5)
-
Heiko Schocher -
Peng Fan (OSS) -
sbabic＠denx.de -
Simon Glass -
Ye Li