On Sun, Apr 22, 2018 at 08:10:51PM +0000, Simon Glass wrote:
> Hi David,
>
> On 17 April 2018 at 21:39, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > On Tue, Apr 17, 2018 at 11:11:11AM -0400, Simon Glass wrote:
> >> Hi David,
> >>
> >> On 11 April 2018 at 22:52, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> wrote:
> >> > libfdt is never supposed to access memory outside the the blob, or
> outside
> >> > the sub-blocks within it, even if the blob is badly corrupted.
> >> >
> >> > We can leverage valgrind's client requests to do better testing of
> this.
> >> > This adds a vg_prepare_blob() function which marks just the valid
> parts of
> >> > an fdt blob as properly initialized, explicitly marking the rest as
> >> > uninitialized. This means valgrind should catch any bad accesses.
> >> >
> >> > We add a call to vg_prepare_blob() to load_blob() so that lots of the
> >> > existing testcases will benefit from the extra checking.
> >> >
> >> > Signed-off-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> >> > ---
> >> > tests/tests.h | 1 +
> >> > tests/testutils.c | 52
> +++++++++++++++++++++++++++++++++++++++++++++-
> >> > tests/truncated_property.c | 2 ++
> >> > tests/truncated_string.c | 2 ++
> >> > 4 files changed, 56 insertions(+), 1 deletion(-)
> >> >
> >>
> >> Reviewed-by: Simon Glass <sjg@xxxxxxxxxxxx>
> >>
> >> Please see below
> >>
> >> [..]
> >>
> >> > diff --git a/tests/testutils.c b/tests/testutils.c
> >> > index d6d6818..ea8a022 100644
> >> > --- a/tests/testutils.c
> >> > +++ b/tests/testutils.c
> >> > @@ -161,14 +161,64 @@ int nodename_eq(const char *s1, const char *s2)
> >> > return 0;
> >> > }
> >> >
> >> > +void vg_prepare_blob(void *fdt, size_t bufsize)
> >> > +{
> >> > + char *blob = fdt;
> >> > + int off_memrsv, off_strings, off_struct;
> >> > + size_t size_memrsv, size_strings, size_struct;
> >> > +
> >> > + size_memrsv = (fdt_num_mem_rsv(fdt) + 1)
> >> > + * sizeof(struct fdt_reserve_entry);
> >> > +
> >> > + VALGRIND_MAKE_MEM_UNDEFINED(blob, bufsize);
> >> > + VALGRIND_MAKE_MEM_DEFINED(blob, FDT_V1_SIZE);
> >> > + VALGRIND_MAKE_MEM_DEFINED(blob, fdt_header_size(fdt));
> >> > +
> >> > + if (fdt_magic(fdt) == FDT_MAGIC) {
> >> > + off_memrsv = fdt_off_mem_rsvmap(fdt);
> >> > +
> >> > + off_strings = fdt_off_dt_strings(fdt);
> >> > + if (fdt_version(fdt) >= 3)
> >> > + size_strings = fdt_size_dt_strings(fdt);
> >> > + else
> >> > + size_strings = fdt_totalsize(fdt) -
> off_strings;
> >> > +
> >> > + off_struct = fdt_off_dt_struct(fdt);
> >> > + if (fdt_version(fdt) >= 17)
> >> > + size_struct = fdt_size_dt_struct(fdt);
> >> > + else
> >> > + size_struct = fdt_totalsize(fdt) - off_struct;
> >> > + } else if (fdt_magic(fdt) == (~FDT_MAGIC)) {
> >>
> >> I suggest making ~FDT_MAGIC a separate constant (in another patch).
> >
> > So, within libfdt proper it already is: FDT_SW_MAGIC defined in
> > libfdt/libfdt_internal.h.
> >
> > This, however, is in the test code where I'm dubious about including
> > that otherwise entirely internal header.
> >
> > I'm inclined to leave it as is, at least until we get a second place
> > that wants to use it in the test code.
>
> OK I see. I thought this was trying to be FDT_SW_MAGIC which has the same
> value...
Actually on second thoughts, it's not unreasonable for the test code
to include the internal header, so I've done that and am now using
FDT_SW_MAGIC here.
--
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
