[U-Boot] [PATCH 1/3] fdtdec: protect against another NULL phandlep in fdtdec_add_reserved_memory()

[U-Boot] [PATCH 1/3] fdtdec: protect against another NULL phandlep in fdtdec_add_reserved_memory()

[Heiko Stuebner]

The change adding fdtdec_add_reserved_memory() already protected the added
phandle against the phandlep being NULL - making the phandlep var optional.

But in the early code checking for an already existing carveout this check
was not done and thus the phandle assignment could run into trouble,
so add a check there as well, which makes the function still return
sucessfully if a matching region is found, even though no-one wants to
work with the phandle.

Fixes: c9222a08b3f7 ("fdtdec: Implement fdtdec_add_reserved_memory()")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 lib/fdtdec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 74525c84e7..17455c5506 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1363,7 +1363,8 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
 		}
 
 		if (addr == carveout->start && (addr + size) == carveout->end) {
-			*phandlep = fdt_get_phandle(blob, node);
+			if (phandlep)
+				*phandlep = fdt_get_phandle(blob, node);
 			return 0;
 		}
 	}
-- 
2.23.0

[U-Boot] [PATCH 2/3] fdtdec: only create phandle if caller wants it in fdtdec_add_reserved_memory()

[Heiko Stuebner]

The phandlep pointer returning the phandle to the caller is optional
and if it is not set when calling fdtdec_add_reserved_memory() it is
highly likely that the caller is not interested in a phandle to the
created reserved-memory area and really just wants that area added.

So just don't create a phandle in that case.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 lib/fdtdec.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 17455c5506..39e87e89c9 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1393,13 +1393,15 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
 	if (node < 0)
 		return node;
 
-	err = fdt_generate_phandle(blob, &phandle);
-	if (err < 0)
-		return err;
-
-	err = fdtdec_set_phandle(blob, node, phandle);
-	if (err < 0)
-		return err;
+	if (phandlep) {
+		err = fdt_generate_phandle(blob, &phandle);
+		if (err < 0)
+			return err;
+
+		err = fdtdec_set_phandle(blob, node, phandle);
+		if (err < 0)
+			return err;
+	}
 
 	/* store one or two address cells */
 	if (na > 1)
-- 
2.23.0

[U-Boot] [PATCH 2/3] fdtdec: only create phandle if caller wants it in fdtdec_add_reserved_memory()

[Simon Glass]

Hi Heiko,

On Mon, 7 Oct 2019 at 18:22, Heiko Stuebner <heiko@sntech.de> wrote:
>
> The phandlep pointer returning the phandle to the caller is optional
> and if it is not set when calling fdtdec_add_reserved_memory() it is
> highly likely that the caller is not interested in a phandle to the
> created reserved-memory area and really just wants that area added.
>
> So just don't create a phandle in that case.

Update function comment?

>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  lib/fdtdec.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

Regards,
Simon

[U-Boot] [PATCH 3/3] image: fdt: copy possible optee nodes to a loaded devicetree

[Heiko Stuebner]

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);
+	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

[U-Boot] [PATCH 3/3] image: fdt: copy possible optee nodes to a loaded devicetree

[Simon Glass]

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

[U-Boot] [PATCH 3/3] image: fdt: copy possible optee nodes to a loaded devicetree

[Heiko Stübner]

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

[U-Boot] [PATCH 3/3] image: fdt: copy possible optee nodes to a loaded devicetree

[Heiko Stübner]

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

[U-Boot] [PATCH 3/3] image: fdt: copy possible optee nodes to a loaded devicetree

[Jens Wiklander]

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
>

[U-Boot] [PATCH 1/3] fdtdec: protect against another NULL phandlep in fdtdec_add_reserved_memory()

[Simon Glass]

Hi Heiko,

On Mon, 7 Oct 2019 at 18:22, Heiko Stuebner <heiko@sntech.de> wrote:
>
> The change adding fdtdec_add_reserved_memory() already protected the added
> phandle against the phandlep being NULL - making the phandlep var optional.
>
> But in the early code checking for an already existing carveout this check
> was not done and thus the phandle assignment could run into trouble,
> so add a check there as well, which makes the function still return
> sucessfully if a matching region is found, even though no-one wants to

successfully

> work with the phandle.
>
> Fixes: c9222a08b3f7 ("fdtdec: Implement fdtdec_add_reserved_memory()")
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  lib/fdtdec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 74525c84e7..17455c5506 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1363,7 +1363,8 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
>                 }
>
>                 if (addr == carveout->start && (addr + size) == carveout->end) {
> -                       *phandlep = fdt_get_phandle(blob, node);
> +                       if (phandlep)
> +                               *phandlep = fdt_get_phandle(blob, node);

Can you please update the function comment to indicate that this can be NULL?

>                         return 0;
>                 }
>         }
> --
> 2.23.0
>

Regards,
Simon