Skip to contentSkip to conversation

Catch unsigned 32bit overflow when parsing flattened device tree offsets

PATCH In review Anton Blanchard 2 participants 3 messages Jan 2, 2016 – Feb 18, 2016 1 message not in the archive

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Catch unsigned 32bit overflow when parsing flattened device tree offsets

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.

---

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

flattree.c +2 -2
index bd99fa2..ec14954 100644
@@ -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");
 	}
 
#1 lore raw
Quoted 8 lines · Anton Blanchard

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.

Quoted 7 lines

Found with AFL.

---

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.

Quoted 22 lines
flattree.c +2 -2
index bd99fa2..ec14954 100644
@@ -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");
 	}
 
Signature · 4 lines
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

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw
Quoted 21 lines · David Gibson
Quoted 8 lines · Anton Blanchard

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.

Quoted 7 lines

Found with AFL.

---

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.

Quoted 5 lines

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.

Signature · 4 lines
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