[PATCH v3 1/2] libfdt: prevent integer overflow in fdt_next_tag

[PATCH v3 1/2] libfdt: prevent integer overflow in fdt_next_tag

[Tadeusz Struk]

Since fdt_next_tag() in a public API function all input parameters,
including the fdt blob should not be trusted. It is possible to forge
a blob with invalid property length that will cause integer overflow
during offset calculation. To prevent that, validate the property length
read from the blob before doing calculations.

Signed-off-by: Tadeusz Struk <tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
v2:
* Use len local variable to avoid multiple calls to fdt32_to_cpu(*lenp)
* Add can_assume(VALID_DTB) to the new checks
v3:
* Use unsigned integer for prop len and offset validation
---
 libfdt/fdt.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 90a39e8..20c6415 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -162,7 +162,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 {
 	const fdt32_t *tagp, *lenp;
-	uint32_t tag;
+	uint32_t tag, len, sum;
 	int offset = startoffset;
 	const char *p;
 
@@ -188,12 +188,19 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
 		if (!can_assume(VALID_DTB) && !lenp)
 			return FDT_END; /* premature end */
+
+		len = fdt32_to_cpu(*lenp);
+		sum = len + offset;
+		if (!can_assume(VALID_DTB) &&
+		    (INT_MAX <= sum || sum < (uint32_t) offset))
+			return FDT_END; /* premature end */
+
 		/* skip-name offset, length and value */
-		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
-			+ fdt32_to_cpu(*lenp);
+		offset += sizeof(struct fdt_property) - FDT_TAGSIZE + len;
+
 		if (!can_assume(LATEST) &&
-		    fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
-		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
+		    fdt_version(fdt) < 0x10 && len >= 8 &&
+		    ((offset - len) % 8) != 0)
 			offset += 4;
 		break;
 
-- 
2.37.3

[PATCH v3 2/2] libfdt: tests: add get_next_tag_invalid_prop_len

[Tadeusz Struk]

Add a new test get_next_tag_invalid_prop_len, which covers
fdt_next_tag(), when it is passed an corrupted blob, with
invalid property len values.

Signed-off-by: Tadeusz Struk <tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 tests/.gitignore                      |  1 +
 tests/Makefile.tests                  |  2 +-
 tests/get_next_tag_invalid_prop_len.c | 84 +++++++++++++++++++++++++++
 tests/meson.build                     |  1 +
 tests/run_tests.sh                    |  1 +
 5 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 tests/get_next_tag_invalid_prop_len.c

diff --git a/tests/.gitignore b/tests/.gitignore
index 03bdde2..3376ed9 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -74,3 +74,4 @@ tmp.*
 /truncated_memrsv
 /utilfdt_test
 /value-labels
+/get_next_tag_invalid_prop_len
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index 2d36c5d..2c5b4c9 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -4,7 +4,7 @@ LIB_TESTS_L = get_mem_rsv \
 	get_path supernode_atdepth_offset parent_offset \
 	node_offset_by_prop_value node_offset_by_phandle \
 	node_check_compatible node_offset_by_compatible \
-	get_alias \
+	get_alias get_next_tag_invalid_prop_len \
 	char_literal \
 	sized_cells \
 	notfound \
diff --git a/tests/get_next_tag_invalid_prop_len.c b/tests/get_next_tag_invalid_prop_len.c
new file mode 100644
index 0000000..f5a6d99
--- /dev/null
+++ b/tests/get_next_tag_invalid_prop_len.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for fdt_next_tag()
+ */
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include <libfdt.h>
+#include "tests.h"
+#include "testdata.h"
+
+#define FDT_SIZE 65536
+#define CHECK_ERR(err) \
+({ if (err) { \
+	free(fdt); \
+	FAIL("%s: %d: %s", __FILE__, __LINE__, fdt_strerror(err)); \
+	} \
+})
+
+int main(int argc, char *argv[])
+{
+	struct fdt_property *prp;
+	void *fdt;
+	int nextoff = 0, offset, err;
+	uint32_t tag, val;
+
+	test_init(argc, argv);
+	fdt = calloc(1, FDT_SIZE);
+	if (!fdt)
+		FAIL("Can't allocate memory");
+	err = fdt_create(fdt, FDT_SIZE);
+	CHECK_ERR(err);
+	err = fdt_add_reservemap_entry(fdt, 0xdeadbeefUL, 0x10000UL);
+	CHECK_ERR(err);
+	err = fdt_finish_reservemap(fdt);
+	CHECK_ERR(err);
+	err = fdt_begin_node(fdt, "");
+	CHECK_ERR(err);
+	err = fdt_begin_node(fdt, "subnode1");
+	CHECK_ERR(err);
+	err = fdt_property_u32(fdt, "prop-int-32", 0x1234);
+	CHECK_ERR(err);
+	err = fdt_property_u32(fdt, "prop2-int-32", 0x4321);
+	CHECK_ERR(err);
+	err = fdt_end_node(fdt);
+	CHECK_ERR(err);
+	err = fdt_end_node(fdt);
+	CHECK_ERR(err);
+	offset = -1;
+	val = cpu_to_fdt32(0x1234);
+	offset = fdt_node_offset_by_prop_value(fdt, offset,  "prop-int-32",
+					       &val, sizeof(val));
+	do {
+		tag = fdt_next_tag(fdt, offset, &nextoff);
+		offset = nextoff;
+	} while (tag != FDT_PROP);
+
+	/* Calculate len to property */
+	prp = (struct fdt_property *)(((char*)fdt) + fdt_off_dt_struct(fdt) + offset);
+
+	/* int overflow case */
+	prp->len = cpu_to_fdt32(0xFFFFFFFA);
+	tag = fdt_next_tag(fdt, offset, &nextoff);
+	if (tag != FDT_END)
+		FAIL("Invalid tag %x, expected premature end", tag);
+
+	if (nextoff != -FDT_ERR_BADSTRUCTURE)
+		FAIL("Invalid nextoff, expected error -FDT_ERR_BADSTRUCTURE");
+
+	/* negative offset case */
+	prp->len = cpu_to_fdt32(0x7FFFFFFA);
+	tag = fdt_next_tag(fdt, offset, &nextoff);
+	if (tag != FDT_END)
+		FAIL("Invalid tag, expected premature end");
+
+	if (nextoff != -FDT_ERR_BADSTRUCTURE)
+		FAIL("Invalid nextoff, expected error -FDT_ERR_BADSTRUCTURE");
+
+	free(fdt);
+	PASS();
+}
diff --git a/tests/meson.build b/tests/meson.build
index 4ac154a..29a42dd 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -47,6 +47,7 @@ tests = [
   'get_path',
   'get_phandle',
   'get_prop_offset',
+  'get_next_tag_invalid_prop_len',
   'getprop',
   'incbin',
   'integer-expressions',
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 244df8a..46678cb 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -513,6 +513,7 @@ libfdt_tests () {
     run_dtc_test -I fs -O dts -o fs.test_tree1.test.dts $FSBASE/test_tree1
     run_dtc_test -I fs -O dtb -o fs.test_tree1.test.dtb $FSBASE/test_tree1
     run_test dtbs_equal_unordered -m fs.test_tree1.test.dtb test_tree1.dtb
+    run_test get_next_tag_invalid_prop_len
 
     ## https://github.com/dgibson/dtc/issues/64
     check_tests "$SRCDIR/phandle-args-overflow.dts" clocks_property
-- 
2.37.3

Re: [PATCH v3 2/2] libfdt: tests: add get_next_tag_invalid_prop_len

[David Gibson]

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

On Wed, Oct 05, 2022 at 04:29:31PM -0700, Tadeusz Struk wrote:
> Add a new test get_next_tag_invalid_prop_len, which covers
> fdt_next_tag(), when it is passed an corrupted blob, with
> invalid property len values.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Looks good overall, but a bunch of minor things to polish.

> ---
>  tests/.gitignore                      |  1 +
>  tests/Makefile.tests                  |  2 +-
>  tests/get_next_tag_invalid_prop_len.c | 84 +++++++++++++++++++++++++++
>  tests/meson.build                     |  1 +
>  tests/run_tests.sh                    |  1 +
>  5 files changed, 88 insertions(+), 1 deletion(-)
>  create mode 100644 tests/get_next_tag_invalid_prop_len.c
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 03bdde2..3376ed9 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -74,3 +74,4 @@ tmp.*
>  /truncated_memrsv
>  /utilfdt_test
>  /value-labels
> +/get_next_tag_invalid_prop_len
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index 2d36c5d..2c5b4c9 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -4,7 +4,7 @@ LIB_TESTS_L = get_mem_rsv \
>  	get_path supernode_atdepth_offset parent_offset \
>  	node_offset_by_prop_value node_offset_by_phandle \
>  	node_check_compatible node_offset_by_compatible \
> -	get_alias \
> +	get_alias get_next_tag_invalid_prop_len \
>  	char_literal \
>  	sized_cells \
>  	notfound \
> diff --git a/tests/get_next_tag_invalid_prop_len.c b/tests/get_next_tag_invalid_prop_len.c
> new file mode 100644
> index 0000000..f5a6d99
> --- /dev/null
> +++ b/tests/get_next_tag_invalid_prop_len.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: LGPL-2.1-or-later
> +/*
> + * libfdt - Flat Device Tree manipulation
> + *	Testcase for fdt_next_tag()
> + */
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +
> +#include <libfdt.h>
> +#include "tests.h"
> +#include "testdata.h"
> +
> +#define FDT_SIZE 65536
> +#define CHECK_ERR(err) \
> +({ if (err) { \
> +	free(fdt); \

You don't need the free() here, you're about to quit the test program
anyway.

> +	FAIL("%s: %d: %s", __FILE__, __LINE__, fdt_strerror(err)); \
> +	} \
> +})
> +
> +int main(int argc, char *argv[])
> +{
> +	struct fdt_property *prp;
> +	void *fdt;
> +	int nextoff = 0, offset, err;
> +	uint32_t tag, val;
> +
> +	test_init(argc, argv);
> +	fdt = calloc(1, FDT_SIZE);

No need to use cleared memory, the fdt_sw functions will work just
fine with an uninitialized buffer.

> +	if (!fdt)
> +		FAIL("Can't allocate memory");
> +	err = fdt_create(fdt, FDT_SIZE);
> +	CHECK_ERR(err);
> +	err = fdt_add_reservemap_entry(fdt, 0xdeadbeefUL, 0x10000UL);

No need to insert a dummy reservemap entry here.

> +	CHECK_ERR(err);
> +	err = fdt_finish_reservemap(fdt);
> +	CHECK_ERR(err);
> +	err = fdt_begin_node(fdt, "");
> +	CHECK_ERR(err);
> +	err = fdt_begin_node(fdt, "subnode1");
> +	CHECK_ERR(err);

No particular need for this subnode either, you can test what you want
with properties on the root node.

> +	err = fdt_property_u32(fdt, "prop-int-32", 0x1234);
> +	CHECK_ERR(err);
> +	err = fdt_property_u32(fdt, "prop2-int-32", 0x4321);
> +	CHECK_ERR(err);
> +	err = fdt_end_node(fdt);
> +	CHECK_ERR(err);
> +	err = fdt_end_node(fdt);
> +	CHECK_ERR(err);
> +	offset = -1;
> +	val = cpu_to_fdt32(0x1234);
> +	offset = fdt_node_offset_by_prop_value(fdt, offset,  "prop-int-32",
> +					       &val, sizeof(val));

fdt_node_offset_by_prop_value() is a very roundabout way to find the
node you need - you know the path (and if you get rid of the
unnecessary subnode, it'll just be the root node at offset 0).

> +	do {
> +		tag = fdt_next_tag(fdt, offset, &nextoff);
> +		offset = nextoff;
> +	} while (tag != FDT_PROP);
> +
> +	/* Calculate len to property */
> +	prp = (struct fdt_property *)(((char*)fdt) + fdt_off_dt_struct(fdt) + offset);

You could replace the loop as well as this nasty pointer arithmetic
with an fdt_get_property_w() call.

> +	/* int overflow case */

Probably worth testing the fdt_next_tag() behaviour on the unmangled
tree before testing the corrupted cases.  If the test ever breaks,
that sort of thing makes it easier to figure out if the breakage is in
the library, or the testcase.

> +	prp->len = cpu_to_fdt32(0xFFFFFFFA);
> +	tag = fdt_next_tag(fdt, offset, &nextoff);
> +	if (tag != FDT_END)
> +		FAIL("Invalid tag %x, expected premature end", tag);
> +
> +	if (nextoff != -FDT_ERR_BADSTRUCTURE)
> +		FAIL("Invalid nextoff, expected error -FDT_ERR_BADSTRUCTURE");
> +
> +	/* negative offset case */
> +	prp->len = cpu_to_fdt32(0x7FFFFFFA);
> +	tag = fdt_next_tag(fdt, offset, &nextoff);
> +	if (tag != FDT_END)
> +		FAIL("Invalid tag, expected premature end");
> +
> +	if (nextoff != -FDT_ERR_BADSTRUCTURE)
> +		FAIL("Invalid nextoff, expected error -FDT_ERR_BADSTRUCTURE");
> +
> +	free(fdt);
> +	PASS();
> +}
> diff --git a/tests/meson.build b/tests/meson.build
<> index 4ac154a..29a42dd 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -47,6 +47,7 @@ tests = [
>    'get_path',
>    'get_phandle',
>    'get_prop_offset',
> +  'get_next_tag_invalid_prop_len',
>    'getprop',
>    'incbin',
>    'integer-expressions',
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 244df8a..46678cb 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -513,6 +513,7 @@ libfdt_tests () {
>      run_dtc_test -I fs -O dts -o fs.test_tree1.test.dts $FSBASE/test_tree1
>      run_dtc_test -I fs -O dtb -o fs.test_tree1.test.dtb $FSBASE/test_tree1
>      run_test dtbs_equal_unordered -m fs.test_tree1.test.dtb test_tree1.dtb
> +    run_test get_next_tag_invalid_prop_len
>  
>      ## https://github.com/dgibson/dtc/issues/64
>      check_tests "$SRCDIR/phandle-args-overflow.dts" clocks_property

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

Re: [PATCH v3 2/2] libfdt: tests: add get_next_tag_invalid_prop_len

[Tadeusz Struk]

On 10/6/22 01:06, David Gibson wrote:
> On Wed, Oct 05, 2022 at 04:29:31PM -0700, Tadeusz Struk wrote:
>> Add a new test get_next_tag_invalid_prop_len, which covers
>> fdt_next_tag(), when it is passed an corrupted blob, with
>> invalid property len values.
>>
>> Signed-off-by: Tadeusz Struk<tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Looks good overall, but a bunch of minor things to polish.

Thanks for reviewing this. I will only follow with the new
version if 2/2. Please take the v3 1/2 as it is.

> 
>> +#define FDT_SIZE 65536
>> +#define CHECK_ERR(err) \
>> +({ if (err) { \
>> +	free(fdt); \
> You don't need the free() here, you're about to quit the test program
> anyway.

Right. I will take that out.

> 
>> +	FAIL("%s: %d: %s", __FILE__, __LINE__, fdt_strerror(err)); \
>> +	} \
>> +})
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	struct fdt_property *prp;
>> +	void *fdt;
>> +	int nextoff = 0, offset, err;
>> +	uint32_t tag, val;
>> +
>> +	test_init(argc, argv);
>> +	fdt = calloc(1, FDT_SIZE);
> No need to use cleared memory, the fdt_sw functions will work just
> fine with an uninitialized buffer.

Ok

> 
>> +	if (!fdt)
>> +		FAIL("Can't allocate memory");
>> +	err = fdt_create(fdt, FDT_SIZE);
>> +	CHECK_ERR(err);
>> +	err = fdt_add_reservemap_entry(fdt, 0xdeadbeefUL, 0x10000UL);> No need to insert a dummy reservemap entry here.

Ok, removed

> 
>> +	CHECK_ERR(err);
>> +	err = fdt_finish_reservemap(fdt);
>> +	CHECK_ERR(err);
>> +	err = fdt_begin_node(fdt, "");
>> +	CHECK_ERR(err);
>> +	err = fdt_begin_node(fdt, "subnode1");
>> +	CHECK_ERR(err);
> No particular need for this subnode either, you can test what you want
> with properties on the root node.

Removed the extra subnode.

> 
>> +	err = fdt_property_u32(fdt, "prop-int-32", 0x1234);
>> +	CHECK_ERR(err);
>> +	err = fdt_property_u32(fdt, "prop2-int-32", 0x4321);
>> +	CHECK_ERR(err);
>> +	err = fdt_end_node(fdt);
>> +	CHECK_ERR(err);
>> +	err = fdt_end_node(fdt);
>> +	CHECK_ERR(err);
>> +	offset = -1;
>> +	val = cpu_to_fdt32(0x1234);
>> +	offset = fdt_node_offset_by_prop_value(fdt, offset,  "prop-int-32",
>> +					       &val, sizeof(val));
> fdt_node_offset_by_prop_value() is a very roundabout way to find the
> node you need - you know the path (and if you get rid of the
> unnecessary subnode, it'll just be the root node at offset 0).

I removed that as well.

> 
>> +	do {
>> +		tag = fdt_next_tag(fdt, offset, &nextoff);
>> +		offset = nextoff;
>> +	} while (tag != FDT_PROP);
>> +
>> +	/* Calculate len to property */
>> +	prp = (struct fdt_property *)(((char*)fdt) + fdt_off_dt_struct(fdt) + offset);
> You could replace the loop as well as this nasty pointer arithmetic
> with an fdt_get_property_w() call.

The fdt_get_property_w() was what I was looking for, thanks, but even using it
I'm getting a different pointer within the fdt to what I'm getting when I
calculate the offset myself. I the tag value that it is pointing to is the
FDT_BEGIN_NODE. Do I need to do anything else after with the pointer returned
from fdt_get_property_w()?

> 
>> +	/* int overflow case */
> Probably worth testing the fdt_next_tag() behaviour on the unmangled
> tree before testing the corrupted cases.  If the test ever breaks,
> that sort of thing makes it easier to figure out if the breakage is in
> the library, or the testcase.
  
Will do.

-- 
Thanks,
Tadeusz

Re: [PATCH v3 1/2] libfdt: prevent integer overflow in fdt_next_tag

[David Gibson]

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

On Wed, Oct 05, 2022 at 04:29:30PM -0700, Tadeusz Struk wrote:
> Since fdt_next_tag() in a public API function all input parameters,
> including the fdt blob should not be trusted. It is possible to forge
> a blob with invalid property length that will cause integer overflow
> during offset calculation. To prevent that, validate the property length
> read from the blob before doing calculations.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

> ---
> v2:
> * Use len local variable to avoid multiple calls to fdt32_to_cpu(*lenp)
> * Add can_assume(VALID_DTB) to the new checks
> v3:
> * Use unsigned integer for prop len and offset validation
> ---
>  libfdt/fdt.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 90a39e8..20c6415 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -162,7 +162,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  {
>  	const fdt32_t *tagp, *lenp;
> -	uint32_t tag;
> +	uint32_t tag, len, sum;
>  	int offset = startoffset;
>  	const char *p;
>  
> @@ -188,12 +188,19 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
>  		if (!can_assume(VALID_DTB) && !lenp)
>  			return FDT_END; /* premature end */
> +
> +		len = fdt32_to_cpu(*lenp);
> +		sum = len + offset;
> +		if (!can_assume(VALID_DTB) &&
> +		    (INT_MAX <= sum || sum < (uint32_t) offset))
> +			return FDT_END; /* premature end */
> +
>  		/* skip-name offset, length and value */
> -		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> -			+ fdt32_to_cpu(*lenp);
> +		offset += sizeof(struct fdt_property) - FDT_TAGSIZE + len;
> +
>  		if (!can_assume(LATEST) &&
> -		    fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
> -		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
> +		    fdt_version(fdt) < 0x10 && len >= 8 &&
> +		    ((offset - len) % 8) != 0)
>  			offset += 4;
>  		break;
>  

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

Re: [PATCH v3 1/2] libfdt: prevent integer overflow in fdt_next_tag

[David Gibson]

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

On Wed, Oct 05, 2022 at 04:29:30PM -0700, Tadeusz Struk wrote:
> Since fdt_next_tag() in a public API function all input parameters,
> including the fdt blob should not be trusted. It is possible to forge
> a blob with invalid property length that will cause integer overflow
> during offset calculation. To prevent that, validate the property length
> read from the blob before doing calculations.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Merged, thanks.

> ---
> v2:
> * Use len local variable to avoid multiple calls to fdt32_to_cpu(*lenp)
> * Add can_assume(VALID_DTB) to the new checks
> v3:
> * Use unsigned integer for prop len and offset validation
> ---
>  libfdt/fdt.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 90a39e8..20c6415 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -162,7 +162,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  {
>  	const fdt32_t *tagp, *lenp;
> -	uint32_t tag;
> +	uint32_t tag, len, sum;
>  	int offset = startoffset;
>  	const char *p;
>  
> @@ -188,12 +188,19 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
>  		if (!can_assume(VALID_DTB) && !lenp)
>  			return FDT_END; /* premature end */
> +
> +		len = fdt32_to_cpu(*lenp);
> +		sum = len + offset;
> +		if (!can_assume(VALID_DTB) &&
> +		    (INT_MAX <= sum || sum < (uint32_t) offset))
> +			return FDT_END; /* premature end */
> +
>  		/* skip-name offset, length and value */
> -		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> -			+ fdt32_to_cpu(*lenp);
> +		offset += sizeof(struct fdt_property) - FDT_TAGSIZE + len;
> +
>  		if (!can_assume(LATEST) &&
> -		    fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
> -		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
> +		    fdt_version(fdt) < 0x10 && len >= 8 &&
> +		    ((offset - len) % 8) != 0)
>  			offset += 4;
>  		break;
>  

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