[PATCH] Catch unsigned 32bit overflow when parsing flattened device tree offsets

[PATCH] Catch unsigned 32bit overflow when parsing flattened device tree offsets

[Anton Blanchard]

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

We have a couple of checks of the form:

    if (offset+size > totalsize)
        die();

We need to check that offset+size doesn't overflow, otherwise the check
will pass, and we may access past totalsize.

Found with AFL.

Signed-off-by: Anton Blanchard <anton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---

I've attached an example device tree, do we want to add binary blobs
to the test suite?

diff --git a/flattree.c b/flattree.c
index bd99fa2..ec14954 100644
--- a/flattree.c
+++ b/flattree.c
@@ -889,7 +889,7 @@ struct boot_info *dt_from_blob(const char *fname)
 
 	if (version >= 3) {
 		uint32_t size_str = fdt32_to_cpu(fdt->size_dt_strings);
-		if (off_str+size_str > totalsize)
+		if ((off_str+size_str < off_str) || (off_str+size_str > totalsize))
 			die("String table extends past total size\n");
 		inbuf_init(&strbuf, blob + off_str, blob + off_str + size_str);
 	} else {
@@ -898,7 +898,7 @@ struct boot_info *dt_from_blob(const char *fname)
 
 	if (version >= 17) {
 		size_dt = fdt32_to_cpu(fdt->size_dt_struct);
-		if (off_dt+size_dt > totalsize)
+		if ((off_dt+size_dt < off_dt) || (off_dt+size_dt > totalsize))
 			die("Structure block extends past total size\n");
 	}
 

[-- Attachment #2: t.dtb --]
[-- Type: application/octet-stream, Size: 90 bytes --]

Re: [PATCH] Catch unsigned 32bit overflow when parsing flattened device tree offsets

[David Gibson]

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

On Sun, Jan 03, 2016 at 08:43:35AM +1100, Anton Blanchard wrote:
> We have a couple of checks of the form:
> 
>     if (offset+size > totalsize)
>         die();
> 
> We need to check that offset+size doesn't overflow, otherwise the check
> will pass, and we may access past totalsize.
> 
> Found with AFL.
> 
> Signed-off-by: Anton Blanchard <anton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> ---
> 
> I've attached an example device tree, do we want to add binary blobs
> to the test suite?

I've generally avoided it, but I forget exactly why.  Usually I try to
generate the testcases as dts and compile them, but I'm guessing this dtb is
something that shouldn't be possible as good output from dtc.

It would be possible to construct it from test/trees.S, but just
including the binary blob might be simpler.

Certainly I would like to include this testcase into the testsuite,
one way or another.

> diff --git a/flattree.c b/flattree.c
> index bd99fa2..ec14954 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -889,7 +889,7 @@ struct boot_info *dt_from_blob(const char *fname)
>  
>  	if (version >= 3) {
>  		uint32_t size_str = fdt32_to_cpu(fdt->size_dt_strings);
> -		if (off_str+size_str > totalsize)
> +		if ((off_str+size_str < off_str) || (off_str+size_str > totalsize))
>  			die("String table extends past total size\n");
>  		inbuf_init(&strbuf, blob + off_str, blob + off_str + size_str);
>  	} else {
> @@ -898,7 +898,7 @@ struct boot_info *dt_from_blob(const char *fname)
>  
>  	if (version >= 17) {
>  		size_dt = fdt32_to_cpu(fdt->size_dt_struct);
> -		if (off_dt+size_dt > totalsize)
> +		if ((off_dt+size_dt < off_dt) || (off_dt+size_dt > totalsize))
>  			die("Structure block extends past total size\n");
>  	}
>  



-- 
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] Catch unsigned 32bit overflow when parsing flattened device tree offsets

[David Gibson]

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

On Sun, Jan 03, 2016 at 08:32:49PM +1100, David Gibson wrote:
> On Sun, Jan 03, 2016 at 08:43:35AM +1100, Anton Blanchard wrote:
> > We have a couple of checks of the form:
> > 
> >     if (offset+size > totalsize)
> >         die();
> > 
> > We need to check that offset+size doesn't overflow, otherwise the check
> > will pass, and we may access past totalsize.
> > 
> > Found with AFL.
> > 
> > Signed-off-by: Anton Blanchard <anton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> > ---
> > 
> > I've attached an example device tree, do we want to add binary blobs
> > to the test suite?
> 
> I've generally avoided it, but I forget exactly why.  Usually I try to
> generate the testcases as dts and compile them, but I'm guessing this dtb is
> something that shouldn't be possible as good output from dtc.
> 
> It would be possible to construct it from test/trees.S, but just
> including the binary blob might be simpler.
> 
> Certainly I would like to include this testcase into the testsuite,
> one way or another.

I finally sorted this out and added this fix, plus a testcase to the
tree.

Btw, do you have the scripts you used to run AFL on dtc?  I'd love to
try it myself.

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