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.
>
> >
> > 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.
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.
Regards,
Simon
