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

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- 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 --]