[PATCH 0/4] Repeal and replace TZDRAM_ related config options

[PATCH 0/4] Repeal and replace TZDRAM_ related config options

[Alexandru Gagniuc]

When enabling OPTEE support in the config menu, one is asked for the
TZDRAM region and OPTEE load address. It would seem that these are
very important values, not just some half-assed bollocks.

There are currently three sources of information regarding OPTEE and
its associated DRAM region:
    1) Devicetree "/reserved-memory" nodes
    2) uImage and FIT load-address and entry-point
    3) The CONFIG_s currently under scrutiny

(1) and (2) are sufficient to identify and reject OP-TEE images which
fall outside TZDRAM. Such a check might make sense in the context of
not bricking a device. But if an incompatible image was already
installed, the device is likely soft-bricked anyway and needs manual
intervention. This series doesn't implement such a check.

It is possible for (1) and (3) to be in conflict. Because the values
in (3) default to 0x0, it is very likely to happen. Such a situation
causes the "bootm" command to reject otherwise valid OP-TEE images.
Thus a perfectly tuned u-boot which doesn't also set (3) will be
non-functional with respect to OP-TEE images.

This becomes confusing, and has caused yours truly a disproportionate
amount of grief.
Fortunately, SPL can also load OP-TEE images, and does not even look
at the values in (3). It only uses (1) and (2) to determine the
appropriate values for firing up OP-TEE. It is more reliable and easy
to use than "bootm".

We are able to remove these configs without breaking anything. This
also resolves the problems with "bootm" mentioned earlier.

I hypothesize that one could re-add the removed checks from
optee_verify_image() by deriving TZDRAM information from (1) instead
of (3). I chose not to implement it because I don't see the value.



Alexandru Gagniuc (4):
  lib: optee: Avoid CONFIG_TZDRAM_* in optee_verify_bootm_image()
  lib: optee: Remove CONFIG_OPTEE_TZDRAM_BASE
  lib: optee: Remove CONFIG_OPTEE_LOAD_ADDR
  arm: imx: mx7: Move CONFIG_OPTEE_TZDRAM_SIZE from lib/optee

 arch/arm/mach-imx/mx7/Kconfig |  8 ++++++++
 configs/warp7_bl33_defconfig  |  1 -
 configs/warp7_defconfig       |  2 --
 include/configs/warp7.h       |  5 -----
 include/tee/optee.h           | 14 --------------
 lib/optee/Kconfig             | 23 -----------------------
 lib/optee/optee.c             | 21 ++++++---------------
 7 files changed, 14 insertions(+), 60 deletions(-)

-- 
2.31.1

[PATCH 1/4] lib: optee: Avoid CONFIG_TZDRAM_* in optee_verify_bootm_image()

[Alexandru Gagniuc]

The configs TZDRAM_BASE and TZDRAM_SIZE are expected to describe the
memory allocated to the OPTEE region. according to according to commit
c5a6e8bd00cc ("optee: Add optee_verify_bootm_image()"). The TZDRAM is
with some limitations, described by "/reserved-memory" nodes in the
devicetree.

Consequently TZDRAM_BASE and TZDRAM_SIZE can point to imaginary
regions which have nothing to do with actual DRAM. They are not used
to configure the hardware or set up the Trust Zone Controller (TZC)
for OP-TEE -- the devicetree values are used instead.

When a valid OP-TEE image does not fall within the region described by
these configs, u-boot will refuse to load it. In fact, it mostly
serves to cause "bootm" to reject perfectly good OP-TEE images.

Ironically, someone has to correctly configure the devicetree for
TZDRAM, then go back and enter the same information in Kconfig for
"bootm". To remedy this, do not use TZDRAM_BASE and TZDRAM_SIZE in the
verification of OPTEE images.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 include/tee/optee.h | 14 --------------
 lib/optee/optee.c   | 21 ++++++---------------
 2 files changed, 6 insertions(+), 29 deletions(-)

