[PATCH v4] libfdt: tests: add get_next_tag_invalid_prop_len

[PATCH v4] 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>
---
v4:
 * I didn't keep track of the changes in the test code,
   but this version should have all the comments addressed.
---
 tests/.gitignore                      |  1 +
 tests/Makefile.tests                  |  2 +-
 tests/get_next_tag_invalid_prop_len.c | 76 +++++++++++++++++++++++++++
 tests/meson.build                     |  1 +
 tests/run_tests.sh                    |  1 +
 5 files changed, 80 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..20c51de
--- /dev/null
+++ b/tests/get_next_tag_invalid_prop_len.c
@@ -0,0 +1,76 @@
+// 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) \
+	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;
+
+	test_init(argc, argv);
+	fdt = malloc(FDT_SIZE);
+	if (!fdt)
+		FAIL("Can't allocate memory");
+	err = fdt_create(fdt, FDT_SIZE);
+	CHECK_ERR(err);
+	fdt_set_off_dt_strings(fdt, FDT_SIZE);
+	err = fdt_begin_node(fdt, "");
+	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);
+	offset = fdt_first_property_offset(fdt, 0);
+	if (offset <= 0)
+		FAIL("FAIL Invalid offset %x, expected value greater than 0\n",
+		     offset);
+
+	/* Normal case */
+	tag = fdt_next_tag(fdt, offset, &nextoff);
+	if (tag != FDT_PROP )
+		FAIL("FAIL Invalid tag %x, expected FDT_PROP\n", tag);
+
+	/* Get a writable ptr to the first property and corrupt the lenght */
+	prp = fdt_get_property_w(fdt, 0, "prop-int-32", NULL);
+	if (!prp)
+		FAIL("Bad property pointer");
+
+	/* 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 FDT_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 FDT_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 v4] libfdt: tests: add get_next_tag_invalid_prop_len

[David Gibson]

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

On Thu, Oct 06, 2022 at 03:31:55PM -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>

Sorry, I was hoping I'd be able to apply this variant, but
unfortunately I realize I've given you some misleading advice in
earlier reviews, so there are still a few nits to squash, details
below.  Thanks for your patience.

> ---
> v4:
>  * I didn't keep track of the changes in the test code,
>    but this version should have all the comments addressed.
> ---
>  tests/.gitignore                      |  1 +
>  tests/Makefile.tests                  |  2 +-
>  tests/get_next_tag_invalid_prop_len.c | 76 +++++++++++++++++++++++++++
>  tests/meson.build                     |  1 +
>  tests/run_tests.sh                    |  1 +
>  5 files changed, 80 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..20c51de
> --- /dev/null
> +++ b/tests/get_next_tag_invalid_prop_len.c
> @@ -0,0 +1,76 @@
> +// 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) \
> +	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;
> +
> +	test_init(argc, argv);
> +	fdt = malloc(FDT_SIZE);
> +	if (!fdt)
> +		FAIL("Can't allocate memory");
> +	err = fdt_create(fdt, FDT_SIZE);
> +	CHECK_ERR(err);
> +	fdt_set_off_dt_strings(fdt, FDT_SIZE);

My comment about not needing to create the dummy reservemap entry was
misleading, sorry.  I was just referring to the actual dummy entry you
created with fdt_add_reservemap_entry.  You should still call
fdt_finish_reservemap() so that the blob is in the right state to call
fdt_begin_node().  Directly manipulating with fdt_set_off_dt_strings()
is unnecesarily fragile since it requires internal knowledge of how
the sw functions keep track of the state.

> +	err = fdt_begin_node(fdt, "");
> +	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);

One more minor deficiency here I missed earlier.  You're not calling
fdt_finish(), so the blob is in sw state.  The read-only libfdt
functions are designed to work on sw state trees as well as finished
trees, but there are some internal logic differences to handle this.

You're probably mostly concerned with the original fdt_next_tag() bug
for finished trees, so it's probably better to call fdt_finish() so
that's the case you're testing.  Alternatively, you could test both
variants.  Since you're corrupting the tree, you'll need to
reconstruct the test blob for each variant.  You could either make a
helper function taking a parameter and call it twice, or make the
whole test binary take a parameter and invoke it twice from
run_tests.sh.

> +	offset = fdt_first_property_offset(fdt, 0);
> +	if (offset <= 0)
> +		FAIL("FAIL Invalid offset %x, expected value greater than 0\n",
> +		     offset);
> +
> +	/* Normal case */
> +	tag = fdt_next_tag(fdt, offset, &nextoff);
> +	if (tag != FDT_PROP )
> +		FAIL("FAIL Invalid tag %x, expected FDT_PROP\n", tag);
> +
> +	/* Get a writable ptr to the first property and corrupt the lenght */
> +	prp = fdt_get_property_w(fdt, 0, "prop-int-32", NULL);
> +	if (!prp)
> +		FAIL("Bad property pointer");

My comment about using fdt_get_property_w() was also a bit misleading,
since I wasn't thinking about the fact that you need both the offset
(for fdt_next_tag()) and the direct pointer to the property struct.

This code is relying on the offset from fdt_first_property_offset()
and the pointer from fdt_get_property_w() referring to the same
location in the blob.  They will be, but it would be better to have
that be obvious by construction.

