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

[Ye Li <ye.li@nxp.com>]

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