diff --git a/include/tee/optee.h b/include/tee/optee.h
index ebdfe5e98d..764a55b264 100644
--- a/include/tee/optee.h
+++ b/include/tee/optee.h
@@ -43,20 +43,6 @@ optee_image_get_load_addr(const struct image_header *hdr)
 	return optee_image_get_entry_point(hdr) - sizeof(struct optee_header);
 }
 
-#if defined(CONFIG_OPTEE)
-int optee_verify_image(struct optee_header *hdr, unsigned long tzdram_start,
-		       unsigned long tzdram_len, unsigned long image_len);
-#else
-static inline int optee_verify_image(struct optee_header *hdr,
-				     unsigned long tzdram_start,
-				     unsigned long tzdram_len,
-				     unsigned long image_len)
-{
-	return -EPERM;
-}
-
-#endif
-
 #if defined(CONFIG_OPTEE)
 int optee_verify_bootm_image(unsigned long image_addr,
 			     unsigned long image_load_addr,
diff --git a/lib/optee/optee.c b/lib/optee/optee.c
index 672690dc53..67e46d71d6 100644
--- a/lib/optee/optee.c
+++ b/lib/optee/optee.c
@@ -16,14 +16,12 @@
 
 #define optee_hdr_err_msg \
 	"OPTEE verification error:" \
-	"\n\thdr=%p image=0x%08lx magic=0x%08x tzdram 0x%08lx-0x%08lx " \
+	"\n\thdr=%p image=0x%08lx magic=0x%08x" \
 	"\n\theader lo=0x%08x hi=0x%08x size=0x%08lx arch=0x%08x" \
 	"\n\tuimage params 0x%08lx-0x%08lx\n"
 
-int optee_verify_image(struct optee_header *hdr, unsigned long tzdram_start,
-		       unsigned long tzdram_len, unsigned long image_len)
+static int optee_verify_image(struct optee_header *hdr, unsigned long image_len)
 {
-	unsigned long tzdram_end = tzdram_start + tzdram_len;
 	uint32_t tee_file_size;
 
 	tee_file_size = hdr->init_size + hdr->paged_size +
@@ -31,11 +29,7 @@ int optee_verify_image(struct optee_header *hdr, unsigned long tzdram_start,
 
 	if (hdr->magic != OPTEE_MAGIC ||
 	    hdr->version != OPTEE_VERSION ||
-	    hdr->init_load_addr_hi > tzdram_end ||
-	    hdr->init_load_addr_lo < tzdram_start ||
-	    tee_file_size > tzdram_len ||
-	    tee_file_size != image_len ||
-	    (hdr->init_load_addr_lo + tee_file_size) > tzdram_end) {
+	    tee_file_size != image_len) {
 		return -EINVAL;
 	}
 
@@ -47,12 +41,9 @@ int optee_verify_bootm_image(unsigned long image_addr,
 			     unsigned long image_len)
 {
 	struct optee_header *hdr = (struct optee_header *)image_addr;
-	unsigned long tzdram_start = CONFIG_OPTEE_TZDRAM_BASE;
-	unsigned long tzdram_len = CONFIG_OPTEE_TZDRAM_SIZE;
-
 	int ret;
 
-	ret = optee_verify_image(hdr, tzdram_start, tzdram_len, image_len);
+	ret = optee_verify_image(hdr, image_len);
 	if (ret)
 		goto error;
 
@@ -63,8 +54,8 @@ int optee_verify_bootm_image(unsigned long image_addr,
 
 	return ret;
 error:
-	printf(optee_hdr_err_msg, hdr, image_addr, hdr->magic, tzdram_start,
-	       tzdram_start + tzdram_len, hdr->init_load_addr_lo,
+	printf(optee_hdr_err_msg, hdr, image_addr, hdr->magic,
+	       hdr->init_load_addr_lo,
 	       hdr->init_load_addr_hi, image_len, hdr->arch, image_load_addr,
 	       image_load_addr + image_len);
 
-- 
2.31.1

Re: [PATCH 1/4] lib: optee: Avoid CONFIG_TZDRAM_* in optee_verify_bootm_image()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

On Tue, Sep 07, 2021 at 12:07:06PM -0500, Alexandru Gagniuc wrote:

> The configs TZDRAM_BASE and TZDRAM_SIZE are expected to describe the
> memory allocated to the OPTEE region. according to according to commit
> c5a6e8bd00cc ("optee: Add optee_verify_bootm_image()"). The TZDRAM is
> with some limitations, described by "/reserved-memory" nodes in the
> devicetree.
> 
> Consequently TZDRAM_BASE and TZDRAM_SIZE can point to imaginary
> regions which have nothing to do with actual DRAM. They are not used
> to configure the hardware or set up the Trust Zone Controller (TZC)
> for OP-TEE -- the devicetree values are used instead.
> 
> When a valid OP-TEE image does not fall within the region described by
> these configs, u-boot will refuse to load it. In fact, it mostly
> serves to cause "bootm" to reject perfectly good OP-TEE images.
> 
> Ironically, someone has to correctly configure the devicetree for
> TZDRAM, then go back and enter the same information in Kconfig for
> "bootm". To remedy this, do not use TZDRAM_BASE and TZDRAM_SIZE in the
> verification of OPTEE images.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 2/4] lib: optee: Remove CONFIG_OPTEE_TZDRAM_BASE

[Alexandru Gagniuc]

It is no longer used in u-boot. Information about the TZDRAM location
is usually available in the devicetree as "/reserved-memory/" nodes.
Because this isn't used, remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 configs/warp7_bl33_defconfig | 1 -
 configs/warp7_defconfig      | 1 -
 lib/optee/Kconfig            | 8 --------
 3 files changed, 10 deletions(-)

diff --git a/configs/warp7_bl33_defconfig b/configs/warp7_bl33_defconfig
index ec078178ab..f9c0add926 100644
--- a/configs/warp7_bl33_defconfig
+++ b/configs/warp7_bl33_defconfig
@@ -67,4 +67,3 @@ CONFIG_USB_ETH_CDC=y
 CONFIG_USBNET_HOST_ADDR="de:ad:be:af:00:00"
 CONFIG_OF_LIBFDT_OVERLAY=y
 CONFIG_OPTEE_TZDRAM_SIZE=0x02000000
-CONFIG_OPTEE_TZDRAM_BASE=0x9e000000
diff --git a/configs/warp7_defconfig b/configs/warp7_defconfig
index 19c0c183f7..1514019064 100644
--- a/configs/warp7_defconfig
+++ b/configs/warp7_defconfig
@@ -73,5 +73,4 @@ CONFIG_USB_ETH_CDC=y
 CONFIG_USBNET_HOST_ADDR="de:ad:be:af:00:00"
 CONFIG_OPTEE_LOAD_ADDR=0x84000000
 CONFIG_OPTEE_TZDRAM_SIZE=0x3000000
-CONFIG_OPTEE_TZDRAM_BASE=0x9d000000
 CONFIG_BOOTM_OPTEE=y
diff --git a/lib/optee/Kconfig b/lib/optee/Kconfig
index c398f9b953..831e9f4ac6 100644
--- a/lib/optee/Kconfig
+++ b/lib/optee/Kconfig
@@ -22,14 +22,6 @@ config OPTEE_TZDRAM_SIZE
 	  The size of pre-allocated Trust Zone DRAM to allocate for the OPTEE
 	  runtime.
 
-config OPTEE_TZDRAM_BASE
-	hex "Base address of Trust-Zone RAM for the OPTEE image"
-	default 0x00000000
-	depends on OPTEE
-	help
-	  The base address of pre-allocated Trust Zone DRAM for
-	  the OPTEE runtime.
-
 config BOOTM_OPTEE
 	bool "Support OPTEE bootm command"
 	select BOOTM_LINUX
-- 
2.31.1

Re: [PATCH 2/4] lib: optee: Remove CONFIG_OPTEE_TZDRAM_BASE

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

On Tue, Sep 07, 2021 at 12:07:07PM -0500, Alexandru Gagniuc wrote:

> It is no longer used in u-boot. Information about the TZDRAM location
> is usually available in the devicetree as "/reserved-memory/" nodes.
> Because this isn't used, remove it.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 3/4] lib: optee: Remove CONFIG_OPTEE_LOAD_ADDR

[Alexandru Gagniuc]

This value is not used by u-boot, and it should not. The load address
of an OPTEE image is defined by said image. Either a uImage or a FIT
will have a defined load address and entry point. Those values are the
correct ones, not CONFIG_OPTEE_LOAD_ADDR.

Commit f25006b96e9f ("optee: Add CONFIG_OPTEE_LOAD_ADDR") justifies
this config by requiring its presence in u-boot's .config for other
images as part of a larger build, claiming it is "the best way".

This argument is not persuasive. U-boot's configuration is driven by
platform requirements, not the other way around. It seems more likely
that the argument is conflating tooling issues with Kconfig. Yocto and
buildroot have excellent mechanisms for defining values across the
board (pun intended). u-boot's Kconfig is the wrong place to do it.

Furthermore, it is not "best" for u-boot because it hardcodes a value
which is then not used. In fact the load address that u-boot uses is
the one derived from the OPTEE image.

Confused yet? I sure was. To prevent future confusion, remove
CONFIG_OPTEE_LOAD_ADDR.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 configs/warp7_defconfig | 1 -
 include/configs/warp7.h | 5 -----
 lib/optee/Kconfig       | 7 -------
 3 files changed, 13 deletions(-)

diff --git a/configs/warp7_defconfig b/configs/warp7_defconfig
index 1514019064..b872548464 100644
--- a/configs/warp7_defconfig
+++ b/configs/warp7_defconfig
@@ -71,6 +71,5 @@ CONFIG_USB_GADGET_DOWNLOAD=y
 CONFIG_USB_ETHER=y
 CONFIG_USB_ETH_CDC=y
 CONFIG_USBNET_HOST_ADDR="de:ad:be:af:00:00"
-CONFIG_OPTEE_LOAD_ADDR=0x84000000
 CONFIG_OPTEE_TZDRAM_SIZE=0x3000000
 CONFIG_BOOTM_OPTEE=y
diff --git a/include/configs/warp7.h b/include/configs/warp7.h
index a5d52e3977..0df7e3b146 100644
--- a/include/configs/warp7.h
+++ b/include/configs/warp7.h
@@ -34,10 +34,6 @@
 #define BOOT_SCR_STRING "source ${bootscriptaddr}\0"
 #endif
 
-#ifndef CONFIG_OPTEE_LOAD_ADDR
-#define CONFIG_OPTEE_LOAD_ADDR 0
-#endif
-
 #define CONFIG_EXTRA_ENV_SETTINGS \
 	CONFIG_DFU_ENV_SETTINGS \
 	"script=boot.scr\0" \
@@ -52,7 +48,6 @@
 	"fdt_file=imx7s-warp.dtb\0" \
 	"fdt_addr=" __stringify(CONFIG_SYS_FDT_ADDR)"\0" \
 	"fdtovaddr=0x83100000\0" \
-	"optee_addr=" __stringify(CONFIG_OPTEE_LOAD_ADDR)"\0" \
 	"boot_fdt=try\0" \
 	"ip_dyn=yes\0" \
 	"mmcdev="__stringify(CONFIG_SYS_MMC_ENV_DEV)"\0" \
diff --git a/lib/optee/Kconfig b/lib/optee/Kconfig
index 831e9f4ac6..26677b7548 100644
--- a/lib/optee/Kconfig
+++ b/lib/optee/Kconfig
@@ -7,13 +7,6 @@ config OPTEE
           OPTEE specific checks before booting an OPTEE image created with
           mkimage.
 
-config OPTEE_LOAD_ADDR
-	hex "OPTEE load address"
-	default 0x00000000
-	depends on OPTEE
-	help
-	  The load address of the bootable OPTEE binary.
-
 config OPTEE_TZDRAM_SIZE
 	hex "Amount of Trust-Zone RAM for the OPTEE image"
 	default 0x0000000
-- 
2.31.1

Re: [PATCH 3/4] lib: optee: Remove CONFIG_OPTEE_LOAD_ADDR

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On Tue, Sep 07, 2021 at 12:07:08PM -0500, Alexandru Gagniuc wrote:

> This value is not used by u-boot, and it should not. The load address
> of an OPTEE image is defined by said image. Either a uImage or a FIT
> will have a defined load address and entry point. Those values are the
> correct ones, not CONFIG_OPTEE_LOAD_ADDR.
> 
> Commit f25006b96e9f ("optee: Add CONFIG_OPTEE_LOAD_ADDR") justifies
> this config by requiring its presence in u-boot's .config for other
> images as part of a larger build, claiming it is "the best way".
> 
> This argument is not persuasive. U-boot's configuration is driven by
> platform requirements, not the other way around. It seems more likely
> that the argument is conflating tooling issues with Kconfig. Yocto and
> buildroot have excellent mechanisms for defining values across the
> board (pun intended). u-boot's Kconfig is the wrong place to do it.
> 
> Furthermore, it is not "best" for u-boot because it hardcodes a value
> which is then not used. In fact the load address that u-boot uses is
> the one derived from the OPTEE image.
> 
> Confused yet? I sure was. To prevent future confusion, remove
> CONFIG_OPTEE_LOAD_ADDR.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 4/4] arm: imx: mx7: Move CONFIG_OPTEE_TZDRAM_SIZE from lib/optee

[Alexandru Gagniuc]

This config is only used by three boards with this SOC. Most other
platforms derive this information from devicetree, and are unlikely
to ever need this config.

Moreover, it is confusing when Kconfig asks for this value under
"Support OPTEE images", but does not do anything with the value.
Move it to imx7 for those boards who still make use of it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 arch/arm/mach-imx/mx7/Kconfig | 8 ++++++++
 lib/optee/Kconfig             | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-imx/mx7/Kconfig b/arch/arm/mach-imx/mx7/Kconfig
index adedc01164..26cb10f6a8 100644
--- a/arch/arm/mach-imx/mx7/Kconfig
+++ b/arch/arm/mach-imx/mx7/Kconfig
@@ -23,6 +23,14 @@ config SPL_TEXT_BASE
 	depends on SPL
 	default 0x00912000
 
+config OPTEE_TZDRAM_SIZE
+	hex "Amount of Trust-Zone RAM for the OPTEE image"
+	default 0x0000000
+	depends on OPTEE
+	help
+	  The size of pre-allocated Trust Zone DRAM to allocate for the OPTEE
+	  runtime.
+
 choice
 	prompt "MX7 board select"
 	optional
diff --git a/lib/optee/Kconfig b/lib/optee/Kconfig
index 26677b7548..9d11b5f37d 100644
--- a/lib/optee/Kconfig
+++ b/lib/optee/Kconfig
@@ -7,14 +7,6 @@ config OPTEE
           OPTEE specific checks before booting an OPTEE image created with
           mkimage.
 
-config OPTEE_TZDRAM_SIZE
-	hex "Amount of Trust-Zone RAM for the OPTEE image"
-	default 0x0000000
-	depends on OPTEE
-	help
-	  The size of pre-allocated Trust Zone DRAM to allocate for the OPTEE
-	  runtime.
-
 config BOOTM_OPTEE
 	bool "Support OPTEE bootm command"
 	select BOOTM_LINUX
-- 
2.31.1

Re: [PATCH 4/4] arm: imx: mx7: Move CONFIG_OPTEE_TZDRAM_SIZE from lib/optee

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Tue, Sep 07, 2021 at 12:07:09PM -0500, Alexandru Gagniuc wrote:

> This config is only used by three boards with this SOC. Most other
> platforms derive this information from devicetree, and are unlikely
> to ever need this config.
> 
> Moreover, it is confusing when Kconfig asks for this value under
> "Support OPTEE images", but does not do anything with the value.
> Move it to imx7 for those boards who still make use of it.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]