On 26/3/18 10:25 am, David Gibson wrote:
> fdt_num_mem_rsv() and fdt_get_mem_rsv() currently don't sanity check their
> parameters, or the memory reserve section offset in the header. That means
> that on a corrupted blob they could access outside of the range of memory
> that they should.
>
> This improves their safety checking, meaning they shouldn't access outside
> the blob's bounds, even if its contents are badly corrupted.
>
> Signed-off-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> ---
> libfdt/fdt_ro.c | 33 ++++++++++++++++++++-----
> tests/.gitignore | 1 +
> tests/Makefile.tests | 2 +-
> tests/run_tests.sh | 1 +
> tests/testdata.h | 1 +
> tests/trees.S | 20 +++++++++++++++
> tests/truncated_memrsv.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 114 insertions(+), 7 deletions(-)
> create mode 100644 tests/truncated_memrsv.c
>
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index d4cec0e..c74b962 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -170,21 +170,42 @@ uint32_t fdt_get_max_phandle(const void *fdt)
> return 0;
> }
>
> +static const struct fdt_reserve_entry *fdt_mem_rsv(const void *fdt, int n)
> +{
> + int offset = n * sizeof(struct fdt_reserve_entry);
> + int absoffset = fdt_off_mem_rsvmap(fdt) + offset;
> +
> + if (absoffset < fdt_off_mem_rsvmap(fdt))
> + return NULL;
> + if (absoffset > fdt_totalsize(fdt) - sizeof(struct fdt_reserve_entry))
> + return NULL;
> + return fdt_mem_rsv_(fdt, n);
> +}
> +
> int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
> {
> + const struct fdt_reserve_entry *re;
> +
> FDT_RO_PROBE(fdt);
> - *address = fdt64_to_cpu(fdt_mem_rsv_(fdt, n)->address);
> - *size = fdt64_to_cpu(fdt_mem_rsv_(fdt, n)->size);
> + re = fdt_mem_rsv(fdt, n);
> + if (!re)
> + return -FDT_ERR_BADOFFSET;
> +
> + *address = fdt64_to_cpu(re->address);
> + *size = fdt64_to_cpu(re->size);
> return 0;
> }
>
> int fdt_num_mem_rsv(const void *fdt)
> {
> - int i = 0;
> + int i;
> + const struct fdt_reserve_entry *re;
>
> - while (fdt64_to_cpu(fdt_mem_rsv_(fdt, i)->size) != 0)
> - i++;
> - return i;
> + for (i = 0; (re = fdt_mem_rsv(fdt, i)) != NULL; i++) {
> + if (fdt64_to_cpu(re->size) == 0)
> + return i;
> + }
> + return -FDT_ERR_TRUNCATED;
QEMU's spapr_h_cas_compose_response() crashes on this one as it calls
fdt_open_into() and that guy does not check if fdt_num_mem_rsv() fails and
it does as there is no any reserved entry, even an empty one in the end of
the list.
fdt_create() could create an empty record or fdt_open_into() could test for
an error, not sure which one is right here.
> }
>
> static int nextprop_(const void *fdt, int offset)
> diff --git a/tests/.gitignore b/tests/.gitignore
> index f643cf9..60bbb09 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -61,5 +61,6 @@ tmp.*
> /sw_tree1
> /truncated_property
> /truncated_string
> +/truncated_memrsv
> /utilfdt_test
> /value-labels
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index 7c04ff9..fc1b160 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -29,7 +29,7 @@ LIB_TESTS_L = get_mem_rsv \
> check_path check_header
> LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
>
> -LIBTREE_TESTS_L = truncated_property truncated_string
> +LIBTREE_TESTS_L = truncated_property truncated_string truncated_memrsv
> LIBTREE_TESTS = $(LIBTREE_TESTS_L:%=$(TESTS_PREFIX)%)
>
> DL_LIB_TESTS_L = asm_tree_dump value-labels
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 8938d8b..ee5a43a 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -415,6 +415,7 @@ libfdt_tests () {
> # Tests for behaviour on various sorts of corrupted trees
> run_test truncated_property
> run_test truncated_string
> + run_test truncated_memrsv
>
> # Check aliases support in fdt_path_offset
> run_dtc_test -I dts -O dtb -o aliases.dtb aliases.dts
> diff --git a/tests/testdata.h b/tests/testdata.h
> index b257011..68dcbac 100644
> --- a/tests/testdata.h
> +++ b/tests/testdata.h
> @@ -48,4 +48,5 @@ extern struct fdt_header bad_node_format;
> extern struct fdt_header bad_prop_char;
> extern struct fdt_header ovf_size_strings;
> extern struct fdt_header truncated_string;
> +extern struct fdt_header truncated_memrsv;
> #endif /* ! __ASSEMBLY */
> diff --git a/tests/trees.S b/tests/trees.S
> index 7ae1bde..efab287 100644
> --- a/tests/trees.S
> +++ b/tests/trees.S
> @@ -259,3 +259,23 @@ truncated_string_bad_string:
> /* NOTE: terminating \0 deliberately missing */
> truncated_string_strings_end:
> truncated_string_end:
> +
> +
> + /* truncated_memrsv */
> + TREE_HDR(truncated_memrsv)
> +
> +truncated_memrsv_struct:
> + BEGIN_NODE("")
> + END_NODE
> + FDTLONG(FDT_END)
> +truncated_memrsv_struct_end:
> +
> +truncated_memrsv_strings:
> +truncated_memrsv_strings_end:
> +
> + .balign 8
> +truncated_memrsv_rsvmap:
> + RSVMAP_ENTRY(TEST_ADDR_1H, TEST_ADDR_1L, TEST_SIZE_1H, TEST_SIZE_1L)
> +truncated_memrsv_rsvmap_end:
> +
> +truncated_memrsv_end:
> diff --git a/tests/truncated_memrsv.c b/tests/truncated_memrsv.c
> new file mode 100644
> index 0000000..2013e98
> --- /dev/null
> +++ b/tests/truncated_memrsv.c
> @@ -0,0 +1,63 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + * Testcase for misbehaviour on a truncated string
> + * Copyright (C) 2018 David Gibson, IBM Corporation.
> + *
> + * 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 <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[])
> +{
> + void *fdt = &truncated_memrsv;
> + int err;
> + uint64_t addr, size;
> +
> + test_init(argc, argv);
> +
> + err = fdt_check_header(fdt);
> + if (err != 0)
> + FAIL("Bad header: %s", fdt_strerror(err));
> +
> + err = fdt_num_mem_rsv(fdt);
> + if (err != -FDT_ERR_TRUNCATED)
> + FAIL("fdt_num_mem_rsv() returned %d instead of -FDT_ERR_TRUNCATED",
> + err);
> +
> + err = fdt_get_mem_rsv(fdt, 0, &addr, &size);
> + if (err != 0)
> + FAIL("fdt_get_mem_rsv() failed on first entry: %s",
> + fdt_strerror(err));
> + if ((addr != TEST_ADDR_1) || (size != TEST_SIZE_1))
> + FAIL("Entry doesn't match: (0x%llx, 0x%llx) != (0x%llx, 0x%llx)",
> + (unsigned long long)addr, (unsigned long long)size,
> + TEST_ADDR_1, TEST_SIZE_1);
> +
> + err = fdt_get_mem_rsv(fdt, 1, &addr, &size);
> + if (err != -FDT_ERR_BADOFFSET)
> + FAIL("fdt_get_mem_rsv(1) returned %d instead of -FDT_ERR_BADOFFSET",
> + err);
> +
> + PASS();
> +}
>
--
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
