On Mon, Mar 26, 2018 at 04:54:11PM +1100, Alexey Kardashevskiy wrote:
> 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.
Yeah, it's logically a bool.
So far I haven't used the bool type within libfdt, though, for fear of
incompatibility on weird embedded compilers. It might be worth
changing that, but that would be a standalone patch, rather than
folded in with an unrelated change.
> > +{
> > + 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.
Yes. size_dt_struct was only added in v17 (in fact, I think that's
the only change from v16 to v17).
> > + 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 :)
True, when I wrote it I think I was expecting to add a bunch more
tests under the heading. I'll take it out.
>
>
> > + run_test check_header test_tree1.dtb
> > }
> >
> > dtc_tests () {
> >
>
>
--
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:
signature.asc
Description: PGP signature
