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().
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.
--
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
