[PATCHv2] Set last_comp_version correctly in new dtb and fix potential version issues in fdt_open_into

[PATCHv2] Set last_comp_version correctly in new dtb and fix potential version issues in fdt_open_into

[Justin Covell]

Hi,

I've added checks to fdt_open_into to validate the version before reading into buffer, as well as maintaining the accurate
version information of the fdt when loaded into the buffer. Hopefully this would help stop any issues with reading a 
fdt with a lower than compatible verison into a buffer and it being misrepresented as a current version.

Signed-off-by: Justin Covell <jujugoboom-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

---
 libfdt/fdt_rw.c | 10 ++++++----
 libfdt/fdt_sw.c |  2 +-
 libfdt/libfdt.h |  1 +
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 68887b9..feab26c 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -428,12 +428,14 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
 
 	if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
 		struct_size = fdt_size_dt_struct(fdt);
-	} else {
+	} else if (fdt_version(fdt) == 16) {
 		struct_size = 0;
 		while (fdt_next_tag(fdt, struct_size, &struct_size) != FDT_END)
 			;
 		if (struct_size < 0)
 			return struct_size;
+	} else {
+		return -FDT_ERR_BADVERSION;
 	}
 
 	if (can_assume(LIBFDT_ORDER) ||
@@ -442,7 +444,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
 		err = fdt_move(fdt, buf, bufsize);
 		if (err)
 			return err;
-		fdt_set_version(buf, 17);
+		fdt_set_version(buf, fdt_version(fdt));
 		fdt_set_size_dt_struct(buf, struct_size);
 		fdt_set_totalsize(buf, bufsize);
 		return 0;
@@ -470,8 +472,8 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
 
 	fdt_set_magic(buf, FDT_MAGIC);
 	fdt_set_totalsize(buf, bufsize);
-	fdt_set_version(buf, 17);
-	fdt_set_last_comp_version(buf, 16);
+	fdt_set_version(buf, fdt_version(fdt));
+	fdt_set_last_comp_version(buf, fdt_last_comp_version(fdt));
 	fdt_set_boot_cpuid_phys(buf, fdt_boot_cpuid_phys(fdt));
 
 	return 0;
diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index 68b543c..4c569ee 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -377,7 +377,7 @@ int fdt_finish(void *fdt)
 	fdt_set_totalsize(fdt, newstroffset + fdt_size_dt_strings(fdt));
 
 	/* And fix up fields that were keeping intermediate state. */
-	fdt_set_last_comp_version(fdt, FDT_FIRST_SUPPORTED_VERSION);
+	fdt_set_last_comp_version(fdt, FDT_LAST_COMPATIBLE_VERSION);
 	fdt_set_magic(fdt, FDT_MAGIC);
 
 	return 0;
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 2bc16a8..73467f7 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -14,6 +14,7 @@ extern "C" {
 #endif
 
 #define FDT_FIRST_SUPPORTED_VERSION	0x02
+#define FDT_LAST_COMPATIBLE_VERSION 0x10
 #define FDT_LAST_SUPPORTED_VERSION	0x11
 
 /* Error codes: informative error codes */
-- 
2.25.1

Re: [PATCHv2] Set last_comp_version correctly in new dtb and fix potential version issues in fdt_open_into

[David Gibson]

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

On Mon, Dec 28, 2020 at 03:42:43PM -0800, Justin Covell wrote:
> Hi,
> 
> I've added checks to fdt_open_into to validate the version before reading into buffer, as well as maintaining the accurate
> version information of the fdt when loaded into the buffer. Hopefully this would help stop any issues with reading a 
> fdt with a lower than compatible verison into a buffer and it being misrepresented as a current version.
> 
> Signed-off-by: Justin Covell <jujugoboom-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> ---
>  libfdt/fdt_rw.c | 10 ++++++----
>  libfdt/fdt_sw.c |  2 +-
>  libfdt/libfdt.h |  1 +
>  3 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 68887b9..feab26c 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -428,12 +428,14 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>  
>  	if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
>  		struct_size = fdt_size_dt_struct(fdt);
> -	} else {
> +	} else if (fdt_version(fdt) == 16) {
>  		struct_size = 0;
>  		while (fdt_next_tag(fdt, struct_size, &struct_size) != FDT_END)
>  			;
>  		if (struct_size < 0)
>  			return struct_size;
> +	} else {
> +		return -FDT_ERR_BADVERSION;

Right, this is further fallout from f1879e1a50ebc3786540a075701ccaead2bfbe1f

>  	}
>  
>  	if (can_assume(LIBFDT_ORDER) ||
> @@ -442,7 +444,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>  		err = fdt_move(fdt, buf, bufsize);
>  		if (err)
>  			return err;
> -		fdt_set_version(buf, 17);
> +		fdt_set_version(buf, fdt_version(fdt));

This change doesn't make sense, though.  For starters, it's a no-op by
definition.  Secondly the change to v17 is correct: the difference
between v16 and v17 is that v17 adds the struct block size, which we
populate in the next line.

>  		fdt_set_size_dt_struct(buf, struct_size);
>  		fdt_set_totalsize(buf, bufsize);
>  		return 0;
> @@ -470,8 +472,8 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>  
>  	fdt_set_magic(buf, FDT_MAGIC);
>  	fdt_set_totalsize(buf, bufsize);
> -	fdt_set_version(buf, 17);
> -	fdt_set_last_comp_version(buf, 16);
> +	fdt_set_version(buf, fdt_version(fdt));
> +	fdt_set_last_comp_version(buf, fdt_last_comp_version(fdt));

Likewise, these are no-ops, and the original version was correct.

>  	fdt_set_boot_cpuid_phys(buf, fdt_boot_cpuid_phys(fdt));
>  
>  	return 0;
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index 68b543c..4c569ee 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -377,7 +377,7 @@ int fdt_finish(void *fdt)
>  	fdt_set_totalsize(fdt, newstroffset + fdt_size_dt_strings(fdt));
>  
>  	/* And fix up fields that were keeping intermediate state. */
> -	fdt_set_last_comp_version(fdt, FDT_FIRST_SUPPORTED_VERSION);
> +	fdt_set_last_comp_version(fdt, FDT_LAST_COMPATIBLE_VERSION);

This is a necessary change, though, again because of f1879e1a.  So
adding a "Fixes" tag to the commit message would be useful.

>  	fdt_set_magic(fdt, FDT_MAGIC);
>  
>  	return 0;
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 2bc16a8..73467f7 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -14,6 +14,7 @@ extern "C" {
>  #endif
>  
>  #define FDT_FIRST_SUPPORTED_VERSION	0x02
> +#define FDT_LAST_COMPATIBLE_VERSION 0x10
>  #define FDT_LAST_SUPPORTED_VERSION	0x11
>  
>  /* Error codes: informative error codes */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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