[PATCH 1/1] dtc: fdtdump: check fdt if not in scanning mode

[PATCH 1/1] dtc: fdtdump: check fdt if not in scanning mode

[Heinrich Schuchardt]

Running fdtdump without scan mode for an invalid file often
results in a segmentation fault because the fdt header is
not checked.

With the patch the header is checked both in scanning as
well as in non-scanning mode.

Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
---
 fdtdump.c | 57 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/fdtdump.c b/fdtdump.c
index a9a2484..717fef5 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -189,39 +189,40 @@ int main(int argc, char *argv[])
 		die("could not read: %s\n", file);
 
 	/* try and locate an embedded fdt in a bigger blob */
-	if (scan) {
-		unsigned char smagic[FDT_MAGIC_SIZE];
-		char *p = buf;
-		char *endp = buf + len;
+	unsigned char smagic[FDT_MAGIC_SIZE];
+	char *p = buf;
+	char *endp = buf + len;
 
-		fdt_set_magic(smagic, FDT_MAGIC);
+	fdt_set_magic(smagic, FDT_MAGIC);
 
-		/* poor man's memmem */
-		while ((endp - p) >= FDT_MAGIC_SIZE) {
-			p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
-			if (!p)
+	/* poor man's memmem */
+	while ((endp - p) >= FDT_MAGIC_SIZE) {
+		p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
+		if (!p)
+			break;
+		if (fdt_magic(p) == FDT_MAGIC) {
+			/* try and validate the main struct */
+			off_t this_len = endp - p;
+			fdt32_t max_version = 17;
+			if (fdt_version(p) <= max_version &&
+			    fdt_last_comp_version(p) < max_version &&
+			    fdt_totalsize(p) < this_len &&
+			    fdt_off_dt_struct(p) < this_len &&
+				fdt_off_dt_strings(p) < this_len)
 				break;
-			if (fdt_magic(p) == FDT_MAGIC) {
-				/* try and validate the main struct */
-				off_t this_len = endp - p;
-				fdt32_t max_version = 17;
-				if (fdt_version(p) <= max_version &&
-				    fdt_last_comp_version(p) < max_version &&
-				    fdt_totalsize(p) < this_len &&
-				    fdt_off_dt_struct(p) < this_len &&
-					fdt_off_dt_strings(p) < this_len)
-					break;
-				if (debug)
-					printf("%s: skipping fdt magic at offset %#zx\n",
-						file, p - buf);
-			}
-			++p;
+			if (debug)
+				printf("%s: skipping fdt magic at offset %#zx\n",
+					file, p - buf);
 		}
-		if (!p || ((endp - p) < FDT_MAGIC_SIZE))
-			die("%s: could not locate fdt magic\n", file);
-		printf("%s: found fdt at offset %#zx\n", file, p - buf);
-		buf = p;
+		if (!scan)
+			die("%s: fdt missing\n", file);
+		++p;
 	}
+	if (!p || ((endp - p) < FDT_MAGIC_SIZE))
+		die("%s: could not locate fdt magic\n", file);
+	if (scan)
+		printf("%s: found fdt at offset %#zx\n", file, p - buf);
+	buf = p;
 
 	dump_blob(buf, debug);
 
-- 
2.11.0

Re: [PATCH 1/1] dtc: fdtdump: check fdt if not in scanning mode

[David Gibson]

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

On Tue, Dec 20, 2016 at 11:32:16PM +0100, Heinrich Schuchardt wrote:
> Running fdtdump without scan mode for an invalid file often
> results in a segmentation fault because the fdt header is
> not checked.
> 
> With the patch the header is checked both in scanning as
> well as in non-scanning mode.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>

This seems to be more complex than it needs to me.

The idea of validating the header in non-scan mode seems sensible, but
this also appears to scan in non-scan mode.

It would be more sensible to just pull the header validation out of
the scanning block.

That said, fdtdump is pretty much a deliberately dumb tool.  In
general the preferred way for decompiling a dtb is to use dtc -I dtb
-O dts.

> ---
>  fdtdump.c | 57 +++++++++++++++++++++++++++++----------------------------
>  1 file changed, 29 insertions(+), 28 deletions(-)
> 
> diff --git a/fdtdump.c b/fdtdump.c
> index a9a2484..717fef5 100644
> --- a/fdtdump.c
> +++ b/fdtdump.c
> @@ -189,39 +189,40 @@ int main(int argc, char *argv[])
>  		die("could not read: %s\n", file);
>  
>  	/* try and locate an embedded fdt in a bigger blob */
> -	if (scan) {
> -		unsigned char smagic[FDT_MAGIC_SIZE];
> -		char *p = buf;
> -		char *endp = buf + len;
> +	unsigned char smagic[FDT_MAGIC_SIZE];
> +	char *p = buf;
> +	char *endp = buf + len;
>  
> -		fdt_set_magic(smagic, FDT_MAGIC);
> +	fdt_set_magic(smagic, FDT_MAGIC);
>  
> -		/* poor man's memmem */
> -		while ((endp - p) >= FDT_MAGIC_SIZE) {
> -			p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
> -			if (!p)
> +	/* poor man's memmem */
> +	while ((endp - p) >= FDT_MAGIC_SIZE) {
> +		p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
> +		if (!p)
> +			break;
> +		if (fdt_magic(p) == FDT_MAGIC) {
> +			/* try and validate the main struct */
> +			off_t this_len = endp - p;
> +			fdt32_t max_version = 17;
> +			if (fdt_version(p) <= max_version &&
> +			    fdt_last_comp_version(p) < max_version &&
> +			    fdt_totalsize(p) < this_len &&
> +			    fdt_off_dt_struct(p) < this_len &&
> +				fdt_off_dt_strings(p) < this_len)
>  				break;
> -			if (fdt_magic(p) == FDT_MAGIC) {
> -				/* try and validate the main struct */
> -				off_t this_len = endp - p;
> -				fdt32_t max_version = 17;
> -				if (fdt_version(p) <= max_version &&
> -				    fdt_last_comp_version(p) < max_version &&
> -				    fdt_totalsize(p) < this_len &&
> -				    fdt_off_dt_struct(p) < this_len &&
> -					fdt_off_dt_strings(p) < this_len)
> -					break;
> -				if (debug)
> -					printf("%s: skipping fdt magic at offset %#zx\n",
> -						file, p - buf);
> -			}
> -			++p;
> +			if (debug)
> +				printf("%s: skipping fdt magic at offset %#zx\n",
> +					file, p - buf);
>  		}
> -		if (!p || ((endp - p) < FDT_MAGIC_SIZE))
> -			die("%s: could not locate fdt magic\n", file);
> -		printf("%s: found fdt at offset %#zx\n", file, p - buf);
> -		buf = p;
> +		if (!scan)
> +			die("%s: fdt missing\n", file);
> +		++p;
>  	}
> +	if (!p || ((endp - p) < FDT_MAGIC_SIZE))
> +		die("%s: could not locate fdt magic\n", file);
> +	if (scan)
> +		printf("%s: found fdt at offset %#zx\n", file, p - buf);
> +	buf = p;
>  
>  	dump_blob(buf, debug);
>  

-- 
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: 819 bytes --]

[PATCH 1/1 v2] dtc: fdtdump: check fdt if not in scanning mode

[Heinrich Schuchardt]

Running fdtdump without scan mode for an invalid file often
results in a segmentation fault because the fdt header is
not checked.

With the patch the header is checked both in scanning as
well as in non-scanning mode.

Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
---

v2:
	Refactor header check as separate function.

 fdtdump.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/fdtdump.c b/fdtdump.c
index a9a2484..4137689 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -16,6 +16,7 @@
 #include "util.h"
 
 #define FDT_MAGIC_SIZE	4
+#define MAX_VERSION 17
 
 #define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
 #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
@@ -159,6 +160,20 @@ static const char * const usage_opts_help[] = {
 	USAGE_COMMON_OPTS_HELP
 };
 
+static inline int valid_header(char *p, off_t len)
+{
+	if (len < sizeof(struct fdt_header) ||
+	    fdt_magic(p) != FDT_MAGIC ||
+	    fdt_version(p) > MAX_VERSION ||
+	    fdt_last_comp_version(p) >= MAX_VERSION ||
+	    fdt_totalsize(p) >= len ||
+	    fdt_off_dt_struct(p) >= len ||
+	    fdt_off_dt_strings(p) >= len)
+		return 0;
+	else
+		return 1;
+}
+
 int main(int argc, char *argv[])
 {
 	int opt;
@@ -204,12 +219,7 @@ int main(int argc, char *argv[])
 			if (fdt_magic(p) == FDT_MAGIC) {
 				/* try and validate the main struct */
 				off_t this_len = endp - p;
-				fdt32_t max_version = 17;
-				if (fdt_version(p) <= max_version &&
-				    fdt_last_comp_version(p) < max_version &&
-				    fdt_totalsize(p) < this_len &&
-				    fdt_off_dt_struct(p) < this_len &&
-					fdt_off_dt_strings(p) < this_len)
+				if (valid_header(p, this_len))
 					break;
 				if (debug)
 					printf("%s: skipping fdt magic at offset %#zx\n",
@@ -217,11 +227,12 @@ int main(int argc, char *argv[])
 			}
 			++p;
 		}
-		if (!p || ((endp - p) < FDT_MAGIC_SIZE))
+		if (!p || endp - p < sizeof(struct fdt_header))
 			die("%s: could not locate fdt magic\n", file);
 		printf("%s: found fdt at offset %#zx\n", file, p - buf);
 		buf = p;
-	}
+	} else if (!valid_header(buf, len))
+		die("%s: header is not valid\n", file);
 
 	dump_blob(buf, debug);
 
-- 
2.11.0

Re: [PATCH 1/1 v2] dtc: fdtdump: check fdt if not in scanning mode

[Simon Glass]

On 22 December 2016 at 12:59, Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org> wrote:
> Running fdtdump without scan mode for an invalid file often
> results in a segmentation fault because the fdt header is
> not checked.
>
> With the patch the header is checked both in scanning as
> well as in non-scanning mode.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
> ---
>
> v2:
>         Refactor header check as separate function.
>
>  fdtdump.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)

Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

nit: I don't think the function needs to be marked 'inline'. It is not
time-critical.

Re: [PATCH 1/1 v2] dtc: fdtdump: check fdt if not in scanning mode

[David Gibson]

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

On Mon, Dec 26, 2016 at 06:24:20PM +1300, Simon Glass wrote:
> On 22 December 2016 at 12:59, Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org> wrote:
> > Running fdtdump without scan mode for an invalid file often
> > results in a segmentation fault because the fdt header is
> > not checked.
> >
> > With the patch the header is checked both in scanning as
> > well as in non-scanning mode.
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
> > ---
> >
> > v2:
> >         Refactor header check as separate function.
> >
> >  fdtdump.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> nit: I don't think the function needs to be marked 'inline'. It is not
> time-critical.

Ah, yes.  In fact, the 'inline' keyword should generally be avoided
except in header files.  The compiler nearly always has better
information than you do about whether it's a good idea to inline a
function or not.

-- 
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: 819 bytes --]

