diff mbox series

arm64: zynqmp: Fix set_fdtfile() not to break u-boots DTB

Message ID 5192ad3001c704c47cb578746f3200aab1dabd94.1593002658.git.michal.simek@xilinx.com
State New
Delegated to: Michal Simek
Headers show
Series arm64: zynqmp: Fix set_fdtfile() not to break u-boots DTB | expand

Commit Message

Michal Simek June 24, 2020, 12:44 p.m. UTC
Origin function was calling strsep which replaced delimiter ',' by a null
byte ('\0'). Operation was done directly on FDT which ends up with the
following behavior:

ZynqMP>  printenv fdtfile
fdtfile=xilinx/zynqmp.dtb
ZynqMP> fdt addr $fdtcontroladdr
ZynqMP> fdt print / compatible
compatible = "xlnx", "zynqmp"

As is visible fdtfile was correctly composed but a null byte caused that
xlnx was separated from zynqmp.
This hasn't been spotted because in all Xilinx DTs there are at least 3
compatible string and only the first one was affected by this issue.
But for systems which only had one compatible string "xlnx,zynqmp" it was
causing an issue when U-Boot's DT was used by Linux kernel.

The patch removes strsep calling and strchr is called instead which just
locate the first char after deliminator ',' (variable called "name").
And using this pointer in fdtfile composing.

Fixes: 91d7e0c47f51 ("arm64: zynqmp: Create fdtfile from compatible string")
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 board/xilinx/zynqmp/zynqmp.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