I'd suggest you first get the offset with fdt_first_property_offset(),
then compute the prp pointer from that with
fdt_get_property_by_offset().  You'll need a cast to remove the const
from the latter in order to mangle the tree, of course.  If you wanted
to add a new fdt_get_property_by_offset_w() wrapper to do that cast,
that would also be fine (if you do, make it a separate patch please).
There's no particular rationale to which functions have _w() variants
and which don't (so far), I just made the _w() variants when I needed
them for other functions internally.

> +
> +	/* 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 FDT_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 FDT_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 v4] libfdt: tests: add get_next_tag_invalid_prop_len

[Tadeusz Struk]

On 10/6/22 21:11, David Gibson wrote:
> On Thu, Oct 06, 2022 at 03:31:55PM -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>
> Sorry, I was hoping I'd be able to apply this variant, but
> unfortunately I realize I've given you some misleading advice in
> earlier reviews, so there are still a few nits to squash, details
> below.  Thanks for your patience.
> 
>> ---
>> v4:
>>   * I didn't keep track of the changes in the test code,
>>     but this version should have all the comments addressed.
>> ---
>>   tests/.gitignore                      |  1 +
>>   tests/Makefile.tests                  |  2 +-
>>   tests/get_next_tag_invalid_prop_len.c | 76 +++++++++++++++++++++++++++
>>   tests/meson.build                     |  1 +
>>   tests/run_tests.sh                    |  1 +
>>   5 files changed, 80 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..20c51de
>> --- /dev/null
>> +++ b/tests/get_next_tag_invalid_prop_len.c
>> @@ -0,0 +1,76 @@
>> +// 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) \
>> +	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;
>> +
>> +	test_init(argc, argv);
>> +	fdt = malloc(FDT_SIZE);
>> +	if (!fdt)
>> +		FAIL("Can't allocate memory");
>> +	err = fdt_create(fdt, FDT_SIZE);
>> +	CHECK_ERR(err);
>> +	fdt_set_off_dt_strings(fdt, FDT_SIZE);
> My comment about not needing to create the dummy reservemap entry was
> misleading, sorry.  I was just referring to the actual dummy entry you
> created with fdt_add_reservemap_entry.  You should still call
> fdt_finish_reservemap() so that the blob is in the right state to call
> fdt_begin_node().  Directly manipulating with fdt_set_off_dt_strings()
> is unnecesarily fragile since it requires internal knowledge of how
> the sw functions keep track of the state.
> 
>> +	err = fdt_begin_node(fdt, "");
>> +	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);
> One more minor deficiency here I missed earlier.  You're not calling
> fdt_finish(), so the blob is in sw state.  The read-only libfdt
> functions are designed to work on sw state trees as well as finished
> trees, but there are some internal logic differences to handle this.
> 
> You're probably mostly concerned with the original fdt_next_tag() bug
> for finished trees, so it's probably better to call fdt_finish() so
> that's the case you're testing.  Alternatively, you could test both
> variants.  Since you're corrupting the tree, you'll need to
> reconstruct the test blob for each variant.  You could either make a
> helper function taking a parameter and call it twice, or make the
> whole test binary take a parameter and invoke it twice from
> run_tests.sh.
> 
>> +	offset = fdt_first_property_offset(fdt, 0);
>> +	if (offset <= 0)
>> +		FAIL("FAIL Invalid offset %x, expected value greater than 0\n",
>> +		     offset);
>> +
>> +	/* Normal case */
>> +	tag = fdt_next_tag(fdt, offset, &nextoff);
>> +	if (tag != FDT_PROP )
>> +		FAIL("FAIL Invalid tag %x, expected FDT_PROP\n", tag);
>> +
>> +	/* Get a writable ptr to the first property and corrupt the lenght */
>> +	prp = fdt_get_property_w(fdt, 0, "prop-int-32", NULL);
>> +	if (!prp)
>> +		FAIL("Bad property pointer");
> My comment about using fdt_get_property_w() was also a bit misleading,
> since I wasn't thinking about the fact that you need both the offset
> (for fdt_next_tag()) and the direct pointer to the property struct.
> 
> This code is relying on the offset from fdt_first_property_offset()
> and the pointer from fdt_get_property_w() referring to the same
> location in the blob.  They will be, but it would be better to have
> that be obvious by construction.
> 
> I'd suggest you first get the offset with fdt_first_property_offset(),
> then compute the prp pointer from that with
> fdt_get_property_by_offset().  You'll need a cast to remove the const
> from the latter in order to mangle the tree, of course.  If you wanted
> to add a new fdt_get_property_by_offset_w() wrapper to do that cast,
> that would also be fine (if you do, make it a separate patch please).
> There's no particular rationale to which functions have _w() variants
> and which don't (so far), I just made the _w() variants when I needed
> them for other functions internally.

I have added a new helper and used it to get the pointer at the same
offset. I also addressed your comments above. New version on its way.
There will be 2 new patches, first with the helper, and second with
the updated test (v5), and they supposed to be applied on the v3 1/1
I sent before. Thanks for your feedback.

-- 
Thanks,
Tadeusz