On Thu, Apr 05, 2018 at 12:21:02PM +1000, Alexey Kardashevskiy wrote:
> On 4/4/18 9:06 pm, David Gibson wrote:
> > On Tue, Mar 27, 2018 at 06:09:12PM +1100, Alexey Kardashevskiy wrote:
> >> 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.
> >
> > Ah. So, technically spapr_h_cas_compose_response() is just doing the
> > wrong thing here. You're supposed to construct a mem reserve segment
> > between fdt_create() and the first fdt_begin_node(), even if that's
> > just a single call to fdt_finish_reservemap().
>
> Fair enough, I've posted a patch for QEMU.
>
> > That said, it would be good for libfdt to be more robust against
> > that. Either fdt_begin_node() should return FDT_ERR_BADSTATE in this
> > situatipon, or it should automatically finish off the reserve map.
>
> Returning an error from fdt_begin_node() should do it. Also probably
> including fdt_finish() as it looks like it is possible to call fdt_create()
> and fdt_finish() without creating nodes in between (have not tested this).
>
> Automation is supposed to fix only one place in QEMU for now and it just
> does not seem worth it.
Fair enough. I've fixed libfdt to return BADSTATE if you try to begin
a node before completing the reservemap.
--
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