Michal Simek June 24, 2020, 12:52 p.m. UTC | #1
On 24. 06. 20 14:44, Michal Simek wrote:
> Origin function was calling strsep which replaced delimiter ',' by a null
> byte ('\0'). Operation was done directly on FDT which ends up with the
> following behavior:
> 
> ZynqMP>  printenv fdtfile
> fdtfile=xilinx/zynqmp.dtb
> ZynqMP> fdt addr $fdtcontroladdr
> ZynqMP> fdt print / compatible
> compatible = "xlnx", "zynqmp"
> 
> As is visible fdtfile was correctly composed but a null byte caused that
> xlnx was separated from zynqmp.
> This hasn't been spotted because in all Xilinx DTs there are at least 3
> compatible string and only the first one was affected by this issue.
> But for systems which only had one compatible string "xlnx,zynqmp" it was
> causing an issue when U-Boot's DT was used by Linux kernel.
> 
> The patch removes strsep calling and strchr is called instead which just
> locate the first char after deliminator ',' (variable called "name").
> And using this pointer in fdtfile composing.
> 
> Fixes: 91d7e0c47f51 ("arm64: zynqmp: Create fdtfile from compatible string")
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
>  board/xilinx/zynqmp/zynqmp.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index ebb71729081d..8a4df6fc1ab6 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -541,23 +541,30 @@ static int set_fdtfile(void)
>  	char *compatible, *fdtfile;
>  	const char *suffix = ".dtb";
>  	const char *vendor = "xilinx/";
> +	int fdt_compat_len;
>  
>  	if (env_get("fdtfile"))
>  		return 0;
>  
> -	compatible = (char *)fdt_getprop(gd->fdt_blob, 0, "compatible", NULL);
> -	if (compatible) {
> +	compatible = (char *)fdt_getprop(gd->fdt_blob, 0, "compatible",
> +					 &fdt_compat_len);
> +	if (compatible && fdt_compat_len) {
> +		char *name;
> +
>  		debug("Compatible: %s\n", compatible);
>  
> -		/* Discard vendor prefix */
> -		strsep(&compatible, ",");
> +		name = strchr(compatible, ',');
> +		if (!name)
> +			return -EINVAL;
> +
> +		name++;
>  
> -		fdtfile = calloc(1, strlen(vendor) + strlen(compatible) +
> +		fdtfile = calloc(1, strlen(vendor) + strlen(name) +
>  				 strlen(suffix) + 1);
>  		if (!fdtfile)
>  			return -ENOMEM;
>  
> -		sprintf(fdtfile, "%s%s%s", vendor, compatible, suffix);
> +		sprintf(fdtfile, "%s%s%s", vendor, name, suffix);
>  
>  		env_set("fdtfile", fdtfile);
>  		free(fdtfile);
> 


Also:
Reported-by: Igor Lantsman <igor.lantsman@opsys-tech.com>
Signed-off-by: Igor Lantsman <igor.lantsman@opsys-tech.com>

Thanks,
Michal
Michal Simek July 24, 2020, 12:14 p.m. UTC | #2
st 24. 6. 2020 v 14:44 odesílatel Michal Simek <michal.simek@xilinx.com> napsal:
>
> Origin function was calling strsep which replaced delimiter ',' by a null
> byte ('\0'). Operation was done directly on FDT which ends up with the
> following behavior:
>
> ZynqMP>  printenv fdtfile
> fdtfile=xilinx/zynqmp.dtb
> ZynqMP> fdt addr $fdtcontroladdr
> ZynqMP> fdt print / compatible
> compatible = "xlnx", "zynqmp"
>
> As is visible fdtfile was correctly composed but a null byte caused that
> xlnx was separated from zynqmp.
> This hasn't been spotted because in all Xilinx DTs there are at least 3
> compatible string and only the first one was affected by this issue.
> But for systems which only had one compatible string "xlnx,zynqmp" it was
> causing an issue when U-Boot's DT was used by Linux kernel.
>
> The patch removes strsep calling and strchr is called instead which just
> locate the first char after deliminator ',' (variable called "name").
> And using this pointer in fdtfile composing.
>
> Fixes: 91d7e0c47f51 ("arm64: zynqmp: Create fdtfile from compatible string")
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  board/xilinx/zynqmp/zynqmp.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index ebb71729081d..8a4df6fc1ab6 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -541,23 +541,30 @@ static int set_fdtfile(void)
>         char *compatible, *fdtfile;
>         const char *suffix = ".dtb";
>         const char *vendor = "xilinx/";
> +       int fdt_compat_len;
>
>         if (env_get("fdtfile"))
>                 return 0;
>
> -       compatible = (char *)fdt_getprop(gd->fdt_blob, 0, "compatible", NULL);
> -       if (compatible) {
> +       compatible = (char *)fdt_getprop(gd->fdt_blob, 0, "compatible",
> +                                        &fdt_compat_len);
> +       if (compatible && fdt_compat_len) {
> +               char *name;
> +
>                 debug("Compatible: %s\n", compatible);
>
> -               /* Discard vendor prefix */
> -               strsep(&compatible, ",");
> +               name = strchr(compatible, ',');
> +               if (!name)
> +                       return -EINVAL;
> +
> +               name++;
>
> -               fdtfile = calloc(1, strlen(vendor) + strlen(compatible) +
> +               fdtfile = calloc(1, strlen(vendor) + strlen(name) +
>                                  strlen(suffix) + 1);
>                 if (!fdtfile)
>                         return -ENOMEM;
>
> -               sprintf(fdtfile, "%s%s%s", vendor, compatible, suffix);
> +               sprintf(fdtfile, "%s%s%s", vendor, name, suffix);
>
>                 env_set("fdtfile", fdtfile);
>                 free(fdtfile);
> --
> 2.27.0
>

Applied.
M
diff mbox series

Patch

diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
index ebb71729081d..8a4df6fc1ab6 100644
--- a/board/xilinx/zynqmp/zynqmp.c
+++ b/board/xilinx/zynqmp/zynqmp.c
@@ -541,23 +541,30 @@  static int set_fdtfile(void)
 	char *compatible, *fdtfile;
 	const char *suffix = ".dtb";
 	const char *vendor = "xilinx/";
+	int fdt_compat_len;
 
 	if (env_get("fdtfile"))
 		return 0;
 
-	compatible = (char *)fdt_getprop(gd->fdt_blob, 0, "compatible", NULL);
-	if (compatible) {
+	compatible = (char *)fdt_getprop(gd->fdt_blob, 0, "compatible",
+					 &fdt_compat_len);
+	if (compatible && fdt_compat_len) {
+		char *name;
+
 		debug("Compatible: %s\n", compatible);
 
-		/* Discard vendor prefix */
-		strsep(&compatible, ",");
+		name = strchr(compatible, ',');
+		if (!name)
+			return -EINVAL;
+
+		name++;
 
-		fdtfile = calloc(1, strlen(vendor) + strlen(compatible) +
+		fdtfile = calloc(1, strlen(vendor) + strlen(name) +
 				 strlen(suffix) + 1);
 		if (!fdtfile)
 			return -ENOMEM;
 
-		sprintf(fdtfile, "%s%s%s", vendor, compatible, suffix);
+		sprintf(fdtfile, "%s%s%s", vendor, name, suffix);
 
 		env_set("fdtfile", fdtfile);
 		free(fdtfile);