Hi David,
On Fri, 9 Aug 2019 at 11:02, Simon Glass <sjg@xxxxxxxxxxxx> wrote:
>
> Hi David,
>
> On Fri, 9 Aug 2019 at 05:53, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Aug 01, 2019 at 01:11:29PM -0600, Simon Glass wrote:
> > > Hi David,
> > >
> > > On Mon, 22 Jul 2019 at 01:25, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Sat, Jul 20, 2019 at 05:31:09PM -0600, Simon Glass wrote:
> > > > > This file provides read-write access to the device tree and includes
> > > > > mostly checks of the header. Allow these checks to be disabled to reduce
> > > > > code size.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> > > > > ---
> > > > >
> > > > > libfdt/fdt_rw.c | 14 ++++++++++----
> > > > > 1 file changed, 10 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> > > > > index 8795947..4ff3aab 100644
> > > > > --- a/libfdt/fdt_rw.c
> > > > > +++ b/libfdt/fdt_rw.c
> > > > > @@ -13,6 +13,8 @@
> > > > > static int fdt_blocks_misordered_(const void *fdt,
> > > > > int mem_rsv_size, int struct_size)
> > > > > {
> > > > > + if (!_check2())
> > > > > + return false;
> > > >
> > > > I think this needs to be check1. An fdt with the blocks in the
> > > > "wrong" order isn't actually invalid - it's just inconvenient for our
> > > > purposes here. So if you disable this check, you're potentially
> > > > making this misbehave on valid blobs, not merely trusting that the
> > > > blob you have is valid.
> > >
> > > Yes, fixed.
> > >
> > > >
> > > > > return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
> > > > > || (fdt_off_dt_struct(fdt) <
> > > > > (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
> > > > > @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
> > > > >
> > > > > static int fdt_rw_probe_(void *fdt)
> > > > > {
> > > > > + if (!_check2())
> > > > > + return 0;
> > > > > FDT_RO_PROBE(fdt);
> > > > >
> > > > > if (fdt_version(fdt) < 17)
> > > > > @@ -40,7 +44,7 @@ static int fdt_rw_probe_(void *fdt)
> > > > > #define FDT_RW_PROBE(fdt) \
> > > > > { \
> > > > > int err_; \
> > > > > - if ((err_ = fdt_rw_probe_(fdt)) != 0) \
> > > > > + if (_check2() && (err_ = fdt_rw_probe_(fdt)) != 0) \
> > > > > return err_; \
> > > > > }
> > > > >
> > > > > @@ -120,7 +124,8 @@ static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
> > > > > int len = strlen(s) + 1;
> > > > > int err;
> > > > >
> > > > > - *allocated = 0;
> > > > > + if (_check1())
> > > > > + *allocated = 0;
> > > > >
> > > > > p = fdt_find_string_(strtab, fdt_size_dt_strings(fdt), s);
> > > > > if (p)
> > > > > @@ -132,7 +137,8 @@ static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
> > > > > if (err)
> > > > > return err;
> > > > >
> > > > > - *allocated = 1;
> > > > > + if (_check1())
> > > > > + *allocated = 1;
> > > >
> > > > IIUC you're also using the check level to avoid searching for an
> > > > existing string and just adding it (possibly duplicated). That's
> > > > potentially a useful thing - I've had other requests for that to avoid
> > > > a potentially slow string search. However, I'm a bit uncomfortable
> > > > with hooking it to something that's allegedly just about checking.
> > > > This will change the behaviour of the library even on correct blobs.
> > >
> > > I don't think so. It's just that if we don't care whether the string
> > > was found or allocated, unless we want to delete the string when we
> > > run out of space. This new feature was not present earlier, and wastes
> > > code space if you don't care.
> >
> > It's true that we don't usually care whether the string is found or
> > allocated. But - for a correct starting dt and correct usage - your
> > other changes will result in byte-for-byte identical output. This one
> > won't.
>
> Yes that's right. Of course this is how it used to work until
> recently, and running out of space is an error condition.
>
> So should I change it to always enable this code except when all
> checks are disabled?
Just checking in on this. I should point out that errors like this
don't typically happen in constrained environments since they are
tested for such problems. If an error happens, then the likely outcome
is that the DT is expanded and the operation retried.
Also, is that your only comment on the series? So I should respin it
once we figure this one out.
- Simon
>
> >
> > > Part of the problem is that fdt_find_add_string_() has no function
> > > comment, so I'll have a crack at adding one.
> > >
> > > >
> > > > >
> > > > > memcpy(new, s, len);
> > > > > return (new - strtab);
> > > > > @@ -206,7 +212,7 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
> > > > >
> > > > > err = fdt_splice_struct_(fdt, *prop, 0, proplen);
> > > > > if (err) {
> > > > > - if (allocated)
> > > > > + if (_check1() && allocated)
> > > > > fdt_del_last_string_(fdt, name);
> > > > > return err;
> > > > > }
> > >
> > > Regards,
> > > Simon
> > >
> Regards,
> Simon
