On 26/3/18 10:25 am, David Gibson wrote:
> Currently fdt_check_header() performs only some rudimentary checks, which
> is not really what the name suggests. This strengthens fdt_check_header()
> to check as much about the blob as is possible from the header alone: as
> well as checking the magic number and version, it checks that the total
> size is sane, and that all the sub-blocks within the blob lie within the
> total size.
>
> * This broadens the meaning of FDT_ERR_TRUNCATED to cover all sorts of
> improperly terminated blocks as well as just a structure block without
> FDT_END.
>
> * This makes fdt_check_header() only succeed on "complete" blobs, not
> in-progress sequential write blobs. The only reason this didn't fail
> before was that this function used to be called by many RO functions
> which are supposed to also work on incomplete SW blobs.
>
> Signed-off-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> ---
> libfdt/fdt.c | 57 ++++++++++++++++++++++-
> libfdt/libfdt.h | 5 +-
> libfdt/libfdt_env.h | 1 +
> tests/.gitignore | 1 +
> tests/Makefile.tests | 2 +-
> tests/check_header.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++
> tests/run_tests.sh | 3 ++
> 7 files changed, 193 insertions(+), 4 deletions(-)
> create mode 100644 tests/check_header.c
>
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index af2f513..4503e9c 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -74,9 +74,64 @@ int fdt_ro_probe_(const void *fdt)
> return 0;
> }
>
> +static int check_off_(uint32_t hdrsize, uint32_t totalsize, uint32_t off)
Make it return "bool"? When it is "int", 0 is rather "success" (like
fdt_check_header does). Same about check_block_ below.
> +{
> + return (off >= hdrsize) && (off <= totalsize);
> +}
> +
> +static int check_block_(uint32_t hdrsize, uint32_t totalsize,
> + uint32_t base, uint32_t size)
> +{
> + if (!check_off_(hdrsize, totalsize, base))
> + return 0; /* block start out of bounds */
> + if ((base + size) < base)
> + return 0; /* overflow */
> + if (!check_off_(hdrsize, totalsize, base + size))
> + return 0; /* block end out of bounds */
> + return 1;
> +}
> +
> int fdt_check_header(const void *fdt)
> {
> - return fdt_ro_probe_(fdt);
> + size_t hdrsize = FDT_V16_SIZE;
> +
> + if (fdt_magic(fdt) != FDT_MAGIC)
> + return -FDT_ERR_BADMAGIC;
> + if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> + || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> + return -FDT_ERR_BADVERSION;
> + if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> + return -FDT_ERR_BADVERSION;
> +
> + if (fdt_version(fdt) >= 17)
> + hdrsize = FDT_V17_SIZE;
> +
> + if ((fdt_totalsize(fdt) < hdrsize)
> + || (fdt_totalsize(fdt) > INT_MAX))
> + return -FDT_ERR_TRUNCATED;
> +
> + /* Bounds check memrsv block */
> + if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
> + return -FDT_ERR_TRUNCATED;
> +
> + /* Bounds check structure block */
> + if (fdt_version(fdt) < 17) {
17, not 16? I do not have v16 handy though (do you? I'd have a copy), only
Devicetree Specification, Release 0.1, which seems to use the same
structure as v16.
> + if (!check_off_(hdrsize, fdt_totalsize(fdt),
> + fdt_off_dt_struct(fdt)))
> + return -FDT_ERR_TRUNCATED;
> + } else {
> + if (!check_block_(hdrsize, fdt_totalsize(fdt),
> + fdt_off_dt_struct(fdt),
> + fdt_size_dt_struct(fdt)))
> + return -FDT_ERR_TRUNCATED;
> + }
> +
> + /* Bounds check strings block */
> + if (!check_block_(hdrsize, fdt_totalsize(fdt),
> + fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
> + return -FDT_ERR_TRUNCATED;
> +
> + return 0;
> }
>
> const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index c8c00fa..1032d38 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -90,8 +90,9 @@
>
> /* Error codes: codes for bad device tree blobs */
> #define FDT_ERR_TRUNCATED 8
> - /* FDT_ERR_TRUNCATED: Structure block of the given device tree
> - * ends without an FDT_END tag. */
> + /* FDT_ERR_TRUNCATED: FDT or a sub-block is improperly
> + * terminated (overflows, goes outside allowed bounds, or
> + * isn't properly terminated). */
> #define FDT_ERR_BADMAGIC 9
> /* FDT_ERR_BADMAGIC: Given "device tree" appears not to be a
> * device tree at all - it is missing the flattened device
> diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h
> index bd24746..eb20538 100644
> --- a/libfdt/libfdt_env.h
> +++ b/libfdt/libfdt_env.h
> @@ -56,6 +56,7 @@
> #include <stdint.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <limits.h>
>
> #ifdef __CHECKER__
> #define FDT_FORCE __attribute__((force))
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 9e209d5..1c25219 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -8,6 +8,7 @@ tmp.*
> /asm_tree_dump
> /boot-cpuid
> /char_literal
> +/check_header
> /check_path
> /del_node
> /del_property
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index 262944a..1f0ba99 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -26,7 +26,7 @@ LIB_TESTS_L = get_mem_rsv \
> property_iterate \
> subnode_iterate \
> overlay overlay_bad_fixup \
> - check_path
> + check_path check_header
> LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
>
> LIBTREE_TESTS_L = truncated_property
> diff --git a/tests/check_header.c b/tests/check_header.c
> new file mode 100644
> index 0000000..5e37813
> --- /dev/null
> +++ b/tests/check_header.c
> @@ -0,0 +1,128 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + * Testcase for fdt_check_header
> + * Copyright (C) 2018 David Gibson
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public License
> + * as published by the Free Software Foundation; either version 2.1 of
> + * the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdio.h>
> +
> +#include <libfdt.h>
> +
> +#include "tests.h"
> +
> +static void *dtdup(void *dt)
> +{
> + size_t bufsize = fdt_totalsize(dt);
> + void *buf = xmalloc(bufsize);
> + fdt_move(dt, buf, bufsize);
> + return buf;
> +}
> +
> +#define CHECK_MANGLE(exerr, code) \
> + do { \
> + void *fdt = dtdup(template); \
> + { code } \
> + err = fdt_check_header(fdt); \
> + verbose_printf("\"%s\" => %s\n", #code, fdt_strerror(err)); \
> + if (err != (exerr)) \
> + FAIL("fdt_check_header() didn't catch mangle %s", \
> + #code); \
> + free(fdt); \
> + } while (0)
> +
> +int main(int argc, char *argv[])
> +{
> + void *template;
> + int err;
> +
> + test_init(argc, argv);
> + template = load_blob(argv[1]);
> +
> + /* Check that the base dt is valid before mangling it */
> + err = fdt_check_header(template);
> + if (err != 0)
> + FAIL("Base tree fails: %s", fdt_strerror(err));
> +
> + /* Check a no-op mangle doesn't break things */
> + CHECK_MANGLE(0, ; );
> +
> + /* Mess up the magic number */
> + CHECK_MANGLE(-FDT_ERR_BADMAGIC,
> + fdt_set_magic(fdt, fdt_magic(fdt) ^ 0x1);
> + );
> + CHECK_MANGLE(-FDT_ERR_BADMAGIC,
> + fdt_set_magic(fdt, fdt_magic(fdt) ^ 0x80000000);
> + );
> +
> + /* Mess up the version */
> + CHECK_MANGLE(-FDT_ERR_BADVERSION,
> + fdt_set_version(fdt, FDT_FIRST_SUPPORTED_VERSION - 1);
> + fdt_set_last_comp_version(fdt, FDT_FIRST_SUPPORTED_VERSION - 1);
> + );
> + CHECK_MANGLE(-FDT_ERR_BADVERSION,
> + fdt_set_version(fdt, FDT_LAST_SUPPORTED_VERSION + 1);
> + fdt_set_last_comp_version(fdt, FDT_LAST_SUPPORTED_VERSION + 1);
> + );
> + CHECK_MANGLE(-FDT_ERR_BADVERSION,
> + fdt_set_version(fdt, FDT_FIRST_SUPPORTED_VERSION);
> + fdt_set_last_comp_version(fdt, FDT_LAST_SUPPORTED_VERSION);
> + );
> +
> + /* Out of bounds sizes */
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_totalsize(fdt, FDT_V1_SIZE - 1);
> + );
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_totalsize(fdt, (uint32_t)INT_MAX + 1);
> + );
> +
> + /* Truncate within various blocks */
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_totalsize(fdt, fdt_off_dt_struct(fdt) - 1);
> + );
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_totalsize(fdt, fdt_off_dt_strings(fdt) - 1);
> + );
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_totalsize(fdt, fdt_off_mem_rsvmap(fdt) - 1);
> + );
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_totalsize(fdt, fdt_off_dt_struct(fdt) + 1);
> + );
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_totalsize(fdt, fdt_off_dt_strings(fdt) + 1);
> + );
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_totalsize(fdt, fdt_off_mem_rsvmap(fdt) + 1);
> + );
> +
> + /* Negative block sizes */
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_size_dt_struct(fdt, (uint32_t)-1);
> + );
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_size_dt_strings(fdt, (uint32_t)-1);
> + );
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_size_dt_struct(fdt, (uint32_t)INT_MIN);
> + );
> + CHECK_MANGLE(-FDT_ERR_TRUNCATED,
> + fdt_set_size_dt_strings(fdt, (uint32_t)INT_MIN);
> + );
> +
> + PASS();
> +}
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index d937260..f2a0198 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -440,6 +440,9 @@ libfdt_tests () {
> run_wrap_error_test $DTC nul-in-line-info2.dts
>
> run_wrap_error_test $DTC -I dtb -O dts -o /dev/null ovf_size_strings.dtb
> +
> + # Check header tests
The comment does not seem to add anything, all 3 words are just in the
command below :)
> + run_test check_header test_tree1.dtb
> }
>
> dtc_tests () {
>
--
Alexey
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
