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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Fri, Sep 30, 2022 at 08:20:04AM -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@linaro.org>
> ---
>  tests/.gitignore                      |  1 +
>  tests/Makefile.tests                  |  2 +-
>  tests/get_next_tag_invalid_prop_len.c | 65 +++++++++++++++++++++++++++
>  tests/meson.build                     |  1 +
>  tests/run_tests.sh                    |  1 +
>  5 files changed, 69 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..c02f6a3
> --- /dev/null
> +++ b/tests/get_next_tag_invalid_prop_len.c
> @@ -0,0 +1,65 @@
> +// 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"
> +
> +int main(int argc, char *argv[])
> +{
> +	struct fdt_header *hdr;
> +	struct fdt_property *prp;
> +	void *fdt;
> +	int size, nextoff = 0;
> +	uint32_t tag;
> +
> +	test_init(argc, argv);
> +	size = sizeof(*hdr) + sizeof(*prp) + 256;
> +	fdt = calloc(1, size);
> +	if (!fdt)
> +		FAIL("Can't allocate memory");
> +
> +	hdr = fdt;
> +	prp = (struct fdt_property *)(((char *) fdt) + sizeof(*hdr));
> +	fdt_set_magic(fdt, FDT_MAGIC);
> +	fdt_set_totalsize(fdt, size);
> +	fdt_set_version(fdt, 0x10);
> +	prp->tag = cpu_to_fdt32(FDT_PROP);
> +	prp->len = cpu_to_fdt32(256);
> +	prp->nameoff = 0;

The dtb you're constructing here isn't a valid dtb, even before you
corrupt the property lengths: it doesn't have valid offsets to the
blocks, and you have no BEGIN_NODE tag for the root node.  That means
that in order to test the specific thing you want to test, you're
relying on *very* detailed knowledge of exactly how the code under
test works and what it does and doesn't check, which makes the test
unnecessarily fragile.

I'd really suggest building the initial tree with the fdt_sw functions
- or even more declaratively in trees.S - before corrupting it to test
the actual overflow condition.

> +	tag = fdt_next_tag(fdt, sizeof(*hdr), &nextoff);
> +	if (tag != FDT_PROP)
> +		FAIL("Invalid tag %X", tag);
> +
> +	if (nextoff != size)
> +		FAIL("Invalid next_offset");
> +
> +	/* int overflow case */
> +	prp->len = cpu_to_fdt32(0xFFFFFFFA);
> +	tag = fdt_next_tag(fdt, sizeof(*hdr), &nextoff);
> +	if (tag != FDT_END)
> +		FAIL("Invalid tag, expected premature end");
> +
> +	if (nextoff != -FDT_ERR_BADSTRUCTURE)
> +		FAIL("Invalid nextoff, expected error FDT_ERR_BADSTRUCTURE");
> +
> +	/* negative offset case */

Is there actually any meaningful difference between the "int overflow"
and "negative offset" cases?

> +	prp->len = cpu_to_fdt32(0x7FFFFFFA);
> +	tag = fdt_next_tag(fdt, sizeof(*hdr), &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..397b9cf 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -346,6 +346,7 @@ tree1_tests () {
>      run_test get_prop_offset $TREE
>      run_test get_phandle $TREE
>      run_test get_path $TREE
> +    run_test get_next_tag_invalid_prop_len $TREE #TREE not really needed

This doesn't belong with tree1_tests() since it has nothing to do with
test_tree1.  It should go under the "Specific bug tests" comment
instead.  Also, since you're not using a passed in tree blob, you
should modify the test code not to expect one.

>      run_test supernode_atdepth_offset $TREE
>      run_test parent_offset $TREE
>      run_test node_offset_by_prop_value $TREE

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