Re: [PATCH 1/2] ARM: i.MX: tqma6ulx: fix barebox chainloading with OP-TEE enabled

[Marco Felsch <m.felsch@pengutronix.de>]

On 25-06-26, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 6/26/25 16:03, Sascha Hauer wrote:
> > Chainloading barebox when OP-TEE is enabled has multiple bugs, fix them.
> > 
> > When barebox starts we have to guess if we have to start OP-TEE or not.
> > As we can't detect the exception level on ARMv7 we do this by checking
> > if a first stage loader has passed us a device tree.
> > 
> > First of all the device tree is passed in r2, not in r0, so fix the
> > register we use.
> > 
> > Then we have to check if r2 is within the SDRAM. We check against
> > MX6_MMDC_P0_BASE_ADDR which is the base of the SDRAM controller. Use the
> > base address of the SDRAM instead.
> > 
> > Finally we manipulate the TZASC which we are obviously not allowed in
> > EL1, so move the manipulation to the code which is only executed in EL2.
> 
> EL1/EL2 are ARMv8-specific.
> 
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  arch/arm/boards/tqma6ulx/lowlevel.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm/boards/tqma6ulx/lowlevel.c b/arch/arm/boards/tqma6ulx/lowlevel.c
> > index 5fd997d2ec..ce330c37af 100644
> > --- a/arch/arm/boards/tqma6ulx/lowlevel.c
> > +++ b/arch/arm/boards/tqma6ulx/lowlevel.c
> > @@ -66,7 +66,7 @@ static void *read_eeprom(void)
> >  	return fdt;
> >  }
> >  
> > -static void noinline start_mba6ulx(u32 r0)
> > +static void noinline start_mba6ulx(u32 r2)
> >  {
> >  	void *fdt;
> >  	int tee_size;
> > @@ -76,16 +76,15 @@ static void noinline start_mba6ulx(u32 r0)
> >  
> >  	fdt = read_eeprom();
> >  
> > -	/* Enable normal/secure r/w for TZC380 region0 */
> > -	writel(0xf0000000, 0x021D0108);
> > -
> >  	/*
> >  	 * Chainloading barebox will pass a device tree within the RAM in r0,
> >  	 * skip OP-TEE early loading in this case
> >  	 */
> >  	if (IS_ENABLED(CONFIG_FIRMWARE_TQMA6UL_OPTEE) &&
> > -	    !(r0 > MX6_MMDC_P0_BASE_ADDR &&
> > -	      r0 < MX6_MMDC_P0_BASE_ADDR + SZ_256M)) {
> > +	    !(r2 > MX6_MMDC_PORT0_BASE_ADDR && r2 < MX6_MMDC_PORT0_BASE_ADDR + SZ_256M)) {
> > +		/* Enable normal/secure r/w for TZC380 region0 */
> > +		writel(0xf0000000, 0x021D0108);
> 
> I think this is problematic:
> 
>   - robustness-wise: We have no guarantee that there isn't some lesser
>     used BootROM code path that happens to leave a suitable DRAM
>     look-alike address that would trick us here.
> 
>   - security wise, even if we check for FDT header if r2 points into
>     DRAM, a compromised OS could spray RAM with FDT magic,
>     trigger a warm reset that has the BootROM produce a DRAM lookalike
>     pointer in r2 and then OP-TEE loading is skipped and the kernel
>     starts in the highest privilege level.

These are good points and should be addressed, but I think the patch is
still good because obvious broken code gets repaired.

> To address this, we need some way to set a sticky bit that's cleared
> only on reset. One way, would be to set up an IVT and try to access the
> L2 cache controller while data_abort_mask() is active, like
> imx6_cannot_write_l2x0 is doing.

Nice trick!

One could also argue that skip the OP-TEE loading (to chainload barebox)
is a dev-feature which should be disabled once the INSECURE=n.

Regards,
  Marco

> Cheers,
> Ahmad
> 
> > +
> >  		get_builtin_firmware(mba6ul_optee_bin, &tee, &tee_size);
> >  
> >  		memset((void *)OPTEE_OVERLAY_LOCATION, 0, 0x1000);
> > @@ -112,5 +111,5 @@ ENTRY_FUNCTION(start_imx6ul_mba6ulx, r0, r1, r2)
> >  	setup_c();
> >  	barrier();
> >  
> > -	start_mba6ulx(r0);
> > +	start_mba6ulx(r2);
> >  }
> 
> -- 
> Pengutronix e.K.                  |                             |
> Steuerwalder Str. 21              | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |
> 
> 
>