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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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.

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.

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 |