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