On Tue, Nov 05, 2019 at 05:46:06AM -0700, Simon Glass wrote:
> Hi David,
>
> On Mon, 4 Nov 2019 at 10:06, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Oct 24, 2019 at 09:29:22PM -0600, Simon Glass wrote:
> > > Allow enabling FDT_ASSUME_SANE to disable basic checks.
> > >
> > > Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> > > ---
> > >
> > > Changes in v3: None
> > > Changes in v2: None
> > >
> > > libfdt/fdt.c | 35 ++++++++++++++++++++---------------
> > > libfdt/fdt_ro.c | 2 +-
> > > libfdt/fdt_rw.c | 22 +++++++++++++++++++---
> > > libfdt/fdt_sw.c | 13 ++++++++-----
> > > libfdt/libfdt_internal.h | 7 +++++--
> > > 5 files changed, 53 insertions(+), 26 deletions(-)
> > >
>
>
> [..]
>
> > > diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> > > index 8795947..d3750f5 100644
> > > --- a/libfdt/fdt_rw.c
> > > +++ b/libfdt/fdt_rw.c
>
> [..]
>
> > > @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
> > >
> > > static int fdt_rw_probe_(void *fdt)
> > > {
> > > + if (!fdt_chk_basic())
> > > + return 0;
> > > FDT_RO_PROBE(fdt);
> > >
> > > if (fdt_version(fdt) < 17)
> > > @@ -112,6 +116,15 @@ static int fdt_splice_string_(void *fdt, int newlen)
> > > return 0;
> > > }
> > >
> > > +/**
> > > + * fdt_find_add_string_() - Find or allocate a string
> > > + *
> > > + * @fdt: pointer to the device tree to check/adjust
> > > + * @s: string to find/add
> > > + * @allocated: Set to 0 if the string was found, 1 if not found and so
> > > + * allocated. Ignored if !fdt_chk_basic()
> > > + * @return offset of string in the string table (whether found or added)
> > > + */
> > > static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
> > > {
> > > char *strtab = (char *)fdt + fdt_off_dt_strings(fdt);
> > > @@ -120,7 +133,8 @@ static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
> > > int len = strlen(s) + 1;
> > > int err;
> > >
> > > - *allocated = 0;
> > > + if (fdt_chk_basic())
> > > + *allocated = 0;
> >
> > Putting the setting of *allocated behind the gate requires a comment I
> > think - with ASSUME_SANE enabled, this means that *allocated will not
> > be initialized by this function which could easily be a gotcha.
>
> I put a comment in the function header above; will add another here.
Ah, I missed the comment in the function header, sorry. That's
probably ok.
>
> >
> > >
> > > p = fdt_find_string_(strtab, fdt_size_dt_strings(fdt), s);
> > > if (p)
> > > @@ -132,7 +146,8 @@ static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
> > > if (err)
> > > return err;
> > >
> > > - *allocated = 1;
> > > + if (fdt_chk_basic())
> > > + *allocated = 1;
> > >
> > > memcpy(new, s, len);
> > > return (new - strtab);
> > > @@ -206,7 +221,8 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
> > >
> > > err = fdt_splice_struct_(fdt, *prop, 0, proplen);
> > > if (err) {
> > > - if (allocated)
> > > + /* Delete the string if we failed to add it */
> > > + if (fdt_chk_basic() && allocated)
> > > fdt_del_last_string_(fdt, name);
> >
> > This doesn't seem like an obvious change to go into this asusmption
> > option. IIRC this can fail because we ran out of space in the buffer,
> > which isn't really in keeping with the other assumptions we're making
> > here - that can happen even with a correct tree and correct arguments
> > to all the functions.
>
> This 'allocated' code didn't exist in libfdt until recently and it is
> only handling backing out of an error condition gracefully.
Right, it's certainly a reasonable thing to skip with one of these
flags, I just don't think this is the right one.
> Should I separate this out into another assumption type? Maybe
> ASSUME_ENOUGH_SPACE? It still leaves the DT in a sane state, just with
> an extra string in the table.
Another assumption type certainly.
So ASSUME_ENOUGH_SPACE seems a reasonable flag to have, but it should
do rather more than just this (there are a heap of bounds checks in
the rw and sw code you could skip with that flag).
Up to you if you also want a finer grained one that skips this
rollback, but not the other out of space bounds checks. A good name
for such a flag is not yet occuring to me, I'm afraid.
--
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