Re: [PATCH 1/1 v2] dtc: fdtdump: check fdt if not in scanning mode

[David Gibson]

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

On Thu, Dec 22, 2016 at 12:59:06AM +0100, Heinrich Schuchardt wrote:
> Running fdtdump without scan mode for an invalid file often
> results in a segmentation fault because the fdt header is
> not checked.
> 
> With the patch the header is checked both in scanning as
> well as in non-scanning mode.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
> ---
> 
> v2:
> 	Refactor header check as separate function.
> 
>  fdtdump.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)

Applied, with a couple of tiny tweaks.

> 
> diff --git a/fdtdump.c b/fdtdump.c
> index a9a2484..4137689 100644
> --- a/fdtdump.c
> +++ b/fdtdump.c
> @@ -16,6 +16,7 @@
>  #include "util.h"
>  
>  #define FDT_MAGIC_SIZE	4
> +#define MAX_VERSION 17
>  
>  #define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
>  #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
> @@ -159,6 +160,20 @@ static const char * const usage_opts_help[] = {
>  	USAGE_COMMON_OPTS_HELP
>  };
>  
> +static inline int valid_header(char *p, off_t len)
> +{
> +	if (len < sizeof(struct fdt_header) ||
> +	    fdt_magic(p) != FDT_MAGIC ||
> +	    fdt_version(p) > MAX_VERSION ||
> +	    fdt_last_comp_version(p) >= MAX_VERSION ||
> +	    fdt_totalsize(p) >= len ||
> +	    fdt_off_dt_struct(p) >= len ||
> +	    fdt_off_dt_strings(p) >= len)
> +		return 0;
> +	else
> +		return 1;
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	int opt;
> @@ -204,12 +219,7 @@ int main(int argc, char *argv[])
>  			if (fdt_magic(p) == FDT_MAGIC) {
>  				/* try and validate the main struct */
>  				off_t this_len = endp - p;
> -				fdt32_t max_version = 17;
> -				if (fdt_version(p) <= max_version &&
> -				    fdt_last_comp_version(p) < max_version &&
> -				    fdt_totalsize(p) < this_len &&
> -				    fdt_off_dt_struct(p) < this_len &&
> -					fdt_off_dt_strings(p) < this_len)
> +				if (valid_header(p, this_len))
>  					break;
>  				if (debug)
>  					printf("%s: skipping fdt magic at offset %#zx\n",
> @@ -217,11 +227,12 @@ int main(int argc, char *argv[])
>  			}
>  			++p;
>  		}
> -		if (!p || ((endp - p) < FDT_MAGIC_SIZE))
> +		if (!p || endp - p < sizeof(struct fdt_header))
>  			die("%s: could not locate fdt magic\n", file);
>  		printf("%s: found fdt at offset %#zx\n", file, p - buf);
>  		buf = p;
> -	}
> +	} else if (!valid_header(buf, len))
> +		die("%s: header is not valid\n", file);
>  
>  	dump_blob(buf, debug);
>  

-- 
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: 819 bytes --]