| Message ID | 20191008002207.14396-3-heiko@sntech.de |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot,1/3] fdtdec: protect against another NULL phandlep in fdtdec_add_reserved_memory() | expand |
Hi Heiko, On Mon, 7 Oct 2019 at 18:22, Heiko Stuebner <heiko@sntech.de> wrote: > > The loading convention for optee or any other tee on arm64 is as bl32 > parameter to the trusted-firmware. So TF-A gets invoked with the TEE as > bl32 and main u-boot as bl33. Once it has done its startup TF-A jumps > into the bl32 for the TEE startup, returns to TF-A and then jumps to bl33. > > All of them get passed a devicetree as parameter and all components often > get loaded from a FIT image. > > OP-TEE will create additional nodes in that devicetree namely a firmware > node and possibly multiple reserved-memory nodes. > > While this devicetree is used in main u-boot, in most cases it won't be > the one passed to the actual kernel. Instead most boot commands will load > a new devicetree from somewhere like mass storage of the network, so if > that happens u-boot should transfer the optee nodes to that new devicetree. > > To make that happen introduce optee_copy_fdt_nodes() called from the dt > setup function in image-fdt which after checking for the optee presence > in the u-boot dt will make sure a optee node is present in the kernel dt > and transfer any reserved-memory regions it can find. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > This goes together with my bl32 work for the spl_atf loader in > https://patchwork.ozlabs.org/patch/1172565/ > > common/image-fdt.c | 8 ++++ > include/tee/optee.h | 9 ++++ > lib/optee/optee.c | 112 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 129 insertions(+) Could we please get a test for this new functionality? Regards, Simon
Hi Heiko, [+Igor] On Tue, Oct 8, 2019 at 2:22 AM Heiko Stuebner <heiko@sntech.de> wrote: > > The loading convention for optee or any other tee on arm64 is as bl32 > parameter to the trusted-firmware. So TF-A gets invoked with the TEE as > bl32 and main u-boot as bl33. Once it has done its startup TF-A jumps > into the bl32 for the TEE startup, returns to TF-A and then jumps to bl33. > > All of them get passed a devicetree as parameter and all components often > get loaded from a FIT image. > > OP-TEE will create additional nodes in that devicetree namely a firmware > node and possibly multiple reserved-memory nodes. > > While this devicetree is used in main u-boot, in most cases it won't be > the one passed to the actual kernel. Instead most boot commands will load > a new devicetree from somewhere like mass storage of the network, so if > that happens u-boot should transfer the optee nodes to that new devicetree. > > To make that happen introduce optee_copy_fdt_nodes() called from the dt > setup function in image-fdt which after checking for the optee presence > in the u-boot dt will make sure a optee node is present in the kernel dt > and transfer any reserved-memory regions it can find. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > This goes together with my bl32 work for the spl_atf loader in > https://patchwork.ozlabs.org/patch/1172565/ > > common/image-fdt.c | 8 ++++ > include/tee/optee.h | 9 ++++ > lib/optee/optee.c | 112 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 129 insertions(+) > > diff --git a/common/image-fdt.c b/common/image-fdt.c > index 4247dcee0c..48388488d9 100644 > --- a/common/image-fdt.c > +++ b/common/image-fdt.c > @@ -17,6 +17,7 @@ > #include <linux/libfdt.h> > #include <mapmem.h> > #include <asm/io.h> > +#include <tee/optee.h> > > #ifndef CONFIG_SYS_FDT_PAD > #define CONFIG_SYS_FDT_PAD 0x3000 > @@ -561,6 +562,13 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob, > } > } > > + fdt_ret = optee_copy_fdt_nodes(gd->fdt_blob, blob); > + if (fdt_ret) { > + printf("ERROR: transfer of optee nodes to new fdt failed: %s\n", > + fdt_strerror(fdt_ret)); > + goto err; > + } > + > /* Delete the old LMB reservation */ > if (lmb) > lmb_free(lmb, (phys_addr_t)(u32)(uintptr_t)blob, > diff --git a/include/tee/optee.h b/include/tee/optee.h > index 9446928fd4..121b30a303 100644 > --- a/include/tee/optee.h > +++ b/include/tee/optee.h > @@ -67,4 +67,13 @@ static inline int optee_verify_bootm_image(unsigned long image_addr, > } > #endif > > +#if defined(CONFIG_OPTEE) && defined(CONFIG_OF_LIBFDT) > +int optee_copy_fdt_nodes(const void *old_blob, void *new_blob); > +#else > +static inline int optee_copy_fdt_nodes(const void *old_blob, void *new_blob) > +{ > + return 0; > +} > +#endif > + > #endif /* _OPTEE_H */ > diff --git a/lib/optee/optee.c b/lib/optee/optee.c > index db92cd9af2..f484b12e67 100644 > --- a/lib/optee/optee.c > +++ b/lib/optee/optee.c > @@ -5,6 +5,8 @@ > */ > > #include <common.h> > +#include <malloc.h> > +#include <linux/libfdt.h> > #include <tee/optee.h> > > #define optee_hdr_err_msg \ > @@ -63,3 +65,113 @@ error: > > return ret; > } > + > +#if defined(CONFIG_OF_LIBFDT) > +static int optee_add_firmware_node(void *fdt_blob) > +{ > + int offs, ret; > + > + if (fdt_path_offset(fdt_blob, "/firmware/optee") >= 0) { > + debug("OP-TEE Device Tree node already exists"); > + return 0; > + } > + > + offs = fdt_path_offset(fdt_blob, "/firmware"); > + if (offs < 0) { > + offs = fdt_path_offset(fdt_blob, "/"); > + if (offs < 0) > + return offs; > + > + offs = fdt_add_subnode(fdt_blob, offs, "firmware"); > + if (offs < 0) > + return offs; > + } > + > + offs = fdt_add_subnode(fdt_blob, offs, "optee"); > + if (offs < 0) > + return ret; > + > + ret = fdt_setprop_string(fdt_blob, offs, "compatible", > + "linaro,optee-tz"); > + if (ret < 0) > + return ret; > + > + ret = fdt_setprop_string(fdt_blob, offs, "method", "smc"); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +int optee_copy_fdt_nodes(const void *old_blob, void *new_blob) > +{ > + int nodeoffset, subnode, ret; > + struct fdt_resource res; > + > + if (fdt_check_header(old_blob)) > + return -EINVAL; > + > + /* only proceed if there is an /firmware/optee node */ > + if (fdt_path_offset(old_blob, "/firmware/optee") < 0) { > + debug("No OP-TEE firmware node in old fdt, nothing to do"); > + return 0; > + } > + > + ret = optee_add_firmware_node(new_blob); I think it's more safe to copy the OP-TEE node instead. The values of "compatible" and "method" may be different on other platforms, "hvc" instead of "smc" for instance. Thanks, Jens > + if (ret < 0) { > + printf("Failed to add OP-TEE firmware node\n"); > + return ret; > + } > + > + /* optee inserts its memory regions as reserved-memory nodes */ > + nodeoffset = fdt_subnode_offset(old_blob, 0, "reserved-memory"); > + if (nodeoffset >= 0) { > + subnode = fdt_first_subnode(old_blob, nodeoffset); > + while (subnode >= 0) { > + const char *name = fdt_get_name(old_blob, > + subnode, NULL); > + if (!name) > + return -EINVAL; > + > + /* only handle optee reservations */ > + if (strncmp(name, "optee", 5)) > + continue; > + > + /* check if this subnode has a reg property */ > + ret = fdt_get_resource(old_blob, subnode, "reg", 0, > + &res); > + if (!ret) { > + struct fdt_memory carveout = { > + .start = res.start, > + .end = res.end, > + }; > + char *oldname, *nodename, *tmp; > + > + oldname = strdup(name); > + if (!oldname) > + return -ENOMEM; > + > + tmp = oldname; > + nodename = strsep(&tmp, "@"); > + if (!nodename) { > + free(oldname); > + return -EINVAL; > + } > + > + ret = fdtdec_add_reserved_memory(new_blob, > + nodename, > + &carveout, > + NULL); > + free(oldname); > + > + if (ret < 0) > + return ret; > + } > + > + subnode = fdt_next_subnode(old_blob, subnode); > + } > + } > + > + return 0; > +} > +#endif > -- > 2.23.0 >
Hi Simon, Am Dienstag, 22. Oktober 2019, 02:17:00 CEST schrieb Simon Glass: > On Mon, 7 Oct 2019 at 18:22, Heiko Stuebner <heiko@sntech.de> wrote: > > > > The loading convention for optee or any other tee on arm64 is as bl32 > > parameter to the trusted-firmware. So TF-A gets invoked with the TEE as > > bl32 and main u-boot as bl33. Once it has done its startup TF-A jumps > > into the bl32 for the TEE startup, returns to TF-A and then jumps to bl33. > > > > All of them get passed a devicetree as parameter and all components often > > get loaded from a FIT image. > > > > OP-TEE will create additional nodes in that devicetree namely a firmware > > node and possibly multiple reserved-memory nodes. > > > > While this devicetree is used in main u-boot, in most cases it won't be > > the one passed to the actual kernel. Instead most boot commands will load > > a new devicetree from somewhere like mass storage of the network, so if > > that happens u-boot should transfer the optee nodes to that new devicetree. > > > > To make that happen introduce optee_copy_fdt_nodes() called from the dt > > setup function in image-fdt which after checking for the optee presence > > in the u-boot dt will make sure a optee node is present in the kernel dt > > and transfer any reserved-memory regions it can find. > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > --- > > This goes together with my bl32 work for the spl_atf loader in > > https://patchwork.ozlabs.org/patch/1172565/ > > > > common/image-fdt.c | 8 ++++ > > include/tee/optee.h | 9 ++++ > > lib/optee/optee.c | 112 ++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 129 insertions(+) > > Could we please get a test for this new functionality? We can try ;-) , but I think I'll need some pointers how this should be done. On first glance, test/overlay/* seems to be a good inspiration. Aka the new function relies on an opaque OP-TEE binary modifying the .itb's dt-blob between SPL and main-U-Boot, so working with stub devicetrees for testing seems reasonable. Aka checking cases of optee-nodes in old_fdt and not having them. Looking at the README though points me to "TODO: convert to pytest" it seems. Though that README.md only talks about interacting with the u-boot console, so I'm not sure how that should work. Thanks Heiko
Hi Simon, Am Dienstag, 22. Oktober 2019, 14:08:04 CEST schrieb Heiko Stübner: > Am Dienstag, 22. Oktober 2019, 02:17:00 CEST schrieb Simon Glass: > > On Mon, 7 Oct 2019 at 18:22, Heiko Stuebner <heiko@sntech.de> wrote: > > > > > > The loading convention for optee or any other tee on arm64 is as bl32 > > > parameter to the trusted-firmware. So TF-A gets invoked with the TEE as > > > bl32 and main u-boot as bl33. Once it has done its startup TF-A jumps > > > into the bl32 for the TEE startup, returns to TF-A and then jumps to bl33. > > > > > > All of them get passed a devicetree as parameter and all components often > > > get loaded from a FIT image. > > > > > > OP-TEE will create additional nodes in that devicetree namely a firmware > > > node and possibly multiple reserved-memory nodes. > > > > > > While this devicetree is used in main u-boot, in most cases it won't be > > > the one passed to the actual kernel. Instead most boot commands will load > > > a new devicetree from somewhere like mass storage of the network, so if > > > that happens u-boot should transfer the optee nodes to that new devicetree. > > > > > > To make that happen introduce optee_copy_fdt_nodes() called from the dt > > > setup function in image-fdt which after checking for the optee presence > > > in the u-boot dt will make sure a optee node is present in the kernel dt > > > and transfer any reserved-memory regions it can find. > > > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > > --- > > > This goes together with my bl32 work for the spl_atf loader in > > > https://patchwork.ozlabs.org/patch/1172565/ > > > > > > common/image-fdt.c | 8 ++++ > > > include/tee/optee.h | 9 ++++ > > > lib/optee/optee.c | 112 ++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 129 insertions(+) > > > > Could we please get a test for this new functionality? > > We can try ;-) , but I think I'll need some pointers how this should be done. > > On first glance, test/overlay/* seems to be a good inspiration. Aka the new > function relies on an opaque OP-TEE binary modifying the .itb's dt-blob > between SPL and main-U-Boot, so working with stub devicetrees for > testing seems reasonable. > > Aka checking cases of optee-nodes in old_fdt and not having them. I just did go ahead and implemented a test like that, see v2 series. Is this what you had in mind? Heiko
diff --git a/common/image-fdt.c b/common/image-fdt.c index 4247dcee0c..48388488d9 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -17,6 +17,7 @@ #include <linux/libfdt.h> #include <mapmem.h> #include <asm/io.h> +#include <tee/optee.h> #ifndef CONFIG_SYS_FDT_PAD #define CONFIG_SYS_FDT_PAD 0x3000 @@ -561,6 +562,13 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob, } } + fdt_ret = optee_copy_fdt_nodes(gd->fdt_blob, blob); + if (fdt_ret) { + printf("ERROR: transfer of optee nodes to new fdt failed: %s\n", + fdt_strerror(fdt_ret)); + goto err; + } + /* Delete the old LMB reservation */ if (lmb) lmb_free(lmb, (phys_addr_t)(u32)(uintptr_t)blob, diff --git a/include/tee/optee.h b/include/tee/optee.h index 9446928fd4..121b30a303 100644 --- a/include/tee/optee.h +++ b/include/tee/optee.h @@ -67,4 +67,13 @@ static inline int optee_verify_bootm_image(unsigned long image_addr, } #endif +#if defined(CONFIG_OPTEE) && defined(CONFIG_OF_LIBFDT) +int optee_copy_fdt_nodes(const void *old_blob, void *new_blob); +#else +static inline int optee_copy_fdt_nodes(const void *old_blob, void *new_blob) +{ + return 0; +} +#endif + #endif /* _OPTEE_H */ diff --git a/lib/optee/optee.c b/lib/optee/optee.c index db92cd9af2..f484b12e67 100644 --- a/lib/optee/optee.c +++ b/lib/optee/optee.c @@ -5,6 +5,8 @@ */ #include <common.h> +#include <malloc.h> +#include <linux/libfdt.h> #include <tee/optee.h> #define optee_hdr_err_msg \ @@ -63,3 +65,113 @@ error: return ret; } + +#if defined(CONFIG_OF_LIBFDT) +static int optee_add_firmware_node(void *fdt_blob) +{ + int offs, ret; + + if (fdt_path_offset(fdt_blob, "/firmware/optee") >= 0) { + debug("OP-TEE Device Tree node already exists"); + return 0; + } + + offs = fdt_path_offset(fdt_blob, "/firmware"); + if (offs < 0) { + offs = fdt_path_offset(fdt_blob, "/"); + if (offs < 0) + return offs; + + offs = fdt_add_subnode(fdt_blob, offs, "firmware"); + if (offs < 0) + return offs; + } + + offs = fdt_add_subnode(fdt_blob, offs, "optee"); + if (offs < 0) + return ret; + + ret = fdt_setprop_string(fdt_blob, offs, "compatible", + "linaro,optee-tz"); + if (ret < 0) + return ret; + + ret = fdt_setprop_string(fdt_blob, offs, "method", "smc"); + if (ret < 0) + return ret; + + return 0; +} + +int optee_copy_fdt_nodes(const void *old_blob, void *new_blob) +{ + int nodeoffset, subnode, ret; + struct fdt_resource res; + + if (fdt_check_header(old_blob)) + return -EINVAL; + + /* only proceed if there is an /firmware/optee node */ + if (fdt_path_offset(old_blob, "/firmware/optee") < 0) { + debug("No OP-TEE firmware node in old fdt, nothing to do"); + return 0; + } + + ret = optee_add_firmware_node(new_blob); + if (ret < 0) { + printf("Failed to add OP-TEE firmware node\n"); + return ret; + } + + /* optee inserts its memory regions as reserved-memory nodes */ + nodeoffset = fdt_subnode_offset(old_blob, 0, "reserved-memory"); + if (nodeoffset >= 0) { + subnode = fdt_first_subnode(old_blob, nodeoffset); + while (subnode >= 0) { + const char *name = fdt_get_name(old_blob, + subnode, NULL); + if (!name) + return -EINVAL; + + /* only handle optee reservations */ + if (strncmp(name, "optee", 5)) + continue; + + /* check if this subnode has a reg property */ + ret = fdt_get_resource(old_blob, subnode, "reg", 0, + &res); + if (!ret) { + struct fdt_memory carveout = { + .start = res.start, + .end = res.end, + }; + char *oldname, *nodename, *tmp; + + oldname = strdup(name); + if (!oldname) + return -ENOMEM; + + tmp = oldname; + nodename = strsep(&tmp, "@"); + if (!nodename) { + free(oldname); + return -EINVAL; + } + + ret = fdtdec_add_reserved_memory(new_blob, + nodename, + &carveout, + NULL); + free(oldname); + + if (ret < 0) + return ret; + } + + subnode = fdt_next_subnode(old_blob, subnode); + } + } + + return 0; +} +#endif
The loading convention for optee or any other tee on arm64 is as bl32 parameter to the trusted-firmware. So TF-A gets invoked with the TEE as bl32 and main u-boot as bl33. Once it has done its startup TF-A jumps into the bl32 for the TEE startup, returns to TF-A and then jumps to bl33. All of them get passed a devicetree as parameter and all components often get loaded from a FIT image. OP-TEE will create additional nodes in that devicetree namely a firmware node and possibly multiple reserved-memory nodes. While this devicetree is used in main u-boot, in most cases it won't be the one passed to the actual kernel. Instead most boot commands will load a new devicetree from somewhere like mass storage of the network, so if that happens u-boot should transfer the optee nodes to that new devicetree. To make that happen introduce optee_copy_fdt_nodes() called from the dt setup function in image-fdt which after checking for the optee presence in the u-boot dt will make sure a optee node is present in the kernel dt and transfer any reserved-memory regions it can find. Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- This goes together with my bl32 work for the spl_atf loader in https://patchwork.ozlabs.org/patch/1172565/ common/image-fdt.c | 8 ++++ include/tee/optee.h | 9 ++++ lib/optee/optee.c | 112 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 129 insertions(+